Re: [PATCH v2 15/18] fs/kernel_file_read: Add "offset" arg for partial reads

[Scott Branden <scott.branden@xxxxxxxxxxxx>]

On 2020-07-22 3:29 p.m., Scott Branden wrote:
> Hi Kees,
>
> These changes don't pass the kernel-selftest for partial reads I added
> (which are at the end of this patch v2 series).
> See change below added for temp workaround for issue.
> Even with such change real request_partial_firmware_into_buf doesn't
> work fully with my bcm-vk driver.  I'm trying to debug that.
I made an adjustment in the logic of the driver with use of 
request_partial_firmware_into_buf
and now everything is working.  So only issue I find with this entire 
patch series is the problem
of security failing without the workaround below.
> On 2020-07-22 12:30 p.m., Kees Cook wrote:
> > To perform partial reads, callers of kernel_read_file*() must have a
> > non-NULL file_size argument and a preallocated buffer. The new "offset"
> > argument can then be used to seek to specific locations in the file to
> > fill the buffer to, at most, "buf_size" per call.
> >
> > Where possible, the LSM hooks can report whether a full file has been
> > read or not so that the contents can be reasoned about.
> >
> > Signed-off-by: Kees Cook <keescook@xxxxxxxxxxxx>
> > ---
> >   drivers/base/firmware_loader/main.c |  2 +-
> >   fs/kernel_read_file.c               | 78 ++++++++++++++++++++---------
> >   include/linux/kernel_read_file.h    |  8 +--
> >   kernel/kexec_file.c                 |  4 +-
> >   kernel/module.c                     |  2 +-
> >   security/integrity/digsig.c         |  2 +-
> >   security/integrity/ima/ima_fs.c     |  3 +-
> >   7 files changed, 65 insertions(+), 34 deletions(-)
> >
> > diff --git a/drivers/base/firmware_loader/main.c 
> > b/drivers/base/firmware_loader/main.c
> > index bd199404935f..d95249b5284e 100644
> > --- a/drivers/base/firmware_loader/main.c
> > +++ b/drivers/base/firmware_loader/main.c
> > @@ -494,7 +494,7 @@ fw_get_filesystem_firmware(struct device *device, 
> > struct fw_priv *fw_priv,
> >           fw_priv->size = 0;
> >             /* load firmware files from the mount namespace of init */
> > -        rc = kernel_read_file_from_path_initns(path, &buffer, msize,
> > +        rc = kernel_read_file_from_path_initns(path, 0, &buffer, msize,
> >                                  NULL,
> >                                  READING_FIRMWARE);
> >           if (rc < 0) {
> > diff --git a/fs/kernel_read_file.c b/fs/kernel_read_file.c
> > index d73bc3fa710a..90d255fbdd9b 100644
> > --- a/fs/kernel_read_file.c
> > +++ b/fs/kernel_read_file.c
> > @@ -9,6 +9,7 @@
> >    * kernel_read_file() - read file contents into a kernel buffer
> >    *
> >    * @file    file to read from
> > + * @offset    where to start reading from (see below).
> >    * @buf        pointer to a "void *" buffer for reading into (if
> >    *        *@buf is NULL, a buffer will be allocated, and
> >    *        @buf_size will be ignored)
> > @@ -19,19 +20,31 @@
> >    * @id        the kernel_read_file_id identifying the type of
> >    *        file contents being read (for LSMs to examine)
> >    *
> > + * @offset must be 0 unless both @buf and @file_size are non-NULL
> > + * (i.e. the caller must be expecting to read partial file contents
> > + * via an already-allocated @buf, in at most @buf_size chunks, and
> > + * will be able to determine when the entire file was read by
> > + * checking @file_size). This isn't a recommended way to read a
> > + * file, though, since it is possible that the contents might
> > + * change between calls to kernel_read_file().
> > + *
> >    * Returns number of bytes read (no single read will be bigger
> >    * than INT_MAX), or negative on error.
> >    *
> >    */
> > -int kernel_read_file(struct file *file, void **buf,
> > +int kernel_read_file(struct file *file, loff_t offset, void **buf,
> >                size_t buf_size, size_t *file_size,
> >                enum kernel_read_file_id id)
> >   {
> >       loff_t i_size, pos;
> > -    ssize_t bytes = 0;
> > +    size_t copied;
> >       void *allocated = NULL;
> > +    bool whole_file;
> >       int ret;
> >   +    if (offset != 0 && (!*buf || !file_size))
> > +        return -EINVAL;
> > +
> >       if (!S_ISREG(file_inode(file)->i_mode))
> >           return -EINVAL;
> >   @@ -39,19 +52,27 @@ int kernel_read_file(struct file *file, void 
> > **buf,
> >       if (ret)
> >           return ret;
> >   -    ret = security_kernel_read_file(file, id, true);
> > -    if (ret)
> > -        goto out;
> > -
> >       i_size = i_size_read(file_inode(file));
> >       if (i_size <= 0) {
> >           ret = -EINVAL;
> >           goto out;
> >       }
> > -    if (i_size > INT_MAX || i_size > buf_size) {
> > +    /* The file is too big for sane activities. */
> > +    if (i_size > INT_MAX) {
> > +        ret = -EFBIG;
> > +        goto out;
> > +    }
> > +    /* The entire file cannot be read in one buffer. */
> > +    if (!file_size && offset == 0 && i_size > buf_size) {
> >           ret = -EFBIG;
> >           goto out;
> >       }
> > +
> > +    whole_file = (offset == 0 && i_size <= buf_size);
> A hack to get this passing I added which probably breaks some security?
> if (whole_file) {
> > +    ret = security_kernel_read_file(file, id, whole_file);
> > +    if (ret)
> > +        goto out;
> > +
> }
> >       if (file_size)
> >           *file_size = i_size;
> >   @@ -62,9 +83,14 @@ int kernel_read_file(struct file *file, void **buf,
> >           goto out;
> >       }
> >   -    pos = 0;
> > -    while (pos < i_size) {
> > -        bytes = kernel_read(file, *buf + pos, i_size - pos, &pos);
> > +    pos = offset;
> > +    copied = 0;
> > +    while (copied < buf_size) {
> > +        ssize_t bytes;
> > +        size_t wanted = min_t(size_t, buf_size - copied,
> > +                          i_size - pos);
> > +
> > +        bytes = kernel_read(file, *buf + copied, wanted, &pos);
> >           if (bytes < 0) {
> >               ret = bytes;
> >               goto out_free;
> > @@ -72,14 +98,17 @@ int kernel_read_file(struct file *file, void **buf,
> >             if (bytes == 0)
> >               break;
> > +        copied += bytes;
> >       }
> >   -    if (pos != i_size) {
> > -        ret = -EIO;
> > -        goto out_free;
> > -    }
> > +    if (whole_file) {
> > +        if (pos != i_size) {
> > +            ret = -EIO;
> > +            goto out_free;
> > +        }
> >   -    ret = security_kernel_post_read_file(file, *buf, i_size, id);
> > +        ret = security_kernel_post_read_file(file, *buf, i_size, id);
> > +    }
> >     out_free:
> >       if (ret < 0) {
> > @@ -91,11 +120,11 @@ int kernel_read_file(struct file *file, void **buf,
> >     out:
> >       allow_write_access(file);
> > -    return ret == 0 ? pos : ret;
> > +    return ret == 0 ? copied : ret;
> >   }
> >   EXPORT_SYMBOL_GPL(kernel_read_file);
> >   -int kernel_read_file_from_path(const char *path, void **buf,
> > +int kernel_read_file_from_path(const char *path, loff_t offset, void 
> > **buf,
> >                      size_t buf_size, size_t *file_size,
> >                      enum kernel_read_file_id id)
> >   {
> > @@ -109,14 +138,15 @@ int kernel_read_file_from_path(const char 
> > *path, void **buf,
> >       if (IS_ERR(file))
> >           return PTR_ERR(file);
> >   -    ret = kernel_read_file(file, buf, buf_size, file_size, id);
> > +    ret = kernel_read_file(file, offset, buf, buf_size, file_size, id);
> >       fput(file);
> >       return ret;
> >   }
> >   EXPORT_SYMBOL_GPL(kernel_read_file_from_path);
> >   -int kernel_read_file_from_path_initns(const char *path, void **buf,
> > -                      size_t buf_size, size_t *file_size,
> > +int kernel_read_file_from_path_initns(const char *path, loff_t offset,
> > +                      void **buf, size_t buf_size,
> > +                      size_t *file_size,
> >                         enum kernel_read_file_id id)
> >   {
> >       struct file *file;
> > @@ -135,14 +165,14 @@ int kernel_read_file_from_path_initns(const 
> > char *path, void **buf,
> >       if (IS_ERR(file))
> >           return PTR_ERR(file);
> >   -    ret = kernel_read_file(file, buf, buf_size, file_size, id);
> > +    ret = kernel_read_file(file, offset, buf, buf_size, file_size, id);
> >       fput(file);
> >       return ret;
> >   }
> >   EXPORT_SYMBOL_GPL(kernel_read_file_from_path_initns);
> >   -int kernel_read_file_from_fd(int fd, void **buf, size_t buf_size,
> > -                 size_t *file_size,
> > +int kernel_read_file_from_fd(int fd, loff_t offset, void **buf,
> > +                 size_t buf_size, size_t *file_size,
> >                    enum kernel_read_file_id id)
> >   {
> >       struct fd f = fdget(fd);
> > @@ -151,7 +181,7 @@ int kernel_read_file_from_fd(int fd, void **buf, 
> > size_t buf_size,
> >       if (!f.file)
> >           goto out;
> >   -    ret = kernel_read_file(f.file, buf, buf_size, file_size, id);
> > +    ret = kernel_read_file(f.file, offset, buf, buf_size, file_size, 
> > id);
> >   out:
> >       fdput(f);
> >       return ret;
> > diff --git a/include/linux/kernel_read_file.h 
> > b/include/linux/kernel_read_file.h
> > index 023293eaf948..575ffa1031d3 100644
> > --- a/include/linux/kernel_read_file.h
> > +++ b/include/linux/kernel_read_file.h
> > @@ -35,19 +35,19 @@ static inline const char 
> > *kernel_read_file_id_str(enum kernel_read_file_id id)
> >       return kernel_read_file_str[id];
> >   }
> >   -int kernel_read_file(struct file *file,
> > +int kernel_read_file(struct file *file, loff_t offset,
> >                void **buf, size_t buf_size,
> >                size_t *file_size,
> >                enum kernel_read_file_id id);
> > -int kernel_read_file_from_path(const char *path,
> > +int kernel_read_file_from_path(const char *path, loff_t offset,
> >                      void **buf, size_t buf_size,
> >                      size_t *file_size,
> >                      enum kernel_read_file_id id);
> > -int kernel_read_file_from_path_initns(const char *path,
> > +int kernel_read_file_from_path_initns(const char *path, loff_t offset,
> >                         void **buf, size_t buf_size,
> >                         size_t *file_size,
> >                         enum kernel_read_file_id id);
> > -int kernel_read_file_from_fd(int fd,
> > +int kernel_read_file_from_fd(int fd, loff_t offset,
> >                    void **buf, size_t buf_size,
> >                    size_t *file_size,
> >                    enum kernel_read_file_id id);
> > diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
> > index 878ca684a3a1..45726bc8f6ce 100644
> > --- a/kernel/kexec_file.c
> > +++ b/kernel/kexec_file.c
> > @@ -221,7 +221,7 @@ kimage_file_prepare_segments(struct kimage 
> > *image, int kernel_fd, int initrd_fd,
> >       int ret;
> >       void *ldata;
> >   -    ret = kernel_read_file_from_fd(kernel_fd, &image->kernel_buf,
> > +    ret = kernel_read_file_from_fd(kernel_fd, 0, &image->kernel_buf,
> >                          INT_MAX, NULL, READING_KEXEC_IMAGE);
> >       if (ret < 0)
> >           return ret;
> > @@ -241,7 +241,7 @@ kimage_file_prepare_segments(struct kimage 
> > *image, int kernel_fd, int initrd_fd,
> >   #endif
> >       /* It is possible that there no initramfs is being loaded */
> >       if (!(flags & KEXEC_FILE_NO_INITRAMFS)) {
> > -        ret = kernel_read_file_from_fd(initrd_fd, &image->initrd_buf,
> > +        ret = kernel_read_file_from_fd(initrd_fd, 0, 
> > &image->initrd_buf,
> >                              INT_MAX, NULL,
> >                              READING_KEXEC_INITRAMFS);
> >           if (ret < 0)
> > diff --git a/kernel/module.c b/kernel/module.c
> > index 90a4788dff9d..d353d1f67681 100644
> > --- a/kernel/module.c
> > +++ b/kernel/module.c
> > @@ -4007,7 +4007,7 @@ SYSCALL_DEFINE3(finit_module, int, fd, const 
> > char __user *, uargs, int, flags)
> >                 |MODULE_INIT_IGNORE_VERMAGIC))
> >           return -EINVAL;
> >   -    err = kernel_read_file_from_fd(fd, &hdr, INT_MAX, NULL,
> > +    err = kernel_read_file_from_fd(fd, 0, &hdr, INT_MAX, NULL,
> >                          READING_MODULE);
> >       if (err < 0)
> >           return err;
> > diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
> > index 8a523dfd7fd7..0f518dcfde05 100644
> > --- a/security/integrity/digsig.c
> > +++ b/security/integrity/digsig.c
> > @@ -175,7 +175,7 @@ int __init integrity_load_x509(const unsigned int 
> > id, const char *path)
> >       int rc;
> >       key_perm_t perm;
> >   -    rc = kernel_read_file_from_path(path, &data, INT_MAX, NULL,
> > +    rc = kernel_read_file_from_path(path, 0, &data, INT_MAX, NULL,
> >                       READING_X509_CERTIFICATE);
> >       if (rc < 0) {
> >           pr_err("Unable to open file: %s (%d)", path, rc);
> > diff --git a/security/integrity/ima/ima_fs.c 
> > b/security/integrity/ima/ima_fs.c
> > index 5fc56ccb6678..ea8ff8a07b36 100644
> > --- a/security/integrity/ima/ima_fs.c
> > +++ b/security/integrity/ima/ima_fs.c
> > @@ -284,7 +284,8 @@ static ssize_t ima_read_policy(char *path)
> >       datap = path;
> >       strsep(&datap, "\n");
> >   -    rc = kernel_read_file_from_path(path, &data, INT_MAX, NULL, 
> > READING_POLICY);
> > +    rc = kernel_read_file_from_path(path, 0, &data, INT_MAX, NULL,
> > +                    READING_POLICY);
> >       if (rc < 0) {
> >           pr_err("Unable to open file: %s (%d)", path, rc);
> >           return rc;


_______________________________________________
kexec mailing list
kexec@xxxxxxxxxxxxxxxxxxx
http://lists.infradead.org/mailman/listinfo/kexec