On Mon, Dec 2, 2024 at 2:10 PM Bernd Schubert <bschubert@xxxxxxx> wrote: > > In fuse_get_user_pages(), set *nbytesp to 0 when struct page **pages > allocation fails. This prevents the caller (fuse_direct_io) from making > incorrect assumptions that could lead to NULL pointer dereferences > when processing the request reply. > > Previously, *nbytesp was left unmodified on allocation failure, which > could cause issues if the caller assumed pages had been added to > ap->descs[] when they hadn't. > > Reported-by: syzbot+87b8e6ed25dbc41759f7@xxxxxxxxxxxxxxxxxxxxxxxxx > Closes: https://syzkaller.appspot.com/bug?extid=87b8e6ed25dbc41759f7 > Fixes: 3b97c3652d91 ("fuse: convert direct io to use folios") > Signed-off-by: Bernd Schubert <bschubert@xxxxxxx> Nice catch! I totally missed that just returning err isn't enough. Thanks for fixing!! Reviewed-by: Joanne Koong <joannelkoong@xxxxxxxxx> > --- > fs/fuse/dev.c | 3 +++ > fs/fuse/file.c | 4 +++- > 2 files changed, 6 insertions(+), 1 deletion(-) > > diff --git a/fs/fuse/dev.c b/fs/fuse/dev.c > index 27ccae63495d14ea339aa6c8da63d0ac44fc8885..2b506493d235e171336f737ba7a380fe16c9f825 100644 > --- a/fs/fuse/dev.c > +++ b/fs/fuse/dev.c > @@ -803,6 +803,9 @@ static int fuse_copy_do(struct fuse_copy_state *cs, void **val, unsigned *size) > void *pgaddr = kmap_local_page(cs->pg); > void *buf = pgaddr + cs->offset; > > + if (WARN_ON_ONCE(!*val)) > + return -EIO; We should never run into this case so it makes sense to me to also not have this line in (to reduce visual clutter in the code) or to just have this be WARN_ON_ONCE(!*val); > + > if (cs->write) > memcpy(buf, *val, ncpy); > else > diff --git a/fs/fuse/file.c b/fs/fuse/file.c > index 88d0946b5bc98705e0d895bc798aa4d9df080c3c..a8960a2908014250a81e1651d8a611b6936848e2 100644 > --- a/fs/fuse/file.c > +++ b/fs/fuse/file.c > @@ -1539,10 +1539,11 @@ static int fuse_get_user_pages(struct fuse_args_pages *ap, struct iov_iter *ii, > * manually extract pages using iov_iter_extract_pages() and then > * copy that to a folios array. > */ > + ret = -ENOMEM; > struct page **pages = kzalloc(max_pages * sizeof(struct page *), > GFP_KERNEL); > if (!pages) > - return -ENOMEM; > + goto out; > > while (nbytes < *nbytesp && nr_pages < max_pages) { > unsigned nfolios, i; > @@ -1584,6 +1585,7 @@ static int fuse_get_user_pages(struct fuse_args_pages *ap, struct iov_iter *ii, > else > ap->args.out_pages = true; > > +out: > *nbytesp = nbytes; > > return ret < 0 ? ret : 0; > > --- > base-commit: e70140ba0d2b1a30467d4af6bcfe761327b9ec95 > change-id: 20241202-fix-fuse_get_user_pages-6a920cb04184 > > Best regards, > -- > Bernd Schubert <bschubert@xxxxxxx> >
