From: Scott Branden <scott.branden@broadcom.com>
To: Kees Cook <keescook@chromium.org>
Cc: Luis Chamberlain <mcgrof@kernel.org>,
Wolfram Sang <wsa@kernel.org>,
Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
David Brown <david.brown@linaro.org>,
Alexander Viro <viro@zeniv.linux.org.uk>,
Shuah Khan <shuah@kernel.org>,
bjorn.andersson@linaro.org,
Shuah Khan <skhan@linuxfoundation.org>,
Arnd Bergmann <arnd@arndb.de>, Mimi Zohar <zohar@linux.ibm.com>,
"Rafael J . Wysocki" <rafael@kernel.org>,
linux-kernel@vger.kernel.org, linux-arm-msm@vger.kernel.org,
linux-fsdevel@vger.kernel.org,
BCM Kernel Feedback <bcm-kernel-feedback-list@broadcom.com>,
Olof Johansson <olof@lixom.net>,
Andrew Morton <akpm@linux-foundation.org>,
Dan Carpenter <dan.carpenter@oracle.com>,
Colin Ian King <colin.king@canonical.com>,
Takashi Iwai <tiwai@suse.de>,
linux-kselftest@vger.kernel.org, Andy Gross <agross@kernel.org>,
linux-integrity@vger.kernel.org,
linux-security-module@vger.kernel.org
Subject: Re: [PATCH v10 2/9] fs: introduce kernel_pread_file* support
Date: Tue, 7 Jul 2020 21:01:02 -0700 [thread overview]
Message-ID: <42169718-d1b8-27f8-eeee-6cdef75a30d9@broadcom.com> (raw)
In-Reply-To: <202007071642.AA705B2A@keescook>
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@broadcom.com>
>> ---
>> 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. :)
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 prev parent reply other threads:[~2020-07-08 4:01 UTC|newest]
Thread overview: 30+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-07-06 23:23 [PATCH v10 0/9] firmware: add request_partial_firmware_into_buf Scott Branden
2020-07-06 23:23 ` [PATCH v10 1/9] fs: move kernel_read_file* to its own include file Scott Branden
2020-07-07 23:40 ` Kees Cook
2020-07-08 3:39 ` Scott Branden
2020-07-06 23:23 ` [PATCH v10 2/9] fs: introduce kernel_pread_file* support Scott Branden
2020-07-07 23:56 ` Kees Cook
2020-07-08 0:24 ` Mimi Zohar
2020-07-08 4:01 ` Scott Branden [this message]
2020-07-08 4:41 ` Scott Branden
2020-07-06 23:23 ` [PATCH v10 3/9] firmware: add request_partial_firmware_into_buf Scott Branden
2020-07-07 5:29 ` kernel test robot
2020-07-07 23:58 ` Kees Cook
2020-07-08 4:07 ` Scott Branden
2020-07-06 23:23 ` [PATCH v10 4/9] test_firmware: add partial read support for request_firmware_into_buf Scott Branden
2020-07-07 23:59 ` Kees Cook
2020-07-08 4:09 ` Scott Branden
2020-07-06 23:23 ` [PATCH v10 5/9] firmware: test partial file reads of request_partial_firmware_into_buf Scott Branden
2020-07-06 23:23 ` [PATCH v10 6/9] bcm-vk: add bcm_vk UAPI Scott Branden
2020-07-06 23:23 ` [PATCH v10 7/9] misc: bcm-vk: add Broadcom VK driver Scott Branden
2020-07-08 0:03 ` Kees Cook
2020-07-08 4:30 ` Scott Branden
2020-07-18 8:44 ` kernel test robot
2020-07-06 23:23 ` [PATCH v10 8/9] MAINTAINERS: bcm-vk: add maintainer for Broadcom VK Driver Scott Branden
2020-07-06 23:23 ` [PATCH v10 9/9] ima: add FIRMWARE_PARTIAL_READ support Scott Branden
2020-07-07 3:08 ` Kees Cook
2020-07-07 17:13 ` Scott Branden
2020-07-07 23:36 ` Kees Cook
2020-07-08 3:35 ` Scott Branden
2020-07-08 4:38 ` [PATCH v10 0/9] firmware: add request_partial_firmware_into_buf Florian Fainelli
2020-07-08 4:51 ` Scott Branden
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=42169718-d1b8-27f8-eeee-6cdef75a30d9@broadcom.com \
--to=scott.branden@broadcom.com \
--cc=agross@kernel.org \
--cc=akpm@linux-foundation.org \
--cc=arnd@arndb.de \
--cc=bcm-kernel-feedback-list@broadcom.com \
--cc=bjorn.andersson@linaro.org \
--cc=colin.king@canonical.com \
--cc=dan.carpenter@oracle.com \
--cc=david.brown@linaro.org \
--cc=gregkh@linuxfoundation.org \
--cc=keescook@chromium.org \
--cc=linux-arm-msm@vger.kernel.org \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-integrity@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-kselftest@vger.kernel.org \
--cc=linux-security-module@vger.kernel.org \
--cc=mcgrof@kernel.org \
--cc=olof@lixom.net \
--cc=rafael@kernel.org \
--cc=shuah@kernel.org \
--cc=skhan@linuxfoundation.org \
--cc=tiwai@suse.de \
--cc=viro@zeniv.linux.org.uk \
--cc=wsa@kernel.org \
--cc=zohar@linux.ibm.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.