[PATCH v2 15/18] fs/kernel_file_read: Add "offset" arg for partial reads
Scott Branden
scott.branden at broadcom.com
Wed Jul 22 18:29:26 EDT 2020
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.
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 at 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;
More information about the kexec
mailing list