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

[Miklos Szeredi <miklos@szeredi.hu>]

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