Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Tue, 17 May 2022 12:28:08 -0700
From:   Matthias Kaehlcke <mka@chromium.org>
To:     Kees Cook <keescook@chromium.org>
Cc:     Alasdair Kergon <agk@redhat.com>,
        Mike Snitzer <snitzer@kernel.org>,
        James Morris <jmorris@namei.org>,
        "Serge E . Hallyn" <serge@hallyn.com>, dm-devel@redhat.com,
        linux-kernel@vger.kernel.org, linux-raid@vger.kernel.org,
        Song Liu <song@kernel.org>,
        Douglas Anderson <dianders@chromium.org>,
        linux-security-module@vger.kernel.org
Subject: Re: [PATCH v3 2/3] LoadPin: Enable loading from trusted dm-verity
 devices
Message-ID: <YoP3SJ62uHI7nrXy@google.com>
References: <20220504195419.1143099-1-mka@chromium.org>
 <20220504125404.v3.2.I01c67af41d2f6525c6d023101671d7339a9bc8b5@changeid>
 <B7FB2BE6-DF1C-414A-B4C2-0C15FD1CBF75@chromium.org>
 <YoKVSEJyHvFg/V9+@google.com>
 <202205162038.B2D1BBAB3@keescook>
MIME-Version: 1.0
Content-Type: text/plain; charset=utf-8
Content-Disposition: inline
In-Reply-To: <202205162038.B2D1BBAB3@keescook>
Precedence: bulk

On Mon, May 16, 2022 at 08:44:37PM -0700, Kees Cook wrote:
> On Mon, May 16, 2022 at 11:17:44AM -0700, Matthias Kaehlcke wrote:
> > On Fri, May 13, 2022 at 03:36:26PM -0700, Kees Cook wrote:
> > > 
> > > 
> > > On May 4, 2022 12:54:18 PM PDT, Matthias Kaehlcke <mka@chromium.org> wrote:
> > > >Extend LoadPin to allow loading of kernel files from trusted dm-verity [1]
> > > >devices.
> > > >
> > > >This change adds the concept of trusted verity devices to LoadPin. LoadPin
> > > >maintains a list of root digests of verity devices it considers trusted.
> > > >Userspace can populate this list through an ioctl on the new LoadPin
> > > >securityfs entry 'dm-verity'. The ioctl receives a file descriptor of
> > > >a file with verity digests as parameter. Verity reads the digests from
> > > >this file after confirming that the file is located on the pinned root.
> > > >The list of trusted digests can only be set up once, which is typically
> > > >done at boot time.
> > > >
> > > >When a kernel file is read LoadPin first checks (as usual) whether the file
> > > >is located on the pinned root, if so the file can be loaded. Otherwise, if
> > > >the verity extension is enabled, LoadPin determines whether the file is
> > > >located on a verity backed device and whether the root digest of that
> > > 
> > > I think this should be "... on an already trusted device ..."
> > 
> > It's not entirely clear which part you want me to substitute. 'an already
> > trusted device' makes me wonder whether you are thinking about reading the
> > list of digests, and not the general case of reading a kernel file, which
> > this paragraph intends to describe.
> 
> Sorry, I think I confused myself while reading what you'd written. I
> think it's fine as is. I think I had skipped around in my mind thinking
> about the trusted verity hashes file coming from the pinned root, but
> you basically already said that. :) Nevermind!
> 
> > > >+static int read_trusted_verity_root_digests(unsigned int fd)
> > > >+{
> > > >+	struct fd f;
> > > >+	void *data;
> > > 
> > > Probably easier if this is u8 *?
> > 
> > Maybe slightly, it would then require a cast when passing it to
> > kernel_read_file()
> 
> Oh, good point. That is a kinda weird API.
> 
> > 
> > > >+	int rc;
> > > >+	char *p, *d;
> > > >+
> > > >+	/* The list of trusted root digests can only be set up once */
> > > >+	if (!list_empty(&trusted_verity_root_digests))
> > > >+		return -EPERM;
> > > >+
> > > >+	f = fdget(fd);
> > > >+	if (!f.file)
> > > >+		return -EINVAL;
> > > >+
> > > >+	data = kzalloc(SZ_4K, GFP_KERNEL);
> > > >+	if (!data) {
> > > >+		rc = -ENOMEM;
> > > >+		goto err;
> > > >+	}
> > > >+
> > > >+	rc = kernel_read_file(f.file, 0, &data, SZ_4K - 1, NULL, READING_POLICY);
> > > >+	if (rc < 0)
> > > >+		goto err;
> 
> So maybe, here, you could do:
> 
> 	p = data;
> 	p[rc] '\0';
> 	p = strim(p);
> 
> etc... (the void * -> char * cast in the assignment should be accepted
> without warning?)

Yes, that would work, I'll change it accordingly, thanks!

> > > >+
> > > >+	((char *)data)[rc] = '\0';
> > > >+
> > > >+	p = strim(data);
> > > >+	while ((d = strsep(&p, ",")) != NULL) {
> > > 
> > > Maybe be flexible and add newline as a separator too?
> > 
> > Sure, I can add that. I'd also be fine with just allowing a newline as
> > separator, which seems a reasonable format for a sysfs file.
> 
> Yeah, that was my thinking too. And easier to parse for command line
> tools, etc. Not a requirement at all, but might make testing easier,
> etc.

Ok, I'll change it to use newline as the only separator.