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@xxxxxxxxxxxx> 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
