On Sat, Dec 19, 2015 at 11:44:41PM -0500, Mimi Zohar wrote:
> On Thu, 2015-12-17 at 22:06 +0100, Luis R. Rodriguez wrote:
> > On Tue, Dec 08, 2015 at 01:01:22PM -0500, Mimi Zohar wrote:
> > > diff --git a/drivers/base/firmware_class.c b/drivers/base/firmware_class.c
> > > index 8524450..dcd902f 100644
> > > --- a/drivers/base/firmware_class.c
> > > +++ b/drivers/base/firmware_class.c
> > > @@ -29,6 +29,7 @@
> > >  #include <linux/syscore_ops.h>
> > >  #include <linux/reboot.h>
> > >  #include <linux/security.h>
> > > +#include <linux/ima.h>
> > >  
> > >  #include <generated/utsrelease.h>
> > >  
> > > @@ -305,11 +306,17 @@ static int fw_read_file_contents(struct file *file, 
> > > struct firmware_buf *fw_buf)
> > >   buf = vmalloc(size);
> > >   if (!buf)
> > >           return -ENOMEM;
> > > - rc = kernel_read(file, 0, buf, size);
> > > - if (rc != size) {
> > > -         if (rc > 0)
> > > -                 rc = -EIO;
> > > +
> > > + rc = ima_read_and_process_file(file, FIRMWARE_CHECK, buf, size);
> > > + if (rc == -EIO)
> > >           goto fail;
> > > + else if (rc != -EOPNOTSUPP) {
> > > +         rc = kernel_read(file, 0, buf, size);
> > > +         if (rc != size) {
> > > +                 if (rc > 0)
> > > +                         rc = -EIO;
> > > +                 goto fail;
> > > +         }
> > >   }
> > >   rc = security_kernel_fw_from_file(file, buf, size);
> > >   if (rc)
> > 
> > This is one way, the other way is to generalize the kernel-read from path
> > routine. I have some changes which help generalize this routine a bit so
> > help on review there would be appreciated. 
> 
> Sure.  Where are the patches?

http://lkml.kernel.org/r/1431996325-8840-2-git-send-email-mcg...@do-not-panic.com

I'll post these in PATCH form now.

> > I'm personally indifferent
> > as to needing or not *now* a generic kernel read routine that is shared
> > for this purpose *but* since this patch set *also* seems to be adding
> > yet-another file reading I'm more inclined to wish for that to be addressed
> > now instead.
> > 
> > Please let me know if this logic is fair.
> 
> Commit e3c4abb - "integrity: define a new function
> integrity_read_file()" defined a method of reading a file from the
> kernel.  It's used to load an x509 key onto the IMA keyring for systems
> without an initramfs.   Dmitry's patch, included in this patch set,
> calls this function to load the IMA policy as well.  So this patch set
> isn't defining a new function for reading a file from the kernel.  It's
> using an existing one.

I see thanks, 

> FYI, sound/sound_firmware.c: do_mod_firmware_load() also reads a file.

Thanks, this should be generalized as well the only reason for a different
implementation I see here is the size constraint to 128k max. I think we can
move that crap check out to take advantage of a common read.

The integrity_read_file() seems rather generic as well and just skips
locking checks and security checks, a generic solution doesn't have to happen
now because as you note this has been in the kernel for a while.

Eventually, once we generalize a common read perhaps we should stuff this
into VFS common code and provide arguments to enable callers to provide 
restrictions or requirements. Let's work together on that after the holidays.

Let's consolidate notes here:

http://kernelnewbies.org/KernelProjects/common-kernel-loader

  Luis
--
To unsubscribe from this list: send the line "unsubscribe 
linux-security-module" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Reply via email to