Re: [RFC/PATCHv3 v3 3/3] firmware: Support loading into a pre-allocated buffer

[Mimi Zohar]

On Wed, 2016-05-11 at 13:22 -0700, Stephen Boyd wrote:
> Quoting Mimi Zohar (2016-05-11 05:41:52)
> > Hi Stephen,
> > 
> > On Tue, 2016-05-10 at 15:26 -0700, Stephen Boyd wrote:
> > 
> > > -static int fw_get_filesystem_firmware(struct device *device,
> > > -                                    struct firmware_buf *buf)
> > > +static int
> > > +fw_get_filesystem_firmware(struct device *device, struct firmware_buf *buf)
> > >  {
> > >       loff_t size;
> > >       int i, len;
> > >       int rc = -ENOENT;
> > >       char *path;
> > > +     enum kernel_read_file_id id = READING_FIRMWARE;
> > > +     size_t msize = INT_MAX;
> > > +
> > > +     /* Already populated data member means we're loading into a buffer */
> > > +     if (buf->data) {
> > > +             id = READING_FIRMWARE_INTO_BUF;
> > 
> > In both cases we're reading the firmware into a buffer.  In this case,
> > it is pre-allocated.  Other than it being pre-allocated, is there
> > anything special about this buffer? 
> 
> No. I'm not sure what you're asking/implying.
> 
> > There has to be a more appropriate
> > string identifier.
> 
> Ok. Any suggestions? The point of the new id is so that we know if we
> need to allocate the buffer or not in kernel_read_file().

If you're still using DMA, then perhaps "READING_FIRMWARE_DMA".   If the
only difference is that the buffer is pre-allocated, then maybe
"READING_FIRMWARE_PREALLOC_BUFFER". 

>  Alternatively
> we could indicate that by a NULL *buf pointer, but it seems that half
> the time that's not initialized to NULL so it didn't seem safe to rely
> on that fact or update the callsites appropriately.

Assuming that the pre-allocated buffer is smaller than the firmware
size, then a new name definitely needs to be specified, so that the
firmware signature can be verified on the security_kernel_read_file()
hook, as opposed to the security_kernel_post_read_file() hook.  The
patch would look something like:

diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index 68b26c3..c799459 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -359,6 +359,10 @@ int ima_read_file(struct file *file, enum kernel_read_file_id read_id)
 #endif
                return 0;       /* We rely on module signature checking */
        }
+
+       if (file && read_id == READING_FIRMWARE_INTO_BUF)
+               return process_measurement(file, NULL, 0, MAY_READ,
+                                          FIRMWARE_CHECK, 0);
        return 0;
 }

@@ -404,6 +408,9 @@ int ima_post_read_file(struct file *file, void *buf, loff_t size,
                return 0;
        }

+       if (file && read_id == READING_FIRMWARE_INTO_BUF)
+               return 0;
+
        func = read_idmap[read_id] ?: FILE_CHECK;
        return process_measurement(file, buf, size, MAY_READ, func, 0);
 }

Once we've decided on a more appropriate identifier string, I'll post
the patch.

Mimi