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: Tue, 30 Apr 2024 11:19:59 +0200 [thread overview]
Message-ID: <28095247-b28d-47f9-a28c-775432d2d6d3@clip-os.org> (raw)
In-Reply-To: <ae15114f-24d3-499b-9c99-ae7e098badd9@suse.cz>
On 4/29/24 22:22, Vlastimil Babka wrote:
> On 4/29/24 6:16 PM, Nicolas Bouchinet wrote:
>> On 4/29/24 16:52, Chengming Zhou wrote:
>>> On 2024/4/29 22:32, Nicolas Bouchinet wrote:
>>>> On 4/29/24 15:35, Chengming Zhou wrote:
>>>>> On 2024/4/29 20:59, Nicolas Bouchinet wrote:
>>>>>>> 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.
>>>>>> I've tried a patch where the freepointer is avoided but it results in the same bug. It seems that the commit 0f181f9fbea8bc7ea ("mm/slub.c: init_on_free=1 should wipe freelist ptr for bulk allocations") inits the freepointer on allocation if init_on_free is set in order to return a clean initialized object to the caller.
>>>>>>
>>>>> Good catch! You may need to change maybe_wipe_obj_freeptr() too,
>>>>> I haven't tested this, not sure whether it works for you. :)
>>>>>
>>>>> diff --git a/mm/slub.c b/mm/slub.c
>>>>> index 3e33ff900d35..3f250a167cb5 100644
>>>>> --- a/mm/slub.c
>>>>> +++ b/mm/slub.c
>>>>> @@ -3796,7 +3796,8 @@ static void *__slab_alloc_node(struct kmem_cache *s,
>>>>> static __always_inline void maybe_wipe_obj_freeptr(struct kmem_cache *s,
>>>>> void *obj)
>>>>> {
>>>>> - if (unlikely(slab_want_init_on_free(s)) && obj)
>>>>> + if (unlikely(slab_want_init_on_free(s)) && obj &&
>>>>> + !freeptr_outside_object(s))
>>>>> memset((void *)((char *)kasan_reset_tag(obj) + s->offset),
>>>>> 0, sizeof(void *));
>>>>> }
>>>>>
>>>>> Thanks!
>>>> Indeed since check_object() avoids objects for which freepointer is in the object and since val is equal to SLUB_RED_ACTIVE in our specific case it should work. Do you want me to add you as Co-authored ?
>>>>
>>> Ok, it's great. Thanks!
>> Now I think of it, doesn't it seems a bit odd to only properly
>> init_on_free object's freepointer only if it's inside the object ? IMHO
>> it is equally necessary to avoid information leaking about the
>> freepointer whether it is inside or outside the object.
>> I think it break the semantic of the commit 0f181f9fbea8bc7ea
>> ("mm/slub.c: init_on_free=1 should wipe freelist ptr for bulk
>> allocations") ?
> Hm, AFAIU, wiping inside object prevents misuse of some buggy kernel code
> that would allocate and accidentally leak prior content (including the
> in-object freepointer) somewhere the attacker can read. Now for wiping the
> freepointer outside the object to be useful it would have assume said
> leak-prone code to additionally be reading past the allocated object size,
> i.e. a read buffer overflow. That to me seems to be a much more rare
> combination, and also in that case such code could also likely read even
> further past the object, i.e. leak the next object's data? IOW I don't think
> it buys us much additional security protection in practice?
>
Moreover, with CONFIG_SLAB_FREELIST_HARDENED activated, freepointers are
encoded and harder to exploit.
>> Thanks.
>>
>
next prev parent reply other threads:[~2024-04-30 9:20 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
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 [this message]
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=28095247-b28d-47f9-a28c-775432d2d6d3@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.