[PATCH] vfs: forbid write access when reading a file into memory

[PATCH] vfs: forbid write access when reading a file into memory

[Mimi Zohar]

From: Dmitry Kasatkin <d.kasatkin@samsung.com>

This patch is based on top of the "vfs: support for a common kernel file
loader" patch set.  In general when the kernel is reading a file into
memory it does not want anything else writing to it.

The kernel currently only forbids write access to a file being executed.
This patch extends this locking to files being read by the kernel.

Changelog:
- moved function to kernel_read_file() - Mimi
- updated patch description - Mimi

Signed-off-by: Dmitry Kasatkin <dmitry.kasatkin@huawei.com>
Cc: Al Viro <viro@ZenIV.linux.org.uk>
Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
---
 fs/exec.c | 29 +++++++++++++++++++++--------
 1 file changed, 21 insertions(+), 8 deletions(-)

diff --git a/fs/exec.c b/fs/exec.c
index 604f669..1b7d617 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -846,15 +846,25 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size,
 	if (ret)
 		return ret;
 
+	ret = deny_write_access(file);
+	if (ret)
+		return ret;
+
 	i_size = i_size_read(file_inode(file));
-	if (max_size > 0 && i_size > max_size)
-		return -EFBIG;
-	if (i_size <= 0)
-		return -EINVAL;
+	if (max_size > 0 && i_size > max_size) {
+		ret = -EFBIG;
+		goto out;
+	}
+	if (i_size <= 0) {
+		ret = -EINVAL;
+		goto out;
+	}
 
 	*buf = vmalloc(i_size);
-	if (!*buf)
-		return -ENOMEM;
+	if (!*buf) {
+		ret = -ENOMEM;
+		goto out;
+	}
 
 	pos = 0;
 	while (pos < i_size) {
@@ -872,18 +882,21 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size,
 
 	if (pos != i_size) {
 		ret = -EIO;
-		goto out;
+		goto out_free;
 	}
 
 	ret = security_kernel_post_read_file(file, *buf, i_size, id);
 	if (!ret)
 		*size = pos;
 
-out:
+out_free:
 	if (ret < 0) {
 		vfree(*buf);
 		*buf = NULL;
 	}
+
+out:
+	allow_write_access(file);
 	return ret;
 }
 EXPORT_SYMBOL_GPL(kernel_read_file);
-- 
2.1.0

Re: [PATCH] vfs: forbid write access when reading a file into memory

[Luis R. Rodriguez]

On Tue, Feb 16, 2016 at 03:54:05PM -0500, Mimi Zohar wrote:
> From: Dmitry Kasatkin <d.kasatkin@samsung.com>
> 
> This patch is based on top of the "vfs: support for a common kernel file
> loader" patch set.  In general when the kernel is reading a file into
> memory it does not want anything else writing to it.
> 
> The kernel currently only forbids write access to a file being executed.
> This patch extends this locking to files being read by the kernel.
> 
> Changelog:
> - moved function to kernel_read_file() - Mimi
> - updated patch description - Mimi
> 
> Signed-off-by: Dmitry Kasatkin <dmitry.kasatkin@huawei.com>
> Cc: Al Viro <viro@ZenIV.linux.org.uk>
> Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>

BTW I don't see your common read work on linux-next yet, would
you resend this later after that queue of patches get merged?

Reviewed-by: Luis R. Rodriguez <mcgrof@kernel.org>

  Luis

Re: [PATCH] vfs: forbid write access when reading a file into memory

[Mimi Zohar]

This patch is based on top of the "vfs: support for a common kernel file
loader" patch set.  In general when the kernel is reading a file into
memory it does not want anything else writing to it.

The kernel currently only forbids write access to a file being executed.
This patch extends this locking to files being read by the kernel.

Changelog:
- moved function to kernel_read_file() - Mimi
- updated patch description - Mimi

Signed-off-by: Dmitry Kasatkin <dmitry.kasatkin@huawei.com>
Cc: Al Viro <viro@ZenIV.linux.org.uk>
Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
Reviewed-by: Luis R. Rodriguez <mcgrof@kernel.org>
Acked-by: Kees Cook <keescook@chromium.org>
---
 fs/exec.c | 29 +++++++++++++++++++++--------
 1 file changed, 21 insertions(+), 8 deletions(-)

diff --git a/fs/exec.c b/fs/exec.c
index 604f669..1b7d617 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -846,15 +846,25 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size,
 	if (ret)
 		return ret;
 
+	ret = deny_write_access(file);
+	if (ret)
+		return ret;
+
 	i_size = i_size_read(file_inode(file));
-	if (max_size > 0 && i_size > max_size)
-		return -EFBIG;
-	if (i_size <= 0)
-		return -EINVAL;
+	if (max_size > 0 && i_size > max_size) {
+		ret = -EFBIG;
+		goto out;
+	}
+	if (i_size <= 0) {
+		ret = -EINVAL;
+		goto out;
+	}
 
 	*buf = vmalloc(i_size);
-	if (!*buf)
-		return -ENOMEM;
+	if (!*buf) {
+		ret = -ENOMEM;
+		goto out;
+	}
 
 	pos = 0;
 	while (pos < i_size) {
@@ -872,18 +882,21 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size,
 
 	if (pos != i_size) {
 		ret = -EIO;
-		goto out;
+		goto out_free;
 	}
 
 	ret = security_kernel_post_read_file(file, *buf, i_size, id);
 	if (!ret)
 		*size = pos;
 
-out:
+out_free:
 	if (ret < 0) {
 		vfree(*buf);
 		*buf = NULL;
 	}
+
+out:
+	allow_write_access(file);
 	return ret;
 }
 EXPORT_SYMBOL_GPL(kernel_read_file);
-- 
2.1.0

Re: [PATCH] vfs: forbid write access when reading a file into memory

[Kees Cook]

On Tue, Feb 16, 2016 at 12:54 PM, Mimi Zohar <zohar@linux.vnet.ibm.com> wrote:
> From: Dmitry Kasatkin <d.kasatkin@samsung.com>
>
> This patch is based on top of the "vfs: support for a common kernel file
> loader" patch set.  In general when the kernel is reading a file into
> memory it does not want anything else writing to it.

Oh yes please. Thanks for this! That had bothered me for a long time. :)

Acked-by: Kees Cook <keescook@chromium.org>

-Kees

>
> The kernel currently only forbids write access to a file being executed.
> This patch extends this locking to files being read by the kernel.
>
> Changelog:
> - moved function to kernel_read_file() - Mimi
> - updated patch description - Mimi
>
> Signed-off-by: Dmitry Kasatkin <dmitry.kasatkin@huawei.com>
> Cc: Al Viro <viro@ZenIV.linux.org.uk>
> Signed-off-by: Mimi Zohar <zohar@linux.vnet.ibm.com>
> ---
>  fs/exec.c | 29 +++++++++++++++++++++--------
>  1 file changed, 21 insertions(+), 8 deletions(-)
>
> diff --git a/fs/exec.c b/fs/exec.c
> index 604f669..1b7d617 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -846,15 +846,25 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size,
>         if (ret)
>                 return ret;
>
> +       ret = deny_write_access(file);
> +       if (ret)
> +               return ret;
> +
>         i_size = i_size_read(file_inode(file));
> -       if (max_size > 0 && i_size > max_size)
> -               return -EFBIG;
> -       if (i_size <= 0)
> -               return -EINVAL;
> +       if (max_size > 0 && i_size > max_size) {
> +               ret = -EFBIG;
> +               goto out;
> +       }
> +       if (i_size <= 0) {
> +               ret = -EINVAL;
> +               goto out;
> +       }
>
>         *buf = vmalloc(i_size);
> -       if (!*buf)
> -               return -ENOMEM;
> +       if (!*buf) {
> +               ret = -ENOMEM;
> +               goto out;
> +       }
>
>         pos = 0;
>         while (pos < i_size) {
> @@ -872,18 +882,21 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size,
>
>         if (pos != i_size) {
>                 ret = -EIO;
> -               goto out;
> +               goto out_free;
>         }
>
>         ret = security_kernel_post_read_file(file, *buf, i_size, id);
>         if (!ret)
>                 *size = pos;
>
> -out:
> +out_free:
>         if (ret < 0) {
>                 vfree(*buf);
>                 *buf = NULL;
>         }
> +
> +out:
> +       allow_write_access(file);
>         return ret;
>  }
>  EXPORT_SYMBOL_GPL(kernel_read_file);
> --
> 2.1.0
>



-- 
Kees Cook
Chrome OS & Brillo Security