diff mbox series

fuse: Set *nbytesp=0 in fuse_get_user_pages on allocation failure

Message ID 20241202-fix-fuse_get_user_pages-v1-1-8b5cccaf5bbe@ddn.com (mailing list archive)
State New
Headers show
Series fuse: Set *nbytesp=0 in fuse_get_user_pages on allocation failure | expand

Commit Message

Bernd Schubert Dec. 2, 2024, 10:10 p.m. UTC 
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@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=87b8e6ed25dbc41759f7
Fixes: 3b97c3652d91 ("fuse: convert direct io to use folios")
Signed-off-by: Bernd Schubert <bschubert@ddn.com>
---
 fs/fuse/dev.c  | 3 +++
 fs/fuse/file.c | 4 +++-
 2 files changed, 6 insertions(+), 1 deletion(-)


---
base-commit: e70140ba0d2b1a30467d4af6bcfe761327b9ec95
change-id: 20241202-fix-fuse_get_user_pages-6a920cb04184

Best regards,

Comments

Joanne Koong Dec. 2, 2024, 10:37 p.m. UTC | #1 
On Mon, Dec 2, 2024 at 2:10 PM Bernd Schubert <bschubert@ddn.com> 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@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=87b8e6ed25dbc41759f7
> Fixes: 3b97c3652d91 ("fuse: convert direct io to use folios")
> Signed-off-by: Bernd Schubert <bschubert@ddn.com>

Nice catch! I totally missed that just returning err isn't enough.
Thanks for fixing!!

Reviewed-by: Joanne Koong <joannelkoong@gmail.com>

> ---
>  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@ddn.com>
>
Bernd Schubert Dec. 2, 2024, 11:04 p.m. UTC | #2 
On 12/2/24 23:37, Joanne Koong wrote:
> On Mon, Dec 2, 2024 at 2:10 PM Bernd Schubert <bschubert@ddn.com> 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@syzkaller.appspotmail.com
>> Closes: https://syzkaller.appspot.com/bug?extid=87b8e6ed25dbc41759f7
>> Fixes: 3b97c3652d91 ("fuse: convert direct io to use folios")
>> Signed-off-by: Bernd Schubert <bschubert@ddn.com>
> 
> Nice catch! I totally missed that just returning err isn't enough.
> Thanks for fixing!!
> 
> Reviewed-by: Joanne Koong <joannelkoong@gmail.com>
> 
>> ---
>>  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
diff mbox series

Patch

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;
+
 		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;