2019-03-04 15:46:04

by David Howells

[permalink] [raw]
Subject: [PATCH] vfs: Move kernel_read_file() to fs/read_write.c

Move kernel_read_file() to fs/read_write.c and out of fs/exec.c as it's not
actually used by anything in the execve subsystem.

Signed-off-by: David Howells <[email protected]>
cc: Mimi Zohar <[email protected]>
---

fs/exec.c | 106 -------------------------------------------------------
fs/read_write.c | 106 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 106 insertions(+), 106 deletions(-)

diff --git a/fs/exec.c b/fs/exec.c
index fb72d36f7823..cbb1a9cd25ca 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -61,7 +61,6 @@
#include <linux/pipe_fs_i.h>
#include <linux/oom.h>
#include <linux/compat.h>
-#include <linux/vmalloc.h>

#include <linux/uaccess.h>
#include <asm/mmu_context.h>
@@ -892,111 +891,6 @@ struct file *open_exec(const char *name)
}
EXPORT_SYMBOL(open_exec);

-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;
- int ret;
-
- if (!S_ISREG(file_inode(file)->i_mode) || max_size < 0)
- return -EINVAL;
-
- ret = deny_write_access(file);
- if (ret)
- return ret;
-
- ret = security_kernel_read_file(file, id);
- if (ret)
- goto out;
-
- i_size = i_size_read(file_inode(file));
- if (i_size <= 0) {
- ret = -EINVAL;
- goto out;
- }
- if (i_size > SIZE_MAX || (max_size > 0 && i_size > max_size)) {
- ret = -EFBIG;
- goto out;
- }
-
- if (id != READING_FIRMWARE_PREALLOC_BUFFER)
- *buf = vmalloc(i_size);
- if (!*buf) {
- ret = -ENOMEM;
- goto out;
- }
-
- pos = 0;
- while (pos < i_size) {
- bytes = kernel_read(file, *buf + pos, i_size - pos, &pos);
- if (bytes < 0) {
- ret = bytes;
- goto out;
- }
-
- if (bytes == 0)
- break;
- }
-
- if (pos != i_size) {
- ret = -EIO;
- goto out_free;
- }
-
- ret = security_kernel_post_read_file(file, *buf, i_size, id);
- if (!ret)
- *size = pos;
-
-out_free:
- if (ret < 0) {
- if (id != READING_FIRMWARE_PREALLOC_BUFFER) {
- vfree(*buf);
- *buf = NULL;
- }
- }
-
-out:
- allow_write_access(file);
- return ret;
-}
-EXPORT_SYMBOL_GPL(kernel_read_file);
-
-int kernel_read_file_from_path(const char *path, void **buf, loff_t *size,
- loff_t max_size, enum kernel_read_file_id id)
-{
- struct file *file;
- int ret;
-
- if (!path || !*path)
- return -EINVAL;
-
- file = filp_open(path, O_RDONLY, 0);
- if (IS_ERR(file))
- return PTR_ERR(file);
-
- ret = kernel_read_file(file, buf, size, max_size, id);
- fput(file);
- return ret;
-}
-EXPORT_SYMBOL_GPL(kernel_read_file_from_path);
-
-int kernel_read_file_from_fd(int fd, void **buf, loff_t *size, loff_t max_size,
- enum kernel_read_file_id id)
-{
- struct fd f = fdget(fd);
- int ret = -EBADF;
-
- if (!f.file)
- goto out;
-
- ret = kernel_read_file(f.file, buf, size, max_size, id);
-out:
- fdput(f);
- return ret;
-}
-EXPORT_SYMBOL_GPL(kernel_read_file_from_fd);
-
ssize_t read_code(struct file *file, unsigned long addr, loff_t pos, size_t len)
{
ssize_t res = vfs_read(file, (void __user *)addr, len, &pos);
diff --git a/fs/read_write.c b/fs/read_write.c
index ff3c5e6f87cf..555dcaec00ac 100644
--- a/fs/read_write.c
+++ b/fs/read_write.c
@@ -20,6 +20,7 @@
#include <linux/compat.h>
#include <linux/mount.h>
#include <linux/fs.h>
+#include <linux/vmalloc.h>
#include "internal.h"

#include <linux/uaccess.h>
@@ -1362,6 +1363,111 @@ COMPAT_SYSCALL_DEFINE6(pwritev2, compat_ulong_t, fd,

#endif

+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;
+ int ret;
+
+ if (!S_ISREG(file_inode(file)->i_mode) || max_size < 0)
+ return -EINVAL;
+
+ ret = deny_write_access(file);
+ if (ret)
+ return ret;
+
+ ret = security_kernel_read_file(file, id);
+ if (ret)
+ goto out;
+
+ i_size = i_size_read(file_inode(file));
+ if (i_size <= 0) {
+ ret = -EINVAL;
+ goto out;
+ }
+ if (i_size > SIZE_MAX || (max_size > 0 && i_size > max_size)) {
+ ret = -EFBIG;
+ goto out;
+ }
+
+ if (id != READING_FIRMWARE_PREALLOC_BUFFER)
+ *buf = vmalloc(i_size);
+ if (!*buf) {
+ ret = -ENOMEM;
+ goto out;
+ }
+
+ pos = 0;
+ while (pos < i_size) {
+ bytes = kernel_read(file, *buf + pos, i_size - pos, &pos);
+ if (bytes < 0) {
+ ret = bytes;
+ goto out;
+ }
+
+ if (bytes == 0)
+ break;
+ }
+
+ if (pos != i_size) {
+ ret = -EIO;
+ goto out_free;
+ }
+
+ ret = security_kernel_post_read_file(file, *buf, i_size, id);
+ if (!ret)
+ *size = pos;
+
+out_free:
+ if (ret < 0) {
+ if (id != READING_FIRMWARE_PREALLOC_BUFFER) {
+ vfree(*buf);
+ *buf = NULL;
+ }
+ }
+
+out:
+ allow_write_access(file);
+ return ret;
+}
+EXPORT_SYMBOL_GPL(kernel_read_file);
+
+int kernel_read_file_from_path(const char *path, void **buf, loff_t *size,
+ loff_t max_size, enum kernel_read_file_id id)
+{
+ struct file *file;
+ int ret;
+
+ if (!path || !*path)
+ return -EINVAL;
+
+ file = filp_open(path, O_RDONLY, 0);
+ if (IS_ERR(file))
+ return PTR_ERR(file);
+
+ ret = kernel_read_file(file, buf, size, max_size, id);
+ fput(file);
+ return ret;
+}
+EXPORT_SYMBOL_GPL(kernel_read_file_from_path);
+
+int kernel_read_file_from_fd(int fd, void **buf, loff_t *size, loff_t max_size,
+ enum kernel_read_file_id id)
+{
+ struct fd f = fdget(fd);
+ int ret = -EBADF;
+
+ if (!f.file)
+ goto out;
+
+ ret = kernel_read_file(f.file, buf, size, max_size, id);
+out:
+ fdput(f);
+ return ret;
+}
+EXPORT_SYMBOL_GPL(kernel_read_file_from_fd);
+
static ssize_t do_sendfile(int out_fd, int in_fd, loff_t *ppos,
size_t count, loff_t max)
{



2019-03-04 16:24:18

by David Howells

[permalink] [raw]
Subject: Re: [PATCH] vfs: Move kernel_read_file() to fs/read_write.c

Mimi Zohar <[email protected]> wrote:

> > Move kernel_read_file() to fs/read_write.c and out of fs/exec.c as it's not
> > actually used by anything in the execve subsystem.
>
> All files being opened by the kernel should be calling one of these
> helper routines.  Has that changed?

prepare_binprm() uses kernel_read() and has done since at least 2014. The
binfmt drivers also use kernel_read().

Since kernel_read_file() is used by a bunch of things that aren't exec, even
if we switch exec to it, it should probably still go in fs/read_write.c since
it seems generic.

David

2019-03-04 17:00:58

by Mimi Zohar

[permalink] [raw]
Subject: Re: [PATCH] vfs: Move kernel_read_file() to fs/read_write.c

On Mon, 2019-03-04 at 15:11 +0000, David Howells wrote:
> Move kernel_read_file() to fs/read_write.c and out of fs/exec.c as it's not
> actually used by anything in the execve subsystem.

All files being opened by the kernel should be calling one of these
helper routines.  Has that changed?

Mimi

>
> Signed-off-by: David Howells <[email protected]>
> cc: Mimi Zohar <[email protected]>
> ---
>
> fs/exec.c | 106 -------------------------------------------------------
> fs/read_write.c | 106 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> 2 files changed, 106 insertions(+), 106 deletions(-)
>
> diff --git a/fs/exec.c b/fs/exec.c
> index fb72d36f7823..cbb1a9cd25ca 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -61,7 +61,6 @@
> #include <linux/pipe_fs_i.h>
> #include <linux/oom.h>
> #include <linux/compat.h>
> -#include <linux/vmalloc.h>
>
> #include <linux/uaccess.h>
> #include <asm/mmu_context.h>
> @@ -892,111 +891,6 @@ struct file *open_exec(const char *name)
> }
> EXPORT_SYMBOL(open_exec);
>
> -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;
> - int ret;
> -
> - if (!S_ISREG(file_inode(file)->i_mode) || max_size < 0)
> - return -EINVAL;
> -
> - ret = deny_write_access(file);
> - if (ret)
> - return ret;
> -
> - ret = security_kernel_read_file(file, id);
> - if (ret)
> - goto out;
> -
> - i_size = i_size_read(file_inode(file));
> - if (i_size <= 0) {
> - ret = -EINVAL;
> - goto out;
> - }
> - if (i_size > SIZE_MAX || (max_size > 0 && i_size > max_size)) {
> - ret = -EFBIG;
> - goto out;
> - }
> -
> - if (id != READING_FIRMWARE_PREALLOC_BUFFER)
> - *buf = vmalloc(i_size);
> - if (!*buf) {
> - ret = -ENOMEM;
> - goto out;
> - }
> -
> - pos = 0;
> - while (pos < i_size) {
> - bytes = kernel_read(file, *buf + pos, i_size - pos, &pos);
> - if (bytes < 0) {
> - ret = bytes;
> - goto out;
> - }
> -
> - if (bytes == 0)
> - break;
> - }
> -
> - if (pos != i_size) {
> - ret = -EIO;
> - goto out_free;
> - }
> -
> - ret = security_kernel_post_read_file(file, *buf, i_size, id);
> - if (!ret)
> - *size = pos;
> -
> -out_free:
> - if (ret < 0) {
> - if (id != READING_FIRMWARE_PREALLOC_BUFFER) {
> - vfree(*buf);
> - *buf = NULL;
> - }
> - }
> -
> -out:
> - allow_write_access(file);
> - return ret;
> -}
> -EXPORT_SYMBOL_GPL(kernel_read_file);
> -
> -int kernel_read_file_from_path(const char *path, void **buf, loff_t *size,
> - loff_t max_size, enum kernel_read_file_id id)
> -{
> - struct file *file;
> - int ret;
> -
> - if (!path || !*path)
> - return -EINVAL;
> -
> - file = filp_open(path, O_RDONLY, 0);
> - if (IS_ERR(file))
> - return PTR_ERR(file);
> -
> - ret = kernel_read_file(file, buf, size, max_size, id);
> - fput(file);
> - return ret;
> -}
> -EXPORT_SYMBOL_GPL(kernel_read_file_from_path);
> -
> -int kernel_read_file_from_fd(int fd, void **buf, loff_t *size, loff_t max_size,
> - enum kernel_read_file_id id)
> -{
> - struct fd f = fdget(fd);
> - int ret = -EBADF;
> -
> - if (!f.file)
> - goto out;
> -
> - ret = kernel_read_file(f.file, buf, size, max_size, id);
> -out:
> - fdput(f);
> - return ret;
> -}
> -EXPORT_SYMBOL_GPL(kernel_read_file_from_fd);
> -
> ssize_t read_code(struct file *file, unsigned long addr, loff_t pos, size_t len)
> {
> ssize_t res = vfs_read(file, (void __user *)addr, len, &pos);
> diff --git a/fs/read_write.c b/fs/read_write.c
> index ff3c5e6f87cf..555dcaec00ac 100644
> --- a/fs/read_write.c
> +++ b/fs/read_write.c
> @@ -20,6 +20,7 @@
> #include <linux/compat.h>
> #include <linux/mount.h>
> #include <linux/fs.h>
> +#include <linux/vmalloc.h>
> #include "internal.h"
>
> #include <linux/uaccess.h>
> @@ -1362,6 +1363,111 @@ COMPAT_SYSCALL_DEFINE6(pwritev2, compat_ulong_t, fd,
>
> #endif
>
> +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;
> + int ret;
> +
> + if (!S_ISREG(file_inode(file)->i_mode) || max_size < 0)
> + return -EINVAL;
> +
> + ret = deny_write_access(file);
> + if (ret)
> + return ret;
> +
> + ret = security_kernel_read_file(file, id);
> + if (ret)
> + goto out;
> +
> + i_size = i_size_read(file_inode(file));
> + if (i_size <= 0) {
> + ret = -EINVAL;
> + goto out;
> + }
> + if (i_size > SIZE_MAX || (max_size > 0 && i_size > max_size)) {
> + ret = -EFBIG;
> + goto out;
> + }
> +
> + if (id != READING_FIRMWARE_PREALLOC_BUFFER)
> + *buf = vmalloc(i_size);
> + if (!*buf) {
> + ret = -ENOMEM;
> + goto out;
> + }
> +
> + pos = 0;
> + while (pos < i_size) {
> + bytes = kernel_read(file, *buf + pos, i_size - pos, &pos);
> + if (bytes < 0) {
> + ret = bytes;
> + goto out;
> + }
> +
> + if (bytes == 0)
> + break;
> + }
> +
> + if (pos != i_size) {
> + ret = -EIO;
> + goto out_free;
> + }
> +
> + ret = security_kernel_post_read_file(file, *buf, i_size, id);
> + if (!ret)
> + *size = pos;
> +
> +out_free:
> + if (ret < 0) {
> + if (id != READING_FIRMWARE_PREALLOC_BUFFER) {
> + vfree(*buf);
> + *buf = NULL;
> + }
> + }
> +
> +out:
> + allow_write_access(file);
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(kernel_read_file);
> +
> +int kernel_read_file_from_path(const char *path, void **buf, loff_t *size,
> + loff_t max_size, enum kernel_read_file_id id)
> +{
> + struct file *file;
> + int ret;
> +
> + if (!path || !*path)
> + return -EINVAL;
> +
> + file = filp_open(path, O_RDONLY, 0);
> + if (IS_ERR(file))
> + return PTR_ERR(file);
> +
> + ret = kernel_read_file(file, buf, size, max_size, id);
> + fput(file);
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(kernel_read_file_from_path);
> +
> +int kernel_read_file_from_fd(int fd, void **buf, loff_t *size, loff_t max_size,
> + enum kernel_read_file_id id)
> +{
> + struct fd f = fdget(fd);
> + int ret = -EBADF;
> +
> + if (!f.file)
> + goto out;
> +
> + ret = kernel_read_file(f.file, buf, size, max_size, id);
> +out:
> + fdput(f);
> + return ret;
> +}
> +EXPORT_SYMBOL_GPL(kernel_read_file_from_fd);
> +
> static ssize_t do_sendfile(int out_fd, int in_fd, loff_t *ppos,
> size_t count, loff_t max)
> {
>


2019-03-04 17:12:06

by Mimi Zohar

[permalink] [raw]
Subject: Re: [PATCH] vfs: Move kernel_read_file() to fs/read_write.c

On Mon, 2019-03-04 at 16:22 +0000, David Howells wrote:
> Mimi Zohar <[email protected]> wrote:
>
> > > Move kernel_read_file() to fs/read_write.c and out of fs/exec.c as it's not
> > > actually used by anything in the execve subsystem.
> >
> > All files being opened by the kernel should be calling one of these
> > helper routines.  Has that changed?
>
> prepare_binprm() uses kernel_read() and has done since at least 2014. The
> binfmt drivers also use kernel_read().
>
> Since kernel_read_file() is used by a bunch of things that aren't exec, even
> if we switch exec to it, it should probably still go in fs/read_write.c since
> it seems generic.

Oh, commit bdd1d2d3d251 ("fs: fix kernel_read prototype") moved
kernel_read() to fs/read_write.c without moving the helpers.
 Definitely makes sense to move the helpers.  Please include a
reference to the commit in this patch. 

Reviewed-by: Mimi Zohar <[email protected]>