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

[Stephen Boyd]

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

> 
> > +             msize = buf->allocated_size;
> > +     }
> > 
> >       path = __getname();
> >       if (!path)
> 
> > --- a/fs/exec.c
> > +++ b/fs/exec.c
> > @@ -836,8 +836,8 @@ int kernel_read(struct file *file, loff_t offset,
> > 
> >  EXPORT_SYMBOL(kernel_read);
> > 
> > -int kernel_read_file(struct file *file, void **buf, loff_t *size,
> > -                  loff_t max_size, enum kernel_read_file_id id)
> > +static int _kernel_read_file(struct file *file, void **buf, loff_t *size,
> > +                          loff_t max_size, enum kernel_read_file_id id)
> >  {
> >       loff_t i_size, pos;
> >       ssize_t bytes = 0;
> > @@ -856,7 +856,8 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size,
> >       if (i_size <= 0)
> >               return -EINVAL;
> > 
> > -     *buf = vmalloc(i_size);
> > +     if (id != READING_FIRMWARE_INTO_BUF)
> > +             *buf = vmalloc(i_size);
> >       if (!*buf)
> >               return -ENOMEM;
> > 
> > @@ -885,11 +886,19 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size,
> > 
> >  out:
> >       if (ret < 0) {
> > -             vfree(*buf);
> > -             *buf = NULL;
> > +             if (id != READING_FIRMWARE_INTO_BUF) {
> > +                     vfree(*buf);
> > +                     *buf = NULL;
> > +             }
> >       }
> >       return ret;
> >  }
> > +
> > +int kernel_read_file(struct file *file, void **buf, loff_t *size,
> > +                  loff_t max_size, enum kernel_read_file_id id)
> > +{
> > +     return _kernel_read_file(file, buf, size, max_size, id);
> > +}
> >  EXPORT_SYMBOL_GPL(kernel_read_file);
> 
> This seems to be exactly the same as the original version.

Ah right, holdovers from previous patches.

> 
> > 
> >  int kernel_read_file_from_path(char *path, void **buf, loff_t *size,
> > @@ -905,7 +914,7 @@ int kernel_read_file_from_path(char *path, void **buf, loff_t *size,
> >       if (IS_ERR(file))
> >               return PTR_ERR(file);
> > 
> > -     ret = kernel_read_file(file, buf, size, max_size, id);
> > +     ret = _kernel_read_file(file, buf, size, max_size, id);
> >       fput(file);
> >       return ret;
> >  }
> > diff --git a/include/linux/firmware.h b/include/linux/firmware.h
> > index 5c41c5e75b5c..bdc24ee92823 100644
> > --- a/include/linux/firmware.h
> > +++ b/include/linux/firmware.h
> > @@ -47,6 +47,8 @@ int request_firmware_nowait(
> >       void (*cont)(const struct firmware *fw, void *context));
> >  int request_firmware_direct(const struct firmware **fw, const char *name,
> >                           struct device *device);
> > +int request_firmware_into_buf(const struct firmware **firmware_p,
> > +     const char *name, struct device *device, void *buf, size_t size);
> > 
> >  void release_firmware(const struct firmware *fw);
> >  #else
> > @@ -75,5 +77,11 @@ static inline int request_firmware_direct(const struct firmware **fw,
> >       return -EINVAL;
> >  }
> > 
> > +static inline int request_firmware_into_buf(const struct firmware **firmware_p,
> > +     const char *name, struct device *device, void *buf, size_t size);
> > +{
> > +     return -EINVAL;
> > +}
> > +
> >  #endif
> >  #endif
> > diff --git a/include/linux/fs.h b/include/linux/fs.h
> > index 14a97194b34b..4fb8596b3dd4 100644
> > --- a/include/linux/fs.h
> > +++ b/include/linux/fs.h
> > @@ -2582,6 +2582,7 @@ extern int do_pipe_flags(int *, int);
> > 
> >  enum kernel_read_file_id {
> >       READING_FIRMWARE = 1,
> > +     READING_FIRMWARE_INTO_BUF,
> >       READING_MODULE,
> >       READING_KEXEC_IMAGE,
> >       READING_KEXEC_INITRAMFS,
> 
> Commit "1284ab5 fs: define a string representation of the
> kernel_read_file_id enumeration", which is queued to be upstreamed,
> changes this a bit.

Ok. Will update, thanks.

> 
> > diff --git a/security/integrity/ima/ima_fs.c b/security/integrity/ima/ima_fs.c
> > index 60d011aaec38..b922d6ca2fb0 100644
> > --- a/security/integrity/ima/ima_fs.c
> > +++ b/security/integrity/ima/ima_fs.c
> > @@ -272,7 +272,8 @@ static ssize_t ima_read_policy(char *path)
> >       datap = path;
> >       strsep(&datap, "\n");
> > 
> > -     rc = kernel_read_file_from_path(path, &data, &size, 0, READING_POLICY);
> > +     rc = kernel_read_file_from_path(path, &data, &size, 0, READING_POLICY,
> > +                                     NULL);
> 
> It doesn't look like kernel_read_file_from_path() changed.
> 

Good catch. Holdovers from the previous patches. Will fix in next
version.