Message-ID: <42169718-d1b8-27f8-eeee-6cdef75a30d9@broadcom.com>
Date:   Tue, 7 Jul 2020 21:01:02 -0700
From:   Scott Branden <scott.branden@...adcom.com>
To:     Kees Cook <keescook@...omium.org>
Cc:     Luis Chamberlain <mcgrof@...nel.org>,
        Wolfram Sang <wsa@...nel.org>,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        David Brown <david.brown@...aro.org>,
        Alexander Viro <viro@...iv.linux.org.uk>,
        Shuah Khan <shuah@...nel.org>, bjorn.andersson@...aro.org,
        Shuah Khan <skhan@...uxfoundation.org>,
        Arnd Bergmann <arnd@...db.de>,
        Mimi Zohar <zohar@...ux.ibm.com>,
        "Rafael J . Wysocki" <rafael@...nel.org>,
        linux-kernel@...r.kernel.org, linux-arm-msm@...r.kernel.org,
        linux-fsdevel@...r.kernel.org,
        BCM Kernel Feedback <bcm-kernel-feedback-list@...adcom.com>,
        Olof Johansson <olof@...om.net>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Dan Carpenter <dan.carpenter@...cle.com>,
        Colin Ian King <colin.king@...onical.com>,
        Takashi Iwai <tiwai@...e.de>, linux-kselftest@...r.kernel.org,
        Andy Gross <agross@...nel.org>,
        linux-integrity@...r.kernel.org,
        linux-security-module@...r.kernel.org
Subject: Re: [PATCH v10 2/9] fs: introduce kernel_pread_file* support



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@...adcom.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?
>

Powered by blists - more mailing lists