Re: [PATCH 06/13] fs/kernel_read_file: Remove redundant size argument
From: Scott Branden
Date: Fri Jul 17 2020 - 15:55:53 EST
On 2020-07-17 12:04 p.m., Scott Branden wrote:
Hi Kees,
On 2020-07-17 10:43 a.m., Kees Cook wrote:
In preparation for refactoring kernel_read_file*(), remove the redundant
"size" argument which is not needed: it can be included in the return
I don't think the size argument is redundant though.
The existing kernel_read_file functions always read the whole file.
Now, what happens if the file is bigger than the buffer.
How does kernel_read_file know it read the whole file by looking at
the return value?
Actually, this change looks ok dealing with the size. I'll look at the
rest.
code, with callers adjusted. (VFS reads already cannot be larger than
INT_MAX.)
Signed-off-by: Kees Cook <keescook@xxxxxxxxxxxx>
---
 drivers/base/firmware_loader/main.c | 8 ++++----
 fs/kernel_read_file.c | 20 +++++++++-----------
 include/linux/kernel_read_file.h | 8 ++++----
 kernel/kexec_file.c | 13 ++++++-------
 kernel/module.c | 7 +++----
 security/integrity/digsig.c | 5 +++--
 security/integrity/ima/ima_fs.c | 5 +++--
 7 files changed, 32 insertions(+), 34 deletions(-)
diff --git a/drivers/base/firmware_loader/main.c
b/drivers/base/firmware_loader/main.c
index d4a413ea48ce..ea419c7d3d34 100644
--- a/drivers/base/firmware_loader/main.c
+++ b/drivers/base/firmware_loader/main.c
@@ -462,7 +462,7 @@ fw_get_filesystem_firmware(struct device *device,
struct fw_priv *fw_priv,
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ size_t in_size,
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ const void *in_buffer))
 {
-ÂÂÂ loff_t size;
+ÂÂÂ size_t size;
ÂÂÂÂÂ int i, len;
ÂÂÂÂÂ int rc = -ENOENT;
ÂÂÂÂÂ char *path;
@@ -494,10 +494,9 @@ 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,
-ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ &size, msize,
+ÂÂÂÂÂÂÂ rc = kernel_read_file_from_path_initns(path, &buffer, msize,
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ READING_FIRMWARE);
-ÂÂÂÂÂÂÂ if (rc) {
+ÂÂÂÂÂÂÂ if (rc < 0) {
ÂÂÂÂÂÂÂÂÂÂÂÂÂ if (rc != -ENOENT)
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ dev_warn(device, "loading %s failed with error %d\n",
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ path, rc);
@@ -506,6 +505,7 @@ fw_get_filesystem_firmware(struct device *device,
struct fw_priv *fw_priv,
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ path);
ÂÂÂÂÂÂÂÂÂÂÂÂÂ continue;
ÂÂÂÂÂÂÂÂÂ }
+ÂÂÂÂÂÂÂ size = rc;
ÂÂÂÂÂÂÂÂÂ dev_dbg(device, "Loading firmware from %s\n", path);
ÂÂÂÂÂÂÂÂÂ if (decompress) {
ÂÂÂÂÂÂÂÂÂÂÂÂÂ dev_dbg(device, "f/w decompressing %s\n",
diff --git a/fs/kernel_read_file.c b/fs/kernel_read_file.c
index 54d972d4befc..dc28a8def597 100644
--- a/fs/kernel_read_file.c
+++ b/fs/kernel_read_file.c
@@ -5,7 +5,7 @@
 #include <linux/security.h>
 #include <linux/vmalloc.h>
 -int kernel_read_file(struct file *file, void **buf, loff_t *size,
+int kernel_read_file(struct file *file, void **buf,
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ loff_t max_size, enum kernel_read_file_id id)
 {
ÂÂÂÂÂ loff_t i_size, pos;
@@ -29,7 +29,7 @@ int kernel_read_file(struct file *file, void **buf,
loff_t *size,
ÂÂÂÂÂÂÂÂÂ ret = -EINVAL;
ÂÂÂÂÂÂÂÂÂ goto out;
ÂÂÂÂÂ }
-ÂÂÂ if (i_size > SIZE_MAX || (max_size > 0 && i_size > max_size)) {
+ÂÂÂ if (i_size > INT_MAX || (max_size > 0 && i_size > max_size)) {
Should this be SSIZE_MAX?
ÂÂÂÂÂÂÂÂÂ ret = -EFBIG;
ÂÂÂÂÂÂÂÂÂ goto out;
ÂÂÂÂÂ }
@@ -59,8 +59,6 @@ int kernel_read_file(struct file *file, void **buf,
loff_t *size,
ÂÂÂÂÂ }
 Â ret = security_kernel_post_read_file(file, *buf, i_size, id);
-ÂÂÂ if (!ret)
-ÂÂÂÂÂÂÂ *size = pos;
  out_free:
ÂÂÂÂÂ if (ret < 0) {
@@ -72,11 +70,11 @@ int kernel_read_file(struct file *file, void
**buf, loff_t *size,
  out:
ÂÂÂÂÂ allow_write_access(file);
-ÂÂÂ return ret;
+ÂÂÂ return ret == 0 ? pos : ret;
 }
 EXPORT_SYMBOL_GPL(kernel_read_file);
 -int kernel_read_file_from_path(const char *path, void **buf,
loff_t *size,
+int kernel_read_file_from_path(const char *path, void **buf,
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ loff_t max_size, enum kernel_read_file_id id)
 {
ÂÂÂÂÂ struct file *file;
@@ -89,14 +87,14 @@ int kernel_read_file_from_path(const 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, max_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,
-ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ loff_t *size, loff_t max_size,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ loff_t max_size,
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ enum kernel_read_file_id id)
 {
ÂÂÂÂÂ struct file *file;
@@ -115,13 +113,13 @@ 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, size, max_size, id);
+ÂÂÂ ret = kernel_read_file(file, buf, max_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, loff_t *size,
loff_t max_size,
+int kernel_read_file_from_fd(int fd, void **buf, loff_t max_size,
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ enum kernel_read_file_id id)
 {
ÂÂÂÂÂ struct fd f = fdget(fd);
@@ -130,7 +128,7 @@ int kernel_read_file_from_fd(int fd, void **buf,
loff_t *size, loff_t max_size,
ÂÂÂÂÂ if (!f.file)
ÂÂÂÂÂÂÂÂÂ goto out;
 - ret = kernel_read_file(f.file, buf, size, max_size, id);
+ÂÂÂ ret = kernel_read_file(f.file, buf, max_size, id);
 out:
ÂÂÂÂÂ fdput(f);
ÂÂÂÂÂ return ret;
diff --git a/include/linux/kernel_read_file.h
b/include/linux/kernel_read_file.h
index 78cf3d7dc835..0ca0bdbed1bd 100644
--- a/include/linux/kernel_read_file.h
+++ b/include/linux/kernel_read_file.h
@@ -36,16 +36,16 @@ static inline const char
*kernel_read_file_id_str(enum kernel_read_file_id id)
 }
  int kernel_read_file(struct file *file,
-ÂÂÂÂÂÂÂÂÂÂÂÂ void **buf, loff_t *size, loff_t max_size,
+ÂÂÂÂÂÂÂÂÂÂÂÂ void **buf, loff_t max_size,
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ enum kernel_read_file_id id);
 int kernel_read_file_from_path(const char *path,
-ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ void **buf, loff_t *size, loff_t max_size,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ void **buf, loff_t max_size,
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ enum kernel_read_file_id id);
 int kernel_read_file_from_path_initns(const char *path,
-ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ void **buf, loff_t *size, loff_t max_size,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ void **buf, loff_t max_size,
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ enum kernel_read_file_id id);
 int kernel_read_file_from_fd(int fd,
-ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ void **buf, loff_t *size, loff_t max_size,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ void **buf, loff_t max_size,
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ enum kernel_read_file_id id);
  #endif /* _LINUX_KERNEL_READ_FILE_H */
diff --git a/kernel/kexec_file.c b/kernel/kexec_file.c
index 1358069ce9e9..a201bbb19158 100644
--- a/kernel/kexec_file.c
+++ b/kernel/kexec_file.c
@@ -220,13 +220,12 @@ kimage_file_prepare_segments(struct kimage
*image, int kernel_fd, int initrd_fd,
 {
ÂÂÂÂÂ int ret;
ÂÂÂÂÂ void *ldata;
-ÂÂÂ loff_t size;
 Â ret = kernel_read_file_from_fd(kernel_fd, &image->kernel_buf,
-ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ &size, INT_MAX, READING_KEXEC_IMAGE);
-ÂÂÂ if (ret)
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ INT_MAX, READING_KEXEC_IMAGE);
+ÂÂÂ if (ret < 0)
ÂÂÂÂÂÂÂÂÂ return ret;
-ÂÂÂ image->kernel_buf_len = size;
+ÂÂÂ image->kernel_buf_len = ret;
 Â /* Call arch image probe handlers */
ÂÂÂÂÂ ret = arch_kexec_kernel_image_probe(image, image->kernel_buf,
@@ -243,11 +242,11 @@ kimage_file_prepare_segments(struct kimage
*image, int kernel_fd, int initrd_fd,
ÂÂÂÂÂ /* 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,
-ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ &size, INT_MAX,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ INT_MAX,
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ READING_KEXEC_INITRAMFS);
-ÂÂÂÂÂÂÂ if (ret)
+ÂÂÂÂÂÂÂ if (ret < 0)
ÂÂÂÂÂÂÂÂÂÂÂÂÂ goto out;
-ÂÂÂÂÂÂÂ image->initrd_buf_len = size;
+ÂÂÂÂÂÂÂ image->initrd_buf_len = ret;
ÂÂÂÂÂ }
 Â if (cmdline_len) {
diff --git a/kernel/module.c b/kernel/module.c
index e9765803601b..b6fd4f51cc30 100644
--- a/kernel/module.c
+++ b/kernel/module.c
@@ -3988,7 +3988,6 @@ SYSCALL_DEFINE3(init_module, void __user *, umod,
 SYSCALL_DEFINE3(finit_module, int, fd, const char __user *, uargs,
int, flags)
 {
ÂÂÂÂÂ struct load_info info = { };
-ÂÂÂ loff_t size;
ÂÂÂÂÂ void *hdr = NULL;
ÂÂÂÂÂ int err;
 @@ -4002,12 +4001,12 @@ 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, &size, INT_MAX,
+ÂÂÂ err = kernel_read_file_from_fd(fd, &hdr, INT_MAX,
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ READING_MODULE);
-ÂÂÂ if (err)
+ÂÂÂ if (err < 0)
ÂÂÂÂÂÂÂÂÂ return err;
ÂÂÂÂÂ info.hdr = hdr;
-ÂÂÂ info.len = size;
+ÂÂÂ info.len = err;
 Â return load_module(&info, uargs, flags);
 }
diff --git a/security/integrity/digsig.c b/security/integrity/digsig.c
index f8869be45d8f..97661ffabc4e 100644
--- a/security/integrity/digsig.c
+++ b/security/integrity/digsig.c
@@ -171,16 +171,17 @@ int __init integrity_add_key(const unsigned int
id, const void *data,
 int __init integrity_load_x509(const unsigned int id, const char
*path)
 {
ÂÂÂÂÂ void *data = NULL;
-ÂÂÂ loff_t size;
+ÂÂÂ size_t size;
ÂÂÂÂÂ int rc;
ÂÂÂÂÂ key_perm_t perm;
 - rc = kernel_read_file_from_path(path, &data, &size, 0,
+ÂÂÂ rc = kernel_read_file_from_path(path, &data, 0,
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ READING_X509_CERTIFICATE);
ÂÂÂÂÂ if (rc < 0) {
ÂÂÂÂÂÂÂÂÂ pr_err("Unable to open file: %s (%d)", path, rc);
ÂÂÂÂÂÂÂÂÂ return rc;
ÂÂÂÂÂ }
+ÂÂÂ size = rc;
 Â perm = (KEY_POS_ALL & ~KEY_POS_SETATTR) | KEY_USR_VIEW |
KEY_USR_READ;
 diff --git a/security/integrity/ima/ima_fs.c
b/security/integrity/ima/ima_fs.c
index e13ffece3726..9ba145d3d6d9 100644
--- a/security/integrity/ima/ima_fs.c
+++ b/security/integrity/ima/ima_fs.c
@@ -275,7 +275,7 @@ static ssize_t ima_read_policy(char *path)
 {
ÂÂÂÂÂ void *data = NULL;
ÂÂÂÂÂ char *datap;
-ÂÂÂ loff_t size;
+ÂÂÂ size_t size;
ÂÂÂÂÂ int rc, pathlen = strlen(path);
 Â char *p;
@@ -284,11 +284,12 @@ 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, 0, READING_POLICY);
ÂÂÂÂÂ if (rc < 0) {
ÂÂÂÂÂÂÂÂÂ pr_err("Unable to open file: %s (%d)", path, rc);
ÂÂÂÂÂÂÂÂÂ return rc;
ÂÂÂÂÂ }
+ÂÂÂ size = rc;
 Â datap = data;
ÂÂÂÂÂ while (size > 0 && (p = strsep(&datap, "\n"))) {