[PATCH] fuse: Don't make buffered read forward overflow value to a userspace process

[PATCH] fuse: Don't make buffered read forward overflow value to a userspace process

[Xiao Yang]

Buffered read in fuse normally goes via:
-> generic_file_buffered_read()
  ------------------------------
  -> fuse_readpages()
    -> fuse_send_readpages()
  or
  -> fuse_readpage() [if fuse_readpages() fails to get page]
    -> fuse_do_readpage()
  ------------------------------
      -> fuse_simple_request()

Buffered read changes original offset to page-aligned length by left-shift
and extends original count to be multiples of PAGE_SIZE and then fuse
forwards these new parameters to a userspace process, so it is possible
for the resulting offset(e.g page-aligned offset + extended count) to
exceed the whole file size(even the max value of off_t) when the userspace
process does read with new parameters.

xfstests generic/525 gets "pread: Invalid argument" error on virtiofs
because it triggers this issue.  See the following explanation:
PAGE_SIZE: 4096, file size: 2^63 - 1
Original: offset: 2^63 - 2, count: 1
Changed by buffered read: offset: 2^63 - 4096, count: 4096
New offset + new count exceeds the file size as well as LLONG_MAX

Make fuse calculate the number of bytes of data pages contain as
nfs_page_length() and generic_file_buffered_read() do, and then forward
page-aligned offset and normal count to a userspace process.

Signed-off-by: Xiao Yang <ice_yangxiao@163.com>
---
 fs/fuse/file.c | 28 ++++++++++++++++++++++++----
 1 file changed, 24 insertions(+), 4 deletions(-)

diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index ce715380143c..5afc4b623eaf 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -19,6 +19,23 @@
 #include <linux/falloc.h>
 #include <linux/uio.h>
 
+static unsigned int fuse_page_length(struct page *page)
+{
+	loff_t i_size = i_size_read(page->mapping->host);
+
+	if (i_size > 0) {
+		pgoff_t index = page_index(page);
+		pgoff_t end_index = (i_size - 1) >> PAGE_SHIFT;
+
+		if (index < end_index)
+			return PAGE_SIZE;
+		if (index == end_index)
+			return ((i_size - 1) & ~PAGE_MASK) + 1;
+	}
+
+	return 0;
+}
+
 static struct page **fuse_pages_alloc(unsigned int npages, gfp_t flags,
 				      struct fuse_page_desc **desc)
 {
@@ -783,7 +800,7 @@ static int fuse_do_readpage(struct file *file, struct page *page)
 	struct inode *inode = page->mapping->host;
 	struct fuse_conn *fc = get_fuse_conn(inode);
 	loff_t pos = page_offset(page);
-	struct fuse_page_desc desc = { .length = PAGE_SIZE };
+	struct fuse_page_desc desc = { .length = fuse_page_length(page) };
 	struct fuse_io_args ia = {
 		.ap.args.page_zeroing = true,
 		.ap.args.out_pages = true,
@@ -881,9 +898,12 @@ static void fuse_send_readpages(struct fuse_io_args *ia, struct file *file)
 	struct fuse_conn *fc = ff->fc;
 	struct fuse_args_pages *ap = &ia->ap;
 	loff_t pos = page_offset(ap->pages[0]);
-	size_t count = ap->num_pages << PAGE_SHIFT;
+	size_t count = 0;
 	ssize_t res;
-	int err;
+	int err, i;
+
+	for (i = 0; i < ap->num_pages; i++)
+		count += ap->descs[i].length;
 
 	ap->args.out_pages = true;
 	ap->args.page_zeroing = true;
@@ -944,7 +964,7 @@ static int fuse_readpages_fill(void *_data, struct page *page)
 
 	get_page(page);
 	ap->pages[ap->num_pages] = page;
-	ap->descs[ap->num_pages].length = PAGE_SIZE;
+	ap->descs[ap->num_pages].length = fuse_page_length(page);
 	ap->num_pages++;
 	data->nr_pages--;
 	return 0;
-- 
2.21.0

Re: [PATCH] fuse: Don't make buffered read forward overflow value to a userspace process

[Miklos Szeredi]

On Mon, Feb 3, 2020 at 8:37 AM Xiao Yang <ice_yangxiao@163.com> wrote:
>
> Buffered read in fuse normally goes via:
> -> generic_file_buffered_read()
>   ------------------------------
>   -> fuse_readpages()
>     -> fuse_send_readpages()
>   or
>   -> fuse_readpage() [if fuse_readpages() fails to get page]
>     -> fuse_do_readpage()
>   ------------------------------
>       -> fuse_simple_request()
>
> Buffered read changes original offset to page-aligned length by left-shift
> and extends original count to be multiples of PAGE_SIZE and then fuse
> forwards these new parameters to a userspace process, so it is possible
> for the resulting offset(e.g page-aligned offset + extended count) to
> exceed the whole file size(even the max value of off_t) when the userspace
> process does read with new parameters.
>
> xfstests generic/525 gets "pread: Invalid argument" error on virtiofs
> because it triggers this issue.  See the following explanation:
> PAGE_SIZE: 4096, file size: 2^63 - 1
> Original: offset: 2^63 - 2, count: 1
> Changed by buffered read: offset: 2^63 - 4096, count: 4096
> New offset + new count exceeds the file size as well as LLONG_MAX

Thanks for the report and analysis.

However this patch may corrupt the cache if i_size changes between
calls to fuse_page_length().  (e.g. first page length set to 33;
second page length to 45; then 33-4095 will be zeroed and 4096-4140
will be filled from offset 33-77).  This will be mitigated by the
pages being invalidated when i_size changes
(fuse_change_attributes()).  Filling the pages with wrong data is not
a good idea regardless.

I think the best approach is first to just fix the xfstest reported
bug by clamping the end offset to LLONG_MAX.  That's a simple patch,
independent of i_size, and hence trivial to verify and hard to mess
up.

Then we can think about  clamping to i_size, whether it is necessary
and if it is, how to best implement it.

Thanks,
Miklos

Re: [PATCH] fuse: Don't make buffered read forward overflow value to a userspace process

[Miklos Szeredi]

On Wed, Feb 5, 2020 at 3:37 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>
> On Mon, Feb 3, 2020 at 8:37 AM Xiao Yang <ice_yangxiao@163.com> wrote:
> >
> > Buffered read in fuse normally goes via:
> > -> generic_file_buffered_read()
> >   ------------------------------
> >   -> fuse_readpages()
> >     -> fuse_send_readpages()
> >   or
> >   -> fuse_readpage() [if fuse_readpages() fails to get page]
> >     -> fuse_do_readpage()
> >   ------------------------------
> >       -> fuse_simple_request()
> >
> > Buffered read changes original offset to page-aligned length by left-shift
> > and extends original count to be multiples of PAGE_SIZE and then fuse
> > forwards these new parameters to a userspace process, so it is possible
> > for the resulting offset(e.g page-aligned offset + extended count) to
> > exceed the whole file size(even the max value of off_t) when the userspace
> > process does read with new parameters.
> >
> > xfstests generic/525 gets "pread: Invalid argument" error on virtiofs
> > because it triggers this issue.  See the following explanation:
> > PAGE_SIZE: 4096, file size: 2^63 - 1
> > Original: offset: 2^63 - 2, count: 1
> > Changed by buffered read: offset: 2^63 - 4096, count: 4096
> > New offset + new count exceeds the file size as well as LLONG_MAX
>
> Thanks for the report and analysis.
>
> However this patch may corrupt the cache if i_size changes between
> calls to fuse_page_length().  (e.g. first page length set to 33;
> second page length to 45; then 33-4095 will be zeroed and 4096-4140
> will be filled from offset 33-77).  This will be mitigated by the
> pages being invalidated when i_size changes
> (fuse_change_attributes()).  Filling the pages with wrong data is not
> a good idea regardless.
>
> I think the best approach is first to just fix the xfstest reported
> bug by clamping the end offset to LLONG_MAX.  That's a simple patch,
> independent of i_size, and hence trivial to verify and hard to mess
> up.

Applied a fix and pushed to:

  git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/fuse.git#for-next

Thanks,
Miklos

Re: [PATCH] fuse: Don't make buffered read forward overflow value to a userspace process

[Xiao Yang]

On 2/6/20 8:14 PM, Miklos Szeredi wrote:
> On Wed, Feb 5, 2020 at 3:37 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>> On Mon, Feb 3, 2020 at 8:37 AM Xiao Yang <ice_yangxiao@163.com> wrote:
>>> Buffered read in fuse normally goes via:
>>> -> generic_file_buffered_read()
>>>    ------------------------------
>>>    -> fuse_readpages()
>>>      -> fuse_send_readpages()
>>>    or
>>>    -> fuse_readpage() [if fuse_readpages() fails to get page]
>>>      -> fuse_do_readpage()
>>>    ------------------------------
>>>        -> fuse_simple_request()
>>>
>>> Buffered read changes original offset to page-aligned length by left-shift
>>> and extends original count to be multiples of PAGE_SIZE and then fuse
>>> forwards these new parameters to a userspace process, so it is possible
>>> for the resulting offset(e.g page-aligned offset + extended count) to
>>> exceed the whole file size(even the max value of off_t) when the userspace
>>> process does read with new parameters.
>>>
>>> xfstests generic/525 gets "pread: Invalid argument" error on virtiofs
>>> because it triggers this issue.  See the following explanation:
>>> PAGE_SIZE: 4096, file size: 2^63 - 1
>>> Original: offset: 2^63 - 2, count: 1
>>> Changed by buffered read: offset: 2^63 - 4096, count: 4096
>>> New offset + new count exceeds the file size as well as LLONG_MAX
>> Thanks for the report and analysis.
>>
>> However this patch may corrupt the cache if i_size changes between
>> calls to fuse_page_length().  (e.g. first page length set to 33;
>> second page length to 45; then 33-4095 will be zeroed and 4096-4140
>> will be filled from offset 33-77).  This will be mitigated by the
>> pages being invalidated when i_size changes
>> (fuse_change_attributes()).  Filling the pages with wrong data is not
>> a good idea regardless.
>>
>> I think the best approach is first to just fix the xfstest reported
>> bug by clamping the end offset to LLONG_MAX.  That's a simple patch,
>> independent of i_size, and hence trivial to verify and hard to mess
>> up.
> Applied a fix and pushed to:
>
>    git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/fuse.git#for-next

Hi Miklos,

Sorry for the late reply.

You have applied a fix quickly when I am going to send a patch today.

Just one comment for your fix:

I think we need to add the overflow check in fuse_send_readpages() and 
fuse_do_readpage().

Because generic_file_buffered_read() will call fuse_readpage() if 
fuse_readpages() fails to get page.

Thanks,

Xiao Yang

>
> Thanks,
> Miklos

Re: [PATCH] fuse: Don't make buffered read forward overflow value to a userspace process

[Miklos Szeredi]

On Thu, Feb 6, 2020 at 1:33 PM Xiao Yang <ice_yangxiao@163.com> wrote:
>
> On 2/6/20 8:14 PM, Miklos Szeredi wrote:
> > On Wed, Feb 5, 2020 at 3:37 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
> >> On Mon, Feb 3, 2020 at 8:37 AM Xiao Yang <ice_yangxiao@163.com> wrote:
> >>> Buffered read in fuse normally goes via:
> >>> -> generic_file_buffered_read()
> >>>    ------------------------------
> >>>    -> fuse_readpages()
> >>>      -> fuse_send_readpages()
> >>>    or
> >>>    -> fuse_readpage() [if fuse_readpages() fails to get page]
> >>>      -> fuse_do_readpage()
> >>>    ------------------------------
> >>>        -> fuse_simple_request()
> >>>
> >>> Buffered read changes original offset to page-aligned length by left-shift
> >>> and extends original count to be multiples of PAGE_SIZE and then fuse
> >>> forwards these new parameters to a userspace process, so it is possible
> >>> for the resulting offset(e.g page-aligned offset + extended count) to
> >>> exceed the whole file size(even the max value of off_t) when the userspace
> >>> process does read with new parameters.
> >>>
> >>> xfstests generic/525 gets "pread: Invalid argument" error on virtiofs
> >>> because it triggers this issue.  See the following explanation:
> >>> PAGE_SIZE: 4096, file size: 2^63 - 1
> >>> Original: offset: 2^63 - 2, count: 1
> >>> Changed by buffered read: offset: 2^63 - 4096, count: 4096
> >>> New offset + new count exceeds the file size as well as LLONG_MAX
> >> Thanks for the report and analysis.
> >>
> >> However this patch may corrupt the cache if i_size changes between
> >> calls to fuse_page_length().  (e.g. first page length set to 33;
> >> second page length to 45; then 33-4095 will be zeroed and 4096-4140
> >> will be filled from offset 33-77).  This will be mitigated by the
> >> pages being invalidated when i_size changes
> >> (fuse_change_attributes()).  Filling the pages with wrong data is not
> >> a good idea regardless.
> >>
> >> I think the best approach is first to just fix the xfstest reported
> >> bug by clamping the end offset to LLONG_MAX.  That's a simple patch,
> >> independent of i_size, and hence trivial to verify and hard to mess
> >> up.
> > Applied a fix and pushed to:
> >
> >    git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/fuse.git#for-next
>
> Hi Miklos,
>
> Sorry for the late reply.
>
> You have applied a fix quickly when I am going to send a patch today.
>
> Just one comment for your fix:
>
> I think we need to add the overflow check in fuse_send_readpages() and
> fuse_do_readpage().
>
> Because generic_file_buffered_read() will call fuse_readpage() if
> fuse_readpages() fails to get page.

Thanks, fixed.

Miklos

Re: [PATCH] fuse: Don't make buffered read forward overflow value to a userspace process

[Xiao Yang]

On 2/6/20 11:40 PM, Miklos Szeredi wrote:
> On Thu, Feb 6, 2020 at 1:33 PM Xiao Yang <ice_yangxiao@163.com> wrote:
>> On 2/6/20 8:14 PM, Miklos Szeredi wrote:
>>> On Wed, Feb 5, 2020 at 3:37 PM Miklos Szeredi <miklos@szeredi.hu> wrote:
>>>> On Mon, Feb 3, 2020 at 8:37 AM Xiao Yang <ice_yangxiao@163.com> wrote:
>>>>> Buffered read in fuse normally goes via:
>>>>> -> generic_file_buffered_read()
>>>>>     ------------------------------
>>>>>     -> fuse_readpages()
>>>>>       -> fuse_send_readpages()
>>>>>     or
>>>>>     -> fuse_readpage() [if fuse_readpages() fails to get page]
>>>>>       -> fuse_do_readpage()
>>>>>     ------------------------------
>>>>>         -> fuse_simple_request()
>>>>>
>>>>> Buffered read changes original offset to page-aligned length by left-shift
>>>>> and extends original count to be multiples of PAGE_SIZE and then fuse
>>>>> forwards these new parameters to a userspace process, so it is possible
>>>>> for the resulting offset(e.g page-aligned offset + extended count) to
>>>>> exceed the whole file size(even the max value of off_t) when the userspace
>>>>> process does read with new parameters.
>>>>>
>>>>> xfstests generic/525 gets "pread: Invalid argument" error on virtiofs
>>>>> because it triggers this issue.  See the following explanation:
>>>>> PAGE_SIZE: 4096, file size: 2^63 - 1
>>>>> Original: offset: 2^63 - 2, count: 1
>>>>> Changed by buffered read: offset: 2^63 - 4096, count: 4096
>>>>> New offset + new count exceeds the file size as well as LLONG_MAX
>>>> Thanks for the report and analysis.
>>>>
>>>> However this patch may corrupt the cache if i_size changes between
>>>> calls to fuse_page_length().  (e.g. first page length set to 33;
>>>> second page length to 45; then 33-4095 will be zeroed and 4096-4140
>>>> will be filled from offset 33-77).  This will be mitigated by the
>>>> pages being invalidated when i_size changes
>>>> (fuse_change_attributes()).  Filling the pages with wrong data is not
>>>> a good idea regardless.
>>>>
>>>> I think the best approach is first to just fix the xfstest reported
>>>> bug by clamping the end offset to LLONG_MAX.  That's a simple patch,
>>>> independent of i_size, and hence trivial to verify and hard to mess
>>>> up.
>>> Applied a fix and pushed to:
>>>
>>>     git://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/fuse.git#for-next
>> Hi Miklos,
>>
>> Sorry for the late reply.
>>
>> You have applied a fix quickly when I am going to send a patch today.
>>
>> Just one comment for your fix:
>>
>> I think we need to add the overflow check in fuse_send_readpages() and
>> fuse_do_readpage().
>>
>> Because generic_file_buffered_read() will call fuse_readpage() if
>> fuse_readpages() fails to get page.
> Thanks, fixed.

Hi Miklos,

Thanks a lot for your quick fix again. :-)

I have tested your patch and it works fine.

Reviewed-by: Xiao Yang <ice_yangxiao@163.com>

Thanks,

Xiao Yang

>
> Miklos