All of lore.kernel.org
 help / color / mirror / Atom feed
From: Kees Cook <keescook@chromium.org>
To: Matthias Kaehlcke <mka@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
Date: Mon, 16 May 2022 20:44:37 -0700	[thread overview]
Message-ID: <202205162038.B2D1BBAB3@keescook> (raw)
In-Reply-To: <YoKVSEJyHvFg/V9+@google.com>

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?)

> > >+
> > >+	((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.

-- 
Kees Cook

WARNING: multiple messages have this Message-ID
From: Kees Cook <keescook@chromium.org>
To: Matthias Kaehlcke <mka@chromium.org>
Cc: linux-security-module@vger.kernel.org,
	Douglas Anderson <dianders@chromium.org>,
	Mike Snitzer <snitzer@kernel.org>,
	linux-kernel@vger.kernel.org, James Morris <jmorris@namei.org>,
	linux-raid@vger.kernel.org, Song Liu <song@kernel.org>,
	dm-devel@redhat.com, Alasdair Kergon <agk@redhat.com>,
	"Serge E . Hallyn" <serge@hallyn.com>
Subject: Re: [dm-devel] [PATCH v3 2/3] LoadPin: Enable loading from trusted dm-verity devices
Date: Mon, 16 May 2022 20:44:37 -0700	[thread overview]
Message-ID: <202205162038.B2D1BBAB3@keescook> (raw)
In-Reply-To: <YoKVSEJyHvFg/V9+@google.com>

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?)

> > >+
> > >+	((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.

-- 
Kees Cook

--
dm-devel mailing list
dm-devel@redhat.com
https://listman.redhat.com/mailman/listinfo/dm-devel


  reply	other threads:[~2022-05-17  3:44 UTC|newest]

Thread overview: 42+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2022-05-04 19:54 [PATCH v3 0/3] " Matthias Kaehlcke
2022-05-04 19:54 ` [dm-devel] " Matthias Kaehlcke
2022-05-04 19:54 ` [PATCH v3 1/3] dm: Add verity helpers for LoadPin Matthias Kaehlcke
2022-05-04 19:54   ` [dm-devel] " Matthias Kaehlcke
2022-05-11 20:54   ` Matthias Kaehlcke
2022-05-11 20:54     ` [dm-devel] " Matthias Kaehlcke
2022-05-12 17:19     ` Mike Snitzer
2022-05-12 17:19       ` [dm-devel] " Mike Snitzer
2022-05-12 18:14       ` Matthias Kaehlcke
2022-05-12 18:14         ` [dm-devel] " Matthias Kaehlcke
2022-05-12 20:44       ` Matthias Kaehlcke
2022-05-12 20:44         ` [dm-devel] " Matthias Kaehlcke
2022-05-13 16:29         ` Mike Snitzer
2022-05-13 16:29           ` [dm-devel] " Mike Snitzer
2022-05-13 16:53           ` Matthias Kaehlcke
2022-05-13 16:53             ` Matthias Kaehlcke
2022-05-13 22:15   ` Kees Cook
2022-05-13 22:15     ` [dm-devel] " Kees Cook
2022-05-16 18:51     ` Matthias Kaehlcke
2022-05-16 18:51       ` [dm-devel] " Matthias Kaehlcke
2022-05-17  3:38       ` Kees Cook
2022-05-17  3:38         ` [dm-devel] " Kees Cook
2022-05-04 19:54 ` [PATCH v3 2/3] LoadPin: Enable loading from trusted dm-verity devices Matthias Kaehlcke
2022-05-04 19:54   ` [dm-devel] " Matthias Kaehlcke
2022-05-04 22:26   ` kernel test robot
2022-05-04 22:26     ` [dm-devel] " kernel test robot
2022-05-13 16:32   ` Mike Snitzer
2022-05-13 16:32     ` [dm-devel] " Mike Snitzer
2022-05-13 17:01     ` Matthias Kaehlcke
2022-05-13 17:01       ` [dm-devel] " Matthias Kaehlcke
2022-05-13 18:26     ` Kees Cook
2022-05-13 18:26       ` [dm-devel] " Kees Cook
2022-05-13 22:36   ` Kees Cook
2022-05-13 22:36     ` [dm-devel] " Kees Cook
2022-05-16 18:17     ` Matthias Kaehlcke
2022-05-16 18:17       ` [dm-devel] " Matthias Kaehlcke
2022-05-17  3:44       ` Kees Cook [this message]
2022-05-17  3:44         ` Kees Cook
2022-05-17 19:28         ` Matthias Kaehlcke
2022-05-17 19:28           ` [dm-devel] " Matthias Kaehlcke
2022-05-04 19:54 ` [PATCH v3 3/3] dm: verity-loadpin: Use CONFIG_SECURITY_LOADPIN_VERITY for conditional compilation Matthias Kaehlcke
2022-05-04 19:54   ` [dm-devel] " Matthias Kaehlcke

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=202205162038.B2D1BBAB3@keescook \
    --to=keescook@chromium.org \
    --cc=agk@redhat.com \
    --cc=dianders@chromium.org \
    --cc=dm-devel@redhat.com \
    --cc=jmorris@namei.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=linux-raid@vger.kernel.org \
    --cc=linux-security-module@vger.kernel.org \
    --cc=mka@chromium.org \
    --cc=serge@hallyn.com \
    --cc=snitzer@kernel.org \
    --cc=song@kernel.org \
    --subject='Re: [PATCH v3 2/3] LoadPin: Enable loading from trusted dm-verity devices' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.