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

Scott Branden
2020-07-23T06:24:12Z

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@chromium.org>
>> ---
>>   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;
>