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 <[email protected]>
---
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
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 <[email protected]>
Looks good to me. You can add:
Reviewed-by: Jan Kara <[email protected]>
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 <[email protected]>
SUSE Labs, CR
On Thu, 19 Jul 2018 10:58:12 +0200 Jan Kara <[email protected]> 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 <[email protected]>
>
> Looks good to me. You can add:
>
> Reviewed-by: Jan Kara <[email protected]>
Yup.
What are the runtime effects of this bug?
On Fri 20-07-18 16:14:29, Andrew Morton wrote:
> On Thu, 19 Jul 2018 10:58:12 +0200 Jan Kara <[email protected]> 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 <[email protected]>
> >
> > Looks good to me. You can add:
> >
> > Reviewed-by: Jan Kara <[email protected]>
>
> 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 <[email protected]>
SUSE Labs, CR
On Mon, 6 Aug 2018 12:22:03 +0200 Jan Kara <[email protected]> wrote:
> On Fri 20-07-18 16:14:29, Andrew Morton wrote:
> > On Thu, 19 Jul 2018 10:58:12 +0200 Jan Kara <[email protected]> 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 <[email protected]>
> > >
> > > Looks good to me. You can add:
> > >
> > > Reviewed-by: Jan Kara <[email protected]>
> >
> > 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.
On Mon 06-08-18 15:59:27, Andrew Morton wrote:
> On Mon, 6 Aug 2018 12:22:03 +0200 Jan Kara <[email protected]> wrote:
>
> > On Fri 20-07-18 16:14:29, Andrew Morton wrote:
> > > On Thu, 19 Jul 2018 10:58:12 +0200 Jan Kara <[email protected]> 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 <[email protected]>
> > > >
> > > > Looks good to me. You can add:
> > > >
> > > > Reviewed-by: Jan Kara <[email protected]>
> > >
> > > 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 <[email protected]>
SUSE Labs, CR
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 <[email protected]> wrote:
>>
>>> On Fri 20-07-18 16:14:29, Andrew Morton wrote:
>>>> On Thu, 19 Jul 2018 10:58:12 +0200 Jan Kara <[email protected]> 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 <[email protected]>
>>>>> Looks good to me. You can add:
>>>>>
>>>>> Reviewed-by: Jan Kara <[email protected]>
>>>> 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
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 <[email protected]> wrote:
> > >
> > > > On Fri 20-07-18 16:14:29, Andrew Morton wrote:
> > > > > On Thu, 19 Jul 2018 10:58:12 +0200 Jan Kara <[email protected]> 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 <[email protected]>
> > > > > > Looks good to me. You can add:
> > > > > >
> > > > > > Reviewed-by: Jan Kara <[email protected]>
> > > > > 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 <[email protected]>
SUSE Labs, CR