Re: [PATCH v10 2/9] fs: introduce kernel_pread_file* support

From: Scott Branden
Date: Wed Jul 08 2020 - 00:42:00 EST

Next message: Florian Fainelli: "[PATCH net-next] net: dsa: loop: Print when registration is successful"
Previous message: Luis Chamberlain: "Re: [PATCH v5 08/12] init: main: add KUnit to kernel init"
In reply to: Scott Branden: "Re: [PATCH v10 2/9] fs: introduce kernel_pread_file* support"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]

Hi Kees,

one more comment below.

On 2020-07-07 9:01 p.m., Scott Branden wrote:

On 2020-07-07 4:56 p.m., Kees Cook wrote:

On Mon, Jul 06, 2020 at 04:23:02PM -0700, Scott Branden wrote:

Add kernel_pread_file* support to kernel to allow for partial read
of files with an offset into the file.

Signed-off-by: Scott Branden <scott.branden@xxxxxxxxxxxx>
---
Â fs/exec.cÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ | 93 ++++++++++++++++++++++++--------
Â include/linux/kernel_read_file.h | 17 ++++++
Â 2 files changed, 87 insertions(+), 23 deletions(-)

diff --git a/fs/exec.c b/fs/exec.c
index 4ea87db5e4d5..e6a8a65f7478 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -928,10 +928,14 @@ 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;
+int kernel_pread_file(struct file *file, void **buf, loff_t *size,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂ loff_t max_size, loff_t pos,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂ enum kernel_read_file_id id)
+{
+ÂÂÂ loff_t alloc_size;
+ÂÂÂ loff_t buf_pos;
+ÂÂÂ loff_t read_end;
+ÂÂÂ loff_t i_size;
ÂÂÂÂÂ ssize_t bytes = 0;
ÂÂÂÂÂ int ret;
Â @@ -951,21 +955,32 @@ 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)) {
+
+ÂÂÂ /* Default read to end of file */
+ÂÂÂ read_end = i_size;
+
+ÂÂÂ /* Allow reading partial portion of file */
+ÂÂÂ if ((id == READING_FIRMWARE_PARTIAL_READ) &&
+ÂÂÂÂÂÂÂ (i_size > (pos + max_size)))
+ÂÂÂÂÂÂÂ read_end = pos + max_size;

There's no need to involve "id" here. There are other signals about
what's happening (i.e. pos != 0, max_size != i_size, etc).

There are other signals other than the fact that kernel_read_file requires
the entire file to be read while kernel_pread_file allows partial files to be read.
So if you do a pread at pos = 0 you need another key to indicate it is "ok" if max_size < i_size.
If id == READING_FIRMWARE_PARTIAL_READ is removed (and we want to share 99% of the code
between kernel_read_file and kernel_pread_file then I need to add another parameter to a common function
called between these functions.Â And adding another parameter was rejected previously in the review as a "swiss army knife approach" by another reviewer.Â I am happy to add it back in because it is necessary to share code and differentiate whether we are performing a partial read or not.

+
+ÂÂÂ alloc_size = read_end - pos;
+ÂÂÂ if (i_size > SIZE_MAX || (max_size > 0 && alloc_size > max_size)) {
ÂÂÂÂÂÂÂÂÂ ret = -EFBIG;
ÂÂÂÂÂÂÂÂÂ goto out;
ÂÂÂÂÂ }
Â -ÂÂÂ if (id != READING_FIRMWARE_PREALLOC_BUFFER)
-ÂÂÂÂÂÂÂ *buf = vmalloc(i_size);
+ÂÂÂ if ((id != READING_FIRMWARE_PARTIAL_READ) &&
+ÂÂÂÂÂÂÂ (id != READING_FIRMWARE_PREALLOC_BUFFER))
+ÂÂÂÂÂÂÂ *buf = vmalloc(alloc_size);
ÂÂÂÂÂ if (!*buf) {
ÂÂÂÂÂÂÂÂÂ ret = -ENOMEM;
ÂÂÂÂÂÂÂÂÂ goto out;
ÂÂÂÂÂ }

The id usage here was a mistake in upstream, and the series I sent is
trying to clean that up.

I see that cleanup and it works fine with the pread.Â Other than I need some sort of key to share code and indicate whether it is "ok" to do a partial read of the file or not.

Greg, it seems this series is going to end up in your tree due to it
being drivers/misc? I guess I need to direct my series to Greg then, but
get LSM folks Acks.

Â -ÂÂÂ pos = 0;
-ÂÂÂ while (pos < i_size) {
-ÂÂÂÂÂÂÂ bytes = kernel_read(file, *buf + pos, i_size - pos, &pos);
+ÂÂÂ buf_pos = 0;
+ÂÂÂ while (pos < read_end) {
+ÂÂÂÂÂÂÂ bytes = kernel_read(file, *buf + buf_pos, read_end - pos, &pos);
ÂÂÂÂÂÂÂÂÂ if (bytes < 0) {
ÂÂÂÂÂÂÂÂÂÂÂÂÂ ret = bytes;
ÂÂÂÂÂÂÂÂÂÂÂÂÂ goto out_free;
@@ -973,20 +988,23 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size,
Â ÂÂÂÂÂÂÂÂÂ if (bytes == 0)
ÂÂÂÂÂÂÂÂÂÂÂÂÂ break;
+
+ÂÂÂÂÂÂÂ buf_pos += bytes;
ÂÂÂÂÂ }
Â -ÂÂÂ if (pos != i_size) {
+ÂÂÂ if (pos != read_end) {
ÂÂÂÂÂÂÂÂÂ ret = -EIO;
ÂÂÂÂÂÂÂÂÂ goto out_free;
ÂÂÂÂÂ }
Â -ÂÂÂ ret = security_kernel_post_read_file(file, *buf, i_size, id);
+ÂÂÂ ret = security_kernel_post_read_file(file, *buf, alloc_size, id);
ÂÂÂÂÂ if (!ret)
ÂÂÂÂÂÂÂÂÂ *size = pos;

This call cannot be inside kernel_pread_file(): any future LSMs will see
a moving window of contents, etc. It'll need to be in kernel_read_file()
proper.

If IMA still passes (after testing my next patch series with your changes and my modifications)
I will need some more help here.

Â Â out_free:
ÂÂÂÂÂ if (ret < 0) {
-ÂÂÂÂÂÂÂ if (id != READING_FIRMWARE_PREALLOC_BUFFER) {
+ÂÂÂÂÂÂÂ if ((id != READING_FIRMWARE_PARTIAL_READ) &&
+ÂÂÂÂÂÂÂÂÂÂÂ (id != READING_FIRMWARE_PREALLOC_BUFFER)) {
ÂÂÂÂÂÂÂÂÂÂÂÂÂ vfree(*buf);
ÂÂÂÂÂÂÂÂÂÂÂÂÂ *buf = NULL;
ÂÂÂÂÂÂÂÂÂ }
@@ -996,10 +1014,18 @@ int kernel_read_file(struct file *file, void **buf, loff_t *size,
ÂÂÂÂÂ allow_write_access(file);
ÂÂÂÂÂ return ret;
Â }
+
+int kernel_read_file(struct file *file, void **buf, loff_t *size,
+ÂÂÂÂÂÂÂÂÂÂÂÂ loff_t max_size, enum kernel_read_file_id id)
+{
+ÂÂÂ return kernel_pread_file(file, buf, size, max_size, 0, id);
+}
Â 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)
+int kernel_pread_file_from_path(const char *path, void **buf,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ loff_t *size,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ loff_t max_size, loff_t pos,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ enum kernel_read_file_id id)
Â {
ÂÂÂÂÂ struct file *file;
ÂÂÂÂÂ int ret;
@@ -1011,15 +1037,22 @@ 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_pread_file(file, buf, size, max_size, pos, id);
ÂÂÂÂÂ fput(file);
ÂÂÂÂÂ return ret;
Â }
+
+int kernel_read_file_from_path(const char *path, void **buf, loff_t *size,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ loff_t max_size, enum kernel_read_file_id id)
+{
+ÂÂÂ return kernel_pread_file_from_path(path, buf, size, max_size, 0, id);
+}
Â 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,
-ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ enum kernel_read_file_id id)
+int kernel_pread_file_from_path_initns(const char *path, void **buf,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ loff_t *size,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ loff_t max_size, loff_t pos,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ enum kernel_read_file_id id)
Â {
ÂÂÂÂÂ struct file *file;
ÂÂÂÂÂ struct path root;
@@ -1037,14 +1070,22 @@ 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_pread_file(file, buf, size, max_size, pos, id);
ÂÂÂÂÂ fput(file);
ÂÂÂÂÂ return ret;
Â }
+
+int kernel_read_file_from_path_initns(const char *path, void **buf,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ loff_t *size, loff_t max_size,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ enum kernel_read_file_id id)
+{
+ÂÂÂ return kernel_pread_file_from_path_initns(path, buf, size, max_size, 0, id);
+}
Â 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,
-ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ enum kernel_read_file_id id)
+int kernel_pread_file_from_fd(int fd, void **buf, loff_t *size,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ loff_t max_size, loff_t pos,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ enum kernel_read_file_id id)
Â {
ÂÂÂÂÂ struct fd f = fdget(fd);
ÂÂÂÂÂ int ret = -EBADF;
@@ -1052,11 +1093,17 @@ 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_pread_file(f.file, buf, size, max_size, pos, id);
Â out:
ÂÂÂÂÂ fdput(f);
ÂÂÂÂÂ return ret;
Â }
+
+int kernel_read_file_from_fd(int fd, void **buf, loff_t *size, loff_t max_size,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ enum kernel_read_file_id id)
+{
+ÂÂÂ return kernel_pread_file_from_fd(fd, buf, size, max_size, 0, id);
+}
Â EXPORT_SYMBOL_GPL(kernel_read_file_from_fd);

For each of these execution path, the mapping to LSM hooks is:

- all path must call security_kernel_read_file(file, id) before reading
ÂÂ (this appears to be fine as-is in your series).

- anything doing a "full" read needs to call
ÂÂ security_kernel_post_read_file() with the file and full buffer, size,
ÂÂ etc (so all the kernel_read_file*() paths). I imagine instead of
ÂÂ adding 3 copy/pasted versions of this, it may be possible to refactor
ÂÂ the helpers into a single core "full" caller that takes struct file,
ÂÂ or doing some logic in kernel_pread_file() that notices it has the
ÂÂ entire file in the buffer and doing the call then.
ÂÂ As an example of what I mean about doing the call, here's how I might
ÂÂ imagine it for one of the paths if it took struct file:

int kernel_read_file_from_file(struct file *file, void **buf, loff_t *size,
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ loff_t max_size, enum kernel_read_file_id id)
{
ÂÂÂÂint ret;

ÂÂÂÂret = kernel_pread_file_from_file(file, buf, size, max_size, 0, id);
ÂÂÂÂif (ret)
ÂÂÂÂÂÂÂ return ret;
ÂÂÂÂreturn security_kernel_post_read_file(file, buf, *size, id);
}

Â Â #if defined(CONFIG_HAVE_AOUT) || defined(CONFIG_BINFMT_FLAT) || \
diff --git a/include/linux/kernel_read_file.h b/include/linux/kernel_read_file.h
index 53f5ca41519a..f061ccb8d0b4 100644
--- a/include/linux/kernel_read_file.h
+++ b/include/linux/kernel_read_file.h
@@ -8,6 +8,7 @@
Â #define __kernel_read_file_id(id) \
ÂÂÂÂÂ id(UNKNOWN, unknown)ÂÂÂÂÂÂÂ \
ÂÂÂÂÂ id(FIRMWARE, firmware)ÂÂÂÂÂÂÂ \
+ÂÂÂ id(FIRMWARE_PARTIAL_READ, firmware)ÂÂÂ \
ÂÂÂÂÂ id(FIRMWARE_PREALLOC_BUFFER, firmware)ÂÂÂ \
ÂÂÂÂÂ id(FIRMWARE_EFI_EMBEDDED, firmware)ÂÂÂ \

And again, sorry that this was in here as a misleading example.

ÂÂÂÂÂ id(MODULE, kernel-module)ÂÂÂÂÂÂÂ \
@@ -36,15 +37,31 @@ static inline const char *kernel_read_file_id_str(enum kernel_read_file_id id)
ÂÂÂÂÂ return kernel_read_file_str[id];
Â }
Â +int kernel_pread_file(struct file *file,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂ void **buf, loff_t *size, loff_t pos,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂ loff_t max_size,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂ enum kernel_read_file_id id);
Â int kernel_read_file(struct file *file,
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ void **buf, loff_t *size, loff_t max_size,
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂ enum kernel_read_file_id id);
+int kernel_pread_file_from_path(const char *path,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ void **buf, loff_t *size, loff_t pos,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ 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,
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ enum kernel_read_file_id id);
+int kernel_pread_file_from_path_initns(const char *path,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ void **buf, loff_t *size, loff_t pos,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ 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,
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ enum kernel_read_file_id id);
+int kernel_pread_file_from_fd(int fd,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ void **buf, loff_t *size, loff_t pos,
+ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ 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,
ÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂÂ enum kernel_read_file_id id);

I remain concerned that adding these helpers will lead a poor
interaction with LSMs, but I guess I get to hold my tongue. :)

I only need kernel_pread_file and kernel_pread_file_from_path_initns.Â kernel_pread_file_from_fd and kernel_pread_file_from_path were only added for completeness.
And are really only helper functions called by their kernel_read_file* counterparts at this time.Â So they can be removed from this patch if that helps?

We could add pread functions that are "unsafe" in nature instead then?
As I certainly do not need any integrity checks on the file for my driver.Â The real check is done by the card the data is loaded to whether is passes the linux security checks or not.
And then, if someone does want to do something "safe" with preads another kernel_read_file_securelock/unlock could be added for those that need security for their partial reads?

Next message: Florian Fainelli: "[PATCH net-next] net: dsa: loop: Print when registration is successful"
Previous message: Luis Chamberlain: "Re: [PATCH v5 08/12] init: main: add KUnit to kernel init"
In reply to: Scott Branden: "Re: [PATCH v10 2/9] fs: introduce kernel_pread_file* support"
Messages sorted by: [ date ] [ thread ] [ subject ] [ author ]