From: Nicolas Bouchinet <nicolas.bouchinet@clip-os.org>
To: Vlastimil Babka <vbabka@suse.cz>,
Chengming Zhou <chengming.zhou@linux.dev>,
linux-mm@kvack.org, linux-kernel@vger.kernel.org
Cc: cl@linux.com, penberg@kernel.org, rientjes@google.com,
iamjoonsoo.kim@lge.com, akpm@linux-foundation.org,
roman.gushchin@linux.dev, 42.hyeyoo@gmail.com,
Xiongwei Song <xiongwei.song@windriver.com>
Subject: Re: [PATCH] slub: Fixes freepointer encoding for single free
Date: Mon, 29 Apr 2024 11:09:51 +0200 [thread overview]
Message-ID: <d17d7b20-b99e-46ce-b7bf-fb7058a66e79@clip-os.org> (raw)
In-Reply-To: <6f977874-2a18-44ef-b207-9eb0baad9d66@suse.cz>
Hi Vlastimil,
thanks for your review and your proposal.
On 4/29/24 10:52, Vlastimil Babka wrote:
> On 4/25/24 5:14 PM, Chengming Zhou wrote:
>> On 2024/4/25 23:02, Nicolas Bouchinet wrote:
> Thanks for finding the bug and the fix!
>
>>> Hy,
>>>
>>> First of all, thanks a lot for your time.
>>>
>>> On 4/25/24 10:36, Chengming Zhou wrote:
>>>> On 2024/4/24 20:47, Nicolas Bouchinet wrote:
>>>>> From: Nicolas Bouchinet<nicolas.bouchinet@ssi.gouv.fr>
>>>>>
>>>>> Commit 284f17ac13fe ("mm/slub: handle bulk and single object freeing
>>>>> separately") splits single and bulk object freeing in two functions
>>>>> slab_free() and slab_free_bulk() which leads slab_free() to call
>>>>> slab_free_hook() directly instead of slab_free_freelist_hook().
>>>> Right.
>>>> y not suitable for a stable-candidate fix we need
>>>>> If `init_on_free` is set, slab_free_hook() zeroes the object.
>>>>> Afterward, if `slub_debug=F` and `CONFIG_SLAB_FREELIST_HARDENED` are
>>>>> set, the do_slab_free() slowpath executes freelist consistency
>>>>> checks and try to decode a zeroed freepointer which leads to a
>>>>> "Freepointer corrupt" detection in check_object().
>>>> IIUC, the "freepointer" can be checked on the free path only when
>>>> it's outside the object memory. Here slab_free_hook() zeroed the
>>>> freepointer and caused the problem.
>>>>
>>>> But why we should zero the memory outside the object_size? It seems
>>>> more reasonable to only zero the object_size when init_on_free is set?
>>> The original purpose was to avoid leaking information through the object and its metadata / tracking information as described in init_on_free initial Commit 6471384af2a6 ("mm: security: introduce init_on_alloc=1 and init_on_free=1 boot options").
>>>
>>> I have to admit I didn't read the entire lore about the original patchset yet, though it could be interesting to know a bit more the threat models, specifically regarding the object metadata init.
>> Thank you for the reference! I also don't get why it needs to zero
>> the metadata and tracking information.
> Hmm taking a step back, it seems really suboptimal to initialize the
> outside-object freepointer as part of init_on_free:
>
> - the freeing itself will always set it one way or another, in this case
> free_to_partial_list() will do set_freepointer() after free_debug_processing()
>
> - we lose the ability to detect if the allocated slab object's user wrote to
> it, which is a buffer overflow
>
> So the best option to me would be to adjust the init in slab_free_hook() to
> avoid the outside-object freepointer similarly to how it avoids the red zone.
>
> We'll still not have the buffer overflow detection ability for bulk free
> where slab_free_freelist_hook() will set the free pointer before we reach
> the checks, but changing that is most likely not worth the trouble, and
> especially not suitable for a stable-candidate fix we need here.
It seems like a good alternative to me, I'll push a V2 patch with those
changes.
I help maintaining the Linux-Hardened patchset in which we have a slab
object canary feature that helps detecting overflows. It is located just
after the object freepointer.
>
>>> The patch could also be optimized a bit by restricting set_freepointer() call to the `CONFIG_SLAB_FREELIST_HARDENED` option value.
>>>
>> Yeah. Maybe memcg_alloc_abort_single() needs this too.
>>
>> Thanks.
>>
>>> Thanks again, Nicolas
>>>
>>>> Thanks.
>>>>
>>>>> Object's freepointer thus needs to be properly set using
>>>>> set_freepointer() after init_on_free.
>>>>>
>>>>> To reproduce, set `slub_debug=FU init_on_free=1 log_level=7` on the
>>>>> command line of a kernel build with `CONFIG_SLAB_FREELIST_HARDENED=y`.
>>>>>
>>>>> dmesg sample log:
>>>>> [ 10.708715] =============================================================================
>>>>> [ 10.710323] BUG kmalloc-rnd-05-32 (Tainted: G B T ): Freepointer corrupt
>>>>> [ 10.712695] -----------------------------------------------------------------------------
>>>>> [ 10.712695]
>>>>> [ 10.712695] Slab 0xffffd8bdc400d580 objects=32 used=4 fp=0xffff9d9a80356f80 flags=0x200000000000a00(workingset|slab|node=0|zone=2)
>>>>> [ 10.716698] Object 0xffff9d9a80356600 @offset=1536 fp=0x7ee4f480ce0ecd7c
>>>>> [ 10.716698]
>>>>> [ 10.716698] Bytes b4 ffff9d9a803565f0: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
>>>>> [ 10.720703] Object ffff9d9a80356600: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
>>>>> [ 10.720703] Object ffff9d9a80356610: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
>>>>> [ 10.724696] Padding ffff9d9a8035666c: 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 00 ................
>>>>> [ 10.724696] Padding ffff9d9a8035667c: 00 00 00 00 ....
>>>>> [ 10.724696] FIX kmalloc-rnd-05-32: Object at 0xffff9d9a80356600 not freed
>>>>>
>>>>> Signed-off-by: Nicolas Bouchinet<nicolas.bouchinet@ssi.gouv.fr>
>>>>> ---
>>>>> mm/slub.c | 8 +++++++-
>>>>> 1 file changed, 7 insertions(+), 1 deletion(-)
>>>>>
>>>>> diff --git a/mm/slub.c b/mm/slub.c
>>>>> index 3aa12b9b323d9..71dbff9ad8f17 100644
>>>>> --- a/mm/slub.c
>>>>> +++ b/mm/slub.c
>>>>> @@ -4342,10 +4342,16 @@ static __fastpath_inline
>>>>> void slab_free(struct kmem_cache *s, struct slab *slab, void *object,
>>>>> unsigned long addr)
>>>>> {
>>>>> + bool init = false;
>>>>> +
>>>>> memcg_slab_free_hook(s, slab, &object, 1);
>>>>> + init = slab_want_init_on_free(s);
>>>>> - if (likely(slab_free_hook(s, object, slab_want_init_on_free(s))))
>>>>> + if (likely(slab_free_hook(s, object, init))) {
>>>>> + if (init)
>>>>> + set_freepointer(s, object, NULL);
>>>>> do_slab_free(s, slab, object, object, 1, addr);
>>>>> + }
>>>>> }
>>>>> static __fastpath_inline
Thanks again for your review,
Nicolas
next prev parent reply other threads:[~2024-04-29 9:10 UTC|newest]
Thread overview: 17+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-04-24 12:47 [PATCH] slub: Fixes freepointer encoding for single free Nicolas Bouchinet
2024-04-25 8:36 ` Chengming Zhou
2024-04-25 15:02 ` Nicolas Bouchinet
2024-04-25 15:14 ` Chengming Zhou
2024-04-29 8:52 ` Vlastimil Babka
2024-04-29 9:09 ` Nicolas Bouchinet [this message]
2024-04-29 12:59 ` Nicolas Bouchinet
2024-04-29 13:35 ` Chengming Zhou
2024-04-29 14:32 ` Nicolas Bouchinet
2024-04-29 14:52 ` Chengming Zhou
2024-04-29 16:16 ` Nicolas Bouchinet
2024-04-29 20:22 ` Vlastimil Babka
2024-04-30 9:19 ` Nicolas Bouchinet
2024-04-26 9:20 ` Xiongwei Song
2024-04-26 12:18 ` Nicolas Bouchinet
2024-04-26 13:14 ` Xiongwei Song
2024-04-29 7:55 ` Nicolas Bouchinet
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=d17d7b20-b99e-46ce-b7bf-fb7058a66e79@clip-os.org \
--to=nicolas.bouchinet@clip-os.org \
--cc=42.hyeyoo@gmail.com \
--cc=akpm@linux-foundation.org \
--cc=chengming.zhou@linux.dev \
--cc=cl@linux.com \
--cc=iamjoonsoo.kim@lge.com \
--cc=linux-kernel@vger.kernel.org \
--cc=linux-mm@kvack.org \
--cc=penberg@kernel.org \
--cc=rientjes@google.com \
--cc=roman.gushchin@linux.dev \
--cc=vbabka@suse.cz \
--cc=xiongwei.song@windriver.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.