[PATCH] mm/util: make memdup_user_nul() similar to memdup_user()

[PATCH] mm/util: make memdup_user_nul() similar to memdup_user()

[Tetsuo Handa]

Since the string data to copy from userspace is likely less than PAGE_SIZE
bytes, replace GFP_KERNEL with GFP_USER like commit 6c2c97a24f09
("memdup_user(): switch to GFP_USER") does and add __GFP_NOWARN like commit
6c8fcc096be9 ("mm: don't let userspace spam allocations warnings") does.
Also, use dedicated slab buckets like commit d73778e4b867 ("mm/util: Use
dedicated slab buckets for memdup_user()") does.

Reported-by: syzbot+7e12e97b36154c54414b@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=7e12e97b36154c54414b
Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>
---
 mm/util.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/mm/util.c b/mm/util.c
index c1c3b06ab4f9..60aa40f612b8 100644
--- a/mm/util.c
+++ b/mm/util.c
@@ -297,12 +297,7 @@ void *memdup_user_nul(const void __user *src, size_t len)
 {
 	char *p;
 
-	/*
-	 * Always use GFP_KERNEL, since copy_from_user() can sleep and
-	 * cause pagefault, which makes it pointless to use GFP_NOFS
-	 * or GFP_ATOMIC.
-	 */
-	p = kmalloc_track_caller(len + 1, GFP_KERNEL);
+	p = kmem_buckets_alloc_track_caller(user_buckets, len + 1, GFP_USER | __GFP_NOWARN);
 	if (!p)
 		return ERR_PTR(-ENOMEM);
 
-- 
2.43.5

Re: [PATCH] mm/util: make memdup_user_nul() similar to memdup_user()

[Andrew Morton]

tl;dr: patch does three different things, some of which appear to be
needed in -stable kernels.


On Sat, 21 Dec 2024 16:47:29 +0900 Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> wrote:

> Since the string data to copy from userspace is likely less than PAGE_SIZE
> bytes, replace GFP_KERNEL with GFP_USER like commit 6c2c97a24f09
> ("memdup_user(): switch to GFP_USER") does

Please provide a reason for this change.  Does it have user-visible
effects?  If so, what are they?

> and add __GFP_NOWARN like commit
> 6c8fcc096be9 ("mm: don't let userspace spam allocations warnings") does.

Ditto.

> Also, use dedicated slab buckets like commit d73778e4b867 ("mm/util: Use
> dedicated slab buckets for memdup_user()") does.

Ditto.

> Reported-by: syzbot+7e12e97b36154c54414b@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=7e12e97b36154c54414b

That's a userspace-triggered WARN, so we'll want to backport the fix
into -stable kernels.  But we won't necessarly want to backport the
other two changes, depending upon what their effects are.


In other words, it would be better to present this as a series of three
(fully changelogged!) patches, with one or more of them cc:stable.

If we really do want to roll all three changes into a single patch and
backport that then please let's justify all three backports within the
changelog.

Re: [PATCH] mm/util: make memdup_user_nul() similar to memdup_user()

[Tetsuo Handa]

On 2024/12/24 10:25, Andrew Morton wrote:
> 
> tl;dr: patch does three different things, some of which appear to be
> needed in -stable kernels.
> 
> 
> On Sat, 21 Dec 2024 16:47:29 +0900 Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> wrote:
> 
>> Since the string data to copy from userspace is likely less than PAGE_SIZE
>> bytes, replace GFP_KERNEL with GFP_USER like commit 6c2c97a24f09
>> ("memdup_user(): switch to GFP_USER") does
> 
> Please provide a reason for this change.  Does it have user-visible
> effects?  If so, what are they?

I think it is hardly user-visible, for GFP_USER allocations up to 8 * PAGE_SIZE bytes
unlikely fails. Also, if there are memdup_user_nul() callers that need larger than
8 * PAGE_SIZE bytes, such user would ask for addition of vmemdup_user_nul().

> 
>> and add __GFP_NOWARN like commit
>> 6c8fcc096be9 ("mm: don't let userspace spam allocations warnings") does.
> 
> Ditto.
> 
>> Also, use dedicated slab buckets like commit d73778e4b867 ("mm/util: Use
>> dedicated slab buckets for memdup_user()") does.
> 
> Ditto.
> 
>> Reported-by: syzbot+7e12e97b36154c54414b@syzkaller.appspotmail.com
>> Closes: https://syzkaller.appspot.com/bug?extid=7e12e97b36154c54414b
> 
> That's a userspace-triggered WARN, so we'll want to backport the fix
> into -stable kernels.  But we won't necessarly want to backport the
> other two changes, depending upon what their effects are.

I don't think we need to backport the fix. This problem was found by fuzz
testing, and the effect of not backporting is nothing but "too large memory
allocation attempt warning" message is printed.

> 
> 
> In other words, it would be better to present this as a series of three
> (fully changelogged!) patches, with one or more of them cc:stable.

Maybe commit 547ade42ced0 ("coccinelle: api: extend memdup_user
transformation with GFP_USER") provided better description than
commit 6c2c97a24f09 ("memdup_user(): switch to GFP_USER").

commit 547ade42ced037db77a82e67d70b55c0eecc49e0:

    coccinelle: api: extend memdup_user transformation with GFP_USER

    Match GFP_USER and optional __GFP_NOWARN allocations with
    memdup_user.cocci rule.
    Commit 6c2c97a24f09 ("memdup_user(): switch to GFP_USER") switched
    memdup_user() from GFP_KERNEL to GFP_USER. In almost all cases it
    is still a good idea to recommend memdup_user() for GFP_KERNEL
    allocations. The motivation behind altering memdup_user() to GFP_USER:
    https://lkml.org/lkml/2018/1/6/333

commit 6c8fcc096be9d02f478c508052a41a4430506ab3:

    mm: don't let userspace spam allocations warnings

    memdump_user usually gets fed unchecked userspace input.  Blasting a
    full backtrace into dmesg every time is a bit excessive - I'm not sure
    on the kernel rule in general, but at least in drm we're trying not to
    let unpriviledge userspace spam the logs freely.  Definitely not entire
    warning backtraces.

    It also means more filtering for our CI, because our testsuite exercises
    these corner cases and so hits these a lot.

> 
> If we really do want to roll all three changes into a single patch and
> backport that then please let's justify all three backports within the
> changelog.
> 

Re: [PATCH] mm/util: make memdup_user_nul() similar to memdup_user()

[Tetsuo Handa]

Would you prefer updating the description to something like below?



The only difference between memdup_user_nul() and memdup_user() should be
that the former appends trailing nul byte. Apply three changes in the
latter to the former.

  commit 6c2c97a24f09 ("memdup_user(): switch to GFP_USER")
  commit 6c8fcc096be9 ("mm: don't let userspace spam allocations warnings")
  commit d73778e4b867 ("mm/util: Use dedicated slab buckets for
                        memdup_user()")

Re: [PATCH] mm/util: make memdup_user_nul() similar to memdup_user()

[Kees Cook]

On Sat, Dec 21, 2024 at 04:47:29PM +0900, Tetsuo Handa wrote:
> Since the string data to copy from userspace is likely less than PAGE_SIZE
> bytes, replace GFP_KERNEL with GFP_USER like commit 6c2c97a24f09
> ("memdup_user(): switch to GFP_USER") does and add __GFP_NOWARN like commit
> 6c8fcc096be9 ("mm: don't let userspace spam allocations warnings") does.
> Also, use dedicated slab buckets like commit d73778e4b867 ("mm/util: Use
> dedicated slab buckets for memdup_user()") does.
> 
> Reported-by: syzbot+7e12e97b36154c54414b@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=7e12e97b36154c54414b
> Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp>

Thanks for this! Yes, please. With the commit log updated to answer
akpm's questions:

Acked-by: Kees Cook <kees@kernel.org>

-Kees

> ---
>  mm/util.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
> 
> diff --git a/mm/util.c b/mm/util.c
> index c1c3b06ab4f9..60aa40f612b8 100644
> --- a/mm/util.c
> +++ b/mm/util.c
> @@ -297,12 +297,7 @@ void *memdup_user_nul(const void __user *src, size_t len)
>  {
>  	char *p;
>  
> -	/*
> -	 * Always use GFP_KERNEL, since copy_from_user() can sleep and
> -	 * cause pagefault, which makes it pointless to use GFP_NOFS
> -	 * or GFP_ATOMIC.
> -	 */
> -	p = kmalloc_track_caller(len + 1, GFP_KERNEL);
> +	p = kmem_buckets_alloc_track_caller(user_buckets, len + 1, GFP_USER | __GFP_NOWARN);
>  	if (!p)
>  		return ERR_PTR(-ENOMEM);
>  
> -- 
> 2.43.5

-- 
Kees Cook