linux-kernel - Re: [PATCH] mm/util: make memdup_user_nul() similar to memdup

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <6ac4cb00-4841-4deb-a3f9-7948a1ba5389@I-love.SAKURA.ne.jp>
Date: Mon, 30 Dec 2024 11:55:07 +0900
From: Tetsuo Handa <penguin-kernel@...ove.SAKURA.ne.jp>
To: Andrew Morton <akpm@...ux-foundation.org>
Cc: linux-mm <linux-mm@...ck.org>, LKML <linux-kernel@...r.kernel.org>,
        Al Viro <viro@...iv.linux.org.uk>, Denis Efremov <efremov@...ux.com>,
        Julia Lawall <Julia.Lawall@...ia.fr>, Michal Hocko <mhocko@...e.com>,
        Kees Cook <kees@...nel.org>, Vlastimil Babka <vbabka@...e.cz>
Subject: Re: [PATCH] mm/util: make memdup_user_nul() similar to memdup_user()

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@...ove.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@...kaller.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.
>