On 12/2/24 23:37, Joanne Koong wrote: > 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; Hi Joanne, sorry, just noticed that unconditionally setting -ENOMEM might cause issues - sent out v2, but kept your review. Hope that is ok. Thanks, Bernd
