[PATCH] mm: adjust max read count in generic_file_buffered_read()

[PATCH] mm: adjust max read count in generic_file_buffered_read()

[Chengguang Xu]

When we try to truncate read count in generic_file_buffered_read(),
should deliver (sb->s_maxbytes - offset) as maximum count not
sb->s_maxbytes itself.

Signed-off-by: Chengguang Xu <cgxu519@gmx.com>
---
 mm/filemap.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mm/filemap.c b/mm/filemap.c
index 52517f28e6f4..5c2d481d21cf 100644
--- a/mm/filemap.c
+++ b/mm/filemap.c
@@ -2064,7 +2064,7 @@ static ssize_t generic_file_buffered_read(struct kiocb *iocb,
 
 	if (unlikely(*ppos >= inode->i_sb->s_maxbytes))
 		return 0;
-	iov_iter_truncate(iter, inode->i_sb->s_maxbytes);
+	iov_iter_truncate(iter, inode->i_sb->s_maxbytes - *ppos);
 
 	index = *ppos >> PAGE_SHIFT;
 	prev_index = ra->prev_pos >> PAGE_SHIFT;
-- 
2.17.1

Re: [PATCH] mm: adjust max read count in generic_file_buffered_read()

[Jan Kara]

On Thu 19-07-18 16:17:26, Chengguang Xu wrote:
> When we try to truncate read count in generic_file_buffered_read(),
> should deliver (sb->s_maxbytes - offset) as maximum count not
> sb->s_maxbytes itself.
> 
> Signed-off-by: Chengguang Xu <cgxu519@gmx.com>

Looks good to me. You can add:

Reviewed-by: Jan Kara <jack@suse.cz>

BTW, I can see you didn't include two (I'd say the most important ;)
addresses to CC: Al Viro as a VFS maintainer and linux-fsdevel mailing
list. Although this code resides in mm/ it is in fact a filesystem code.
Added now.

								Honza

> ---
>  mm/filemap.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/mm/filemap.c b/mm/filemap.c
> index 52517f28e6f4..5c2d481d21cf 100644
> --- a/mm/filemap.c
> +++ b/mm/filemap.c
> @@ -2064,7 +2064,7 @@ static ssize_t generic_file_buffered_read(struct kiocb *iocb,
>  
>  	if (unlikely(*ppos >= inode->i_sb->s_maxbytes))
>  		return 0;
> -	iov_iter_truncate(iter, inode->i_sb->s_maxbytes);
> +	iov_iter_truncate(iter, inode->i_sb->s_maxbytes - *ppos);
>  
>  	index = *ppos >> PAGE_SHIFT;
>  	prev_index = ra->prev_pos >> PAGE_SHIFT;
> -- 
> 2.17.1
> 
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH] mm: adjust max read count in generic_file_buffered_read()

[Andrew Morton]

On Thu, 19 Jul 2018 10:58:12 +0200 Jan Kara <jack@suse.cz> wrote:

> On Thu 19-07-18 16:17:26, Chengguang Xu wrote:
> > When we try to truncate read count in generic_file_buffered_read(),
> > should deliver (sb->s_maxbytes - offset) as maximum count not
> > sb->s_maxbytes itself.
> > 
> > Signed-off-by: Chengguang Xu <cgxu519@gmx.com>
> 
> Looks good to me. You can add:
> 
> Reviewed-by: Jan Kara <jack@suse.cz>

Yup.

What are the runtime effects of this bug?

Re: [PATCH] mm: adjust max read count in generic_file_buffered_read()

[Jan Kara]

On Fri 20-07-18 16:14:29, Andrew Morton wrote:
> On Thu, 19 Jul 2018 10:58:12 +0200 Jan Kara <jack@suse.cz> wrote:
> 
> > On Thu 19-07-18 16:17:26, Chengguang Xu wrote:
> > > When we try to truncate read count in generic_file_buffered_read(),
> > > should deliver (sb->s_maxbytes - offset) as maximum count not
> > > sb->s_maxbytes itself.
> > > 
> > > Signed-off-by: Chengguang Xu <cgxu519@gmx.com>
> > 
> > Looks good to me. You can add:
> > 
> > Reviewed-by: Jan Kara <jack@suse.cz>
> 
> Yup.
> 
> What are the runtime effects of this bug?

Good question. I think ->readpage() could be called for index beyond
maximum file size supported by the filesystem leading to weird filesystem
behavior due to overflows in internal calculations.

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH] mm: adjust max read count in generic_file_buffered_read()

[Andrew Morton]

On Mon, 6 Aug 2018 12:22:03 +0200 Jan Kara <jack@suse.cz> wrote:

> On Fri 20-07-18 16:14:29, Andrew Morton wrote:
> > On Thu, 19 Jul 2018 10:58:12 +0200 Jan Kara <jack@suse.cz> wrote:
> > 
> > > On Thu 19-07-18 16:17:26, Chengguang Xu wrote:
> > > > When we try to truncate read count in generic_file_buffered_read(),
> > > > should deliver (sb->s_maxbytes - offset) as maximum count not
> > > > sb->s_maxbytes itself.
> > > > 
> > > > Signed-off-by: Chengguang Xu <cgxu519@gmx.com>
> > > 
> > > Looks good to me. You can add:
> > > 
> > > Reviewed-by: Jan Kara <jack@suse.cz>
> > 
> > Yup.
> > 
> > What are the runtime effects of this bug?
> 
> Good question. I think ->readpage() could be called for index beyond
> maximum file size supported by the filesystem leading to weird filesystem
> behavior due to overflows in internal calculations.
> 

Sure.  But is it possible for userspace to trigger this behaviour? 
Possibly all callers have already sanitized the arguments by this stage
in which case the statement is arguably redundant.

I guess I'll put a cc:stable on it and send it in for 4.19-rc1, so we
get a bit more time to poke at it.  But we should have a better
understanding of the userspace impact.

Re: [PATCH] mm: adjust max read count in generic_file_buffered_read()

[Jan Kara]

On Mon 06-08-18 15:59:27, Andrew Morton wrote:
> On Mon, 6 Aug 2018 12:22:03 +0200 Jan Kara <jack@suse.cz> wrote:
> 
> > On Fri 20-07-18 16:14:29, Andrew Morton wrote:
> > > On Thu, 19 Jul 2018 10:58:12 +0200 Jan Kara <jack@suse.cz> wrote:
> > > 
> > > > On Thu 19-07-18 16:17:26, Chengguang Xu wrote:
> > > > > When we try to truncate read count in generic_file_buffered_read(),
> > > > > should deliver (sb->s_maxbytes - offset) as maximum count not
> > > > > sb->s_maxbytes itself.
> > > > > 
> > > > > Signed-off-by: Chengguang Xu <cgxu519@gmx.com>
> > > > 
> > > > Looks good to me. You can add:
> > > > 
> > > > Reviewed-by: Jan Kara <jack@suse.cz>
> > > 
> > > Yup.
> > > 
> > > What are the runtime effects of this bug?
> > 
> > Good question. I think ->readpage() could be called for index beyond
> > maximum file size supported by the filesystem leading to weird filesystem
> > behavior due to overflows in internal calculations.
> > 
> 
> Sure.  But is it possible for userspace to trigger this behaviour? 
> Possibly all callers have already sanitized the arguments by this stage
> in which case the statement is arguably redundant.

So I don't think there's any sanitization going on before
generic_file_buffered_read(). E.g. I don't see any s_maxbytes check on
ksys_read() -> vfs_read() -> __vfs_read() -> new_sync_read() ->
call_read_iter() -> generic_file_read_iter() ->
generic_file_buffered_read() path... However now thinking about this again:
We are guaranteed i_size is within s_maxbytes (places modifying i_size
are checking for this) and generic_file_buffered_read() stops when it
should read beyond i_size. So in the end I don't think there's any breakage
possible and the patch is not necessary?

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR

Re: [PATCH] mm: adjust max read count in generic_file_buffered_read()

[cgxu519]

On 08/07/2018 09:54 PM, Jan Kara wrote:
> On Mon 06-08-18 15:59:27, Andrew Morton wrote:
>> On Mon, 6 Aug 2018 12:22:03 +0200 Jan Kara <jack@suse.cz> wrote:
>>
>>> On Fri 20-07-18 16:14:29, Andrew Morton wrote:
>>>> On Thu, 19 Jul 2018 10:58:12 +0200 Jan Kara <jack@suse.cz> wrote:
>>>>
>>>>> On Thu 19-07-18 16:17:26, Chengguang Xu wrote:
>>>>>> When we try to truncate read count in generic_file_buffered_read(),
>>>>>> should deliver (sb->s_maxbytes - offset) as maximum count not
>>>>>> sb->s_maxbytes itself.
>>>>>>
>>>>>> Signed-off-by: Chengguang Xu <cgxu519@gmx.com>
>>>>> Looks good to me. You can add:
>>>>>
>>>>> Reviewed-by: Jan Kara <jack@suse.cz>
>>>> Yup.
>>>>
>>>> What are the runtime effects of this bug?
>>> Good question. I think ->readpage() could be called for index beyond
>>> maximum file size supported by the filesystem leading to weird filesystem
>>> behavior due to overflows in internal calculations.
>>>
>> Sure.  But is it possible for userspace to trigger this behaviour?
>> Possibly all callers have already sanitized the arguments by this stage
>> in which case the statement is arguably redundant.
> So I don't think there's any sanitization going on before
> generic_file_buffered_read(). E.g. I don't see any s_maxbytes check on
> ksys_read() -> vfs_read() -> __vfs_read() -> new_sync_read() ->
> call_read_iter() -> generic_file_read_iter() ->
> generic_file_buffered_read() path... However now thinking about this again:
> We are guaranteed i_size is within s_maxbytes (places modifying i_size
> are checking for this) and generic_file_buffered_read() stops when it
> should read beyond i_size. So in the end I don't think there's any breakage
> possible and the patch is not necessary?
>
I think most of time i_size is within s_maxbytes in local filesystem,
but consider network filesystem, write big file in 64bit client and
read in 32bit client, in this case maybe generic_file_buffered_read()
can read more than s_maxbytes, right?


Thanks,
Chengguang

Re: [PATCH] mm: adjust max read count in generic_file_buffered_read()

[Jan Kara]

On Wed 08-08-18 08:57:13, cgxu519 wrote:
> On 08/07/2018 09:54 PM, Jan Kara wrote:
> > On Mon 06-08-18 15:59:27, Andrew Morton wrote:
> > > On Mon, 6 Aug 2018 12:22:03 +0200 Jan Kara <jack@suse.cz> wrote:
> > > 
> > > > On Fri 20-07-18 16:14:29, Andrew Morton wrote:
> > > > > On Thu, 19 Jul 2018 10:58:12 +0200 Jan Kara <jack@suse.cz> wrote:
> > > > > 
> > > > > > On Thu 19-07-18 16:17:26, Chengguang Xu wrote:
> > > > > > > When we try to truncate read count in generic_file_buffered_read(),
> > > > > > > should deliver (sb->s_maxbytes - offset) as maximum count not
> > > > > > > sb->s_maxbytes itself.
> > > > > > > 
> > > > > > > Signed-off-by: Chengguang Xu <cgxu519@gmx.com>
> > > > > > Looks good to me. You can add:
> > > > > > 
> > > > > > Reviewed-by: Jan Kara <jack@suse.cz>
> > > > > Yup.
> > > > > 
> > > > > What are the runtime effects of this bug?
> > > > Good question. I think ->readpage() could be called for index beyond
> > > > maximum file size supported by the filesystem leading to weird filesystem
> > > > behavior due to overflows in internal calculations.
> > > > 
> > > Sure.  But is it possible for userspace to trigger this behaviour?
> > > Possibly all callers have already sanitized the arguments by this stage
> > > in which case the statement is arguably redundant.
> > So I don't think there's any sanitization going on before
> > generic_file_buffered_read(). E.g. I don't see any s_maxbytes check on
> > ksys_read() -> vfs_read() -> __vfs_read() -> new_sync_read() ->
> > call_read_iter() -> generic_file_read_iter() ->
> > generic_file_buffered_read() path... However now thinking about this again:
> > We are guaranteed i_size is within s_maxbytes (places modifying i_size
> > are checking for this) and generic_file_buffered_read() stops when it
> > should read beyond i_size. So in the end I don't think there's any breakage
> > possible and the patch is not necessary?
> > 
> I think most of time i_size is within s_maxbytes in local filesystem,
> but consider network filesystem, write big file in 64bit client and
> read in 32bit client, in this case maybe generic_file_buffered_read()
> can read more than s_maxbytes, right?

I'd consider this an internal problem in the implementation of the
networking filesystem. Not something VFS should care about. It's similar to
a normal filesystem loading corrupted file size from disk...

								Honza
-- 
Jan Kara <jack@suse.com>
SUSE Labs, CR