[PATCH v2] fuse: Set *nbytesp=0 in fuse_get_user_pages on allocation failure

[PATCH v2] fuse: Set *nbytesp=0 in fuse_get_user_pages on allocation failure

[Bernd Schubert]

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>
Reviewed-by: Joanne Koong <joannelkoong@gmail.com>
---
Changes in v2:
- Set ret in the (!pages) condition only to avoid returning
  ENOMEM when the while loop would not do anything
- Remove the error check in fuse_copy_do(), that is a bit debatable.
  I had added it to prevent kernel crashes on fuse error, but then
  it causes 'visual clutter' (Joanne)
- Link to v1: https://lore.kernel.org/r/20241202-fix-fuse_get_user_pages-v1-1-8b5cccaf5bbe@ddn.com
---
 fs/fuse/file.c | 7 +++++--
 1 file changed, 5 insertions(+), 2 deletions(-)

diff --git a/fs/fuse/file.c b/fs/fuse/file.c
index 88d0946b5bc98705e0d895bc798aa4d9df080c3c..ae74d2b7ad5be14e4d157495e7c00fcf3fc40625 100644
--- a/fs/fuse/file.c
+++ b/fs/fuse/file.c
@@ -1541,8 +1541,10 @@ static int fuse_get_user_pages(struct fuse_args_pages *ap, struct iov_iter *ii,
 	 */
 	struct page **pages = kzalloc(max_pages * sizeof(struct page *),
 				      GFP_KERNEL);
-	if (!pages)
-		return -ENOMEM;
+	if (!pages) {
+		ret = -ENOMEM;
+		goto out;
+	}
 
 	while (nbytes < *nbytesp && nr_pages < max_pages) {
 		unsigned nfolios, i;
@@ -1584,6 +1586,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>

Re: [PATCH v2] fuse: Set *nbytesp=0 in fuse_get_user_pages on allocation failure

[Joanne Koong]

On Mon, Dec 2, 2024 at 3:01 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>
> Reviewed-by: Joanne Koong <joannelkoong@gmail.com>
> ---
> Changes in v2:
> - Set ret in the (!pages) condition only to avoid returning
>   ENOMEM when the while loop would not do anything
> - Remove the error check in fuse_copy_do(), that is a bit debatable.
>   I had added it to prevent kernel crashes on fuse error, but then
>   it causes 'visual clutter' (Joanne)
> - Link to v1: https://lore.kernel.org/r/20241202-fix-fuse_get_user_pages-v1-1-8b5cccaf5bbe@ddn.com
> ---
>  fs/fuse/file.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> index 88d0946b5bc98705e0d895bc798aa4d9df080c3c..ae74d2b7ad5be14e4d157495e7c00fcf3fc40625 100644
> --- a/fs/fuse/file.c
> +++ b/fs/fuse/file.c
> @@ -1541,8 +1541,10 @@ static int fuse_get_user_pages(struct fuse_args_pages *ap, struct iov_iter *ii,
>          */
>         struct page **pages = kzalloc(max_pages * sizeof(struct page *),
>                                       GFP_KERNEL);
> -       if (!pages)
> -               return -ENOMEM;
> +       if (!pages) {
> +               ret = -ENOMEM;
> +               goto out;
> +       }
>
>         while (nbytes < *nbytesp && nr_pages < max_pages) {
>                 unsigned nfolios, i;
> @@ -1584,6 +1586,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;
>

LGTM!

Thanks,
Joanne
> ---
> base-commit: e70140ba0d2b1a30467d4af6bcfe761327b9ec95
> change-id: 20241202-fix-fuse_get_user_pages-6a920cb04184
>
> Best regards,
> --
> Bernd Schubert <bschubert@ddn.com>
>

Re: [PATCH v2] fuse: Set *nbytesp=0 in fuse_get_user_pages on allocation failure

[Jingbo Xu]

Hi Miklos and Christian,

I find that these two fixes for 6.13 [1]:

fuse: fix direct io folio offset and length calculation
fuse: Set *nbytesp=0 in fuse_get_user_pages on allocation failure

are still out of v6.13-rc6 mainline, neither are they in
"linux-next/pending-fixes".  FYI in case of they got missed.

[1]
https://git.kernel.org/pub/scm/linux/kernel/git/mszeredi/fuse.git/log/?h=for-next


On 12/3/24 7:01 AM, Bernd Schubert 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>
> Reviewed-by: Joanne Koong <joannelkoong@gmail.com>
> ---
> Changes in v2:
> - Set ret in the (!pages) condition only to avoid returning
>   ENOMEM when the while loop would not do anything
> - Remove the error check in fuse_copy_do(), that is a bit debatable.
>   I had added it to prevent kernel crashes on fuse error, but then
>   it causes 'visual clutter' (Joanne)
> - Link to v1: https://lore.kernel.org/r/20241202-fix-fuse_get_user_pages-v1-1-8b5cccaf5bbe@ddn.com
> ---
>  fs/fuse/file.c | 7 +++++--
>  1 file changed, 5 insertions(+), 2 deletions(-)
> 
> diff --git a/fs/fuse/file.c b/fs/fuse/file.c
> index 88d0946b5bc98705e0d895bc798aa4d9df080c3c..ae74d2b7ad5be14e4d157495e7c00fcf3fc40625 100644
> --- a/fs/fuse/file.c
> +++ b/fs/fuse/file.c
> @@ -1541,8 +1541,10 @@ static int fuse_get_user_pages(struct fuse_args_pages *ap, struct iov_iter *ii,
>  	 */
>  	struct page **pages = kzalloc(max_pages * sizeof(struct page *),
>  				      GFP_KERNEL);
> -	if (!pages)
> -		return -ENOMEM;
> +	if (!pages) {
> +		ret = -ENOMEM;
> +		goto out;
> +	}
>  
>  	while (nbytes < *nbytesp && nr_pages < max_pages) {
>  		unsigned nfolios, i;
> @@ -1584,6 +1586,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,

-- 
Thanks,
Jingbo

Re: [PATCH v2] fuse: Set *nbytesp=0 in fuse_get_user_pages on allocation failure

[Miklos Szeredi]

On Mon, 6 Jan 2025 at 02:58, Jingbo Xu <jefflexu@linux.alibaba.com> wrote:
>
> Hi Miklos and Christian,
>
> I find that these two fixes for 6.13 [1]:
>
> fuse: fix direct io folio offset and length calculation
> fuse: Set *nbytesp=0 in fuse_get_user_pages on allocation failure
>
> are still out of v6.13-rc6 mainline, neither are they in
> "linux-next/pending-fixes".  FYI in case of they got missed.

Thanks.  Will send a PR to Christian shortly.

Miklos