* [PATCH] slub: Fixes freepointer encoding for single free
@ 2024-04-24 12:47 Nicolas Bouchinet
2024-04-25 8:36 ` Chengming Zhou
2024-04-26 9:20 ` Xiongwei Song
0 siblings, 2 replies; 17+ messages in thread
From: Nicolas Bouchinet @ 2024-04-24 12:47 UTC (permalink / raw
To: linux-mm, linux-kernel
Cc: cl, penberg, rientjes, iamjoonsoo.kim, akpm, vbabka,
roman.gushchin, 42.hyeyoo
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().
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().
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
--
2.44.0
^ permalink raw reply related [flat|nested] 17+ messages in thread* Re: [PATCH] slub: Fixes freepointer encoding for single free 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-26 9:20 ` Xiongwei Song 1 sibling, 1 reply; 17+ messages in thread From: Chengming Zhou @ 2024-04-25 8:36 UTC (permalink / raw To: Nicolas Bouchinet, linux-mm, linux-kernel Cc: cl, penberg, rientjes, iamjoonsoo.kim, akpm, vbabka, roman.gushchin, 42.hyeyoo 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. > > 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? 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] slub: Fixes freepointer encoding for single free 2024-04-25 8:36 ` Chengming Zhou @ 2024-04-25 15:02 ` Nicolas Bouchinet 2024-04-25 15:14 ` Chengming Zhou 0 siblings, 1 reply; 17+ messages in thread From: Nicolas Bouchinet @ 2024-04-25 15:02 UTC (permalink / raw To: Chengming Zhou, linux-mm, linux-kernel Cc: cl, penberg, rientjes, iamjoonsoo.kim, akpm, vbabka, roman.gushchin, 42.hyeyoo [-- Attachment #1: Type: text/plain, Size: 4035 bytes --] 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. > >> 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. The patch could also be optimized a bit by restricting set_freepointer() call to the `CONFIG_SLAB_FREELIST_HARDENED` option value. 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 [-- Attachment #2: Type: text/html, Size: 5229 bytes --] ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] slub: Fixes freepointer encoding for single free 2024-04-25 15:02 ` Nicolas Bouchinet @ 2024-04-25 15:14 ` Chengming Zhou 2024-04-29 8:52 ` Vlastimil Babka 0 siblings, 1 reply; 17+ messages in thread From: Chengming Zhou @ 2024-04-25 15:14 UTC (permalink / raw To: Nicolas Bouchinet, linux-mm, linux-kernel Cc: cl, penberg, rientjes, iamjoonsoo.kim, akpm, vbabka, roman.gushchin, 42.hyeyoo On 2024/4/25 23:02, Nicolas Bouchinet wrote: > 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. >> >>> 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. > > 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] slub: Fixes freepointer encoding for single free 2024-04-25 15:14 ` Chengming Zhou @ 2024-04-29 8:52 ` Vlastimil Babka 2024-04-29 9:09 ` Nicolas Bouchinet 0 siblings, 1 reply; 17+ messages in thread From: Vlastimil Babka @ 2024-04-29 8:52 UTC (permalink / raw To: Chengming Zhou, Nicolas Bouchinet, linux-mm, linux-kernel Cc: cl, penberg, rientjes, iamjoonsoo.kim, akpm, roman.gushchin, 42.hyeyoo, Xiongwei Song 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. >>> >>>> 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. >> >> 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] slub: Fixes freepointer encoding for single free 2024-04-29 8:52 ` Vlastimil Babka @ 2024-04-29 9:09 ` Nicolas Bouchinet 2024-04-29 12:59 ` Nicolas Bouchinet 0 siblings, 1 reply; 17+ messages in thread From: Nicolas Bouchinet @ 2024-04-29 9:09 UTC (permalink / raw To: Vlastimil Babka, Chengming Zhou, linux-mm, linux-kernel Cc: cl, penberg, rientjes, iamjoonsoo.kim, akpm, roman.gushchin, 42.hyeyoo, Xiongwei Song 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] slub: Fixes freepointer encoding for single free 2024-04-29 9:09 ` Nicolas Bouchinet @ 2024-04-29 12:59 ` Nicolas Bouchinet 2024-04-29 13:35 ` Chengming Zhou 0 siblings, 1 reply; 17+ messages in thread From: Nicolas Bouchinet @ 2024-04-29 12:59 UTC (permalink / raw To: Vlastimil Babka, Chengming Zhou, linux-mm, linux-kernel Cc: cl, penberg, rientjes, iamjoonsoo.kim, akpm, roman.gushchin, 42.hyeyoo, Xiongwei Song On 4/29/24 11:09, Nicolas Bouchinet wrote: > 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. 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. > >> >>>> 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 > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] slub: Fixes freepointer encoding for single free 2024-04-29 12:59 ` Nicolas Bouchinet @ 2024-04-29 13:35 ` Chengming Zhou 2024-04-29 14:32 ` Nicolas Bouchinet 0 siblings, 1 reply; 17+ messages in thread From: Chengming Zhou @ 2024-04-29 13:35 UTC (permalink / raw To: Nicolas Bouchinet, Vlastimil Babka, linux-mm, linux-kernel Cc: cl, penberg, rientjes, iamjoonsoo.kim, akpm, roman.gushchin, 42.hyeyoo, Xiongwei Song On 2024/4/29 20:59, Nicolas Bouchinet wrote: > > On 4/29/24 11:09, Nicolas Bouchinet wrote: >> 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 Ah, right, this ability seems important for debugging overflow problem. >>> >>> 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. Agree. >>> >>> 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. > > > 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! ^ permalink raw reply related [flat|nested] 17+ messages in thread
* Re: [PATCH] slub: Fixes freepointer encoding for single free 2024-04-29 13:35 ` Chengming Zhou @ 2024-04-29 14:32 ` Nicolas Bouchinet 2024-04-29 14:52 ` Chengming Zhou 0 siblings, 1 reply; 17+ messages in thread From: Nicolas Bouchinet @ 2024-04-29 14:32 UTC (permalink / raw To: Chengming Zhou, Vlastimil Babka, linux-mm, linux-kernel Cc: cl, penberg, rientjes, iamjoonsoo.kim, akpm, roman.gushchin, 42.hyeyoo, Xiongwei Song On 4/29/24 15:35, Chengming Zhou wrote: > On 2024/4/29 20:59, Nicolas Bouchinet wrote: >> On 4/29/24 11:09, Nicolas Bouchinet wrote: >>> 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 > Ah, right, this ability seems important for debugging overflow problem. > >>>> 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. > Agree. > >>>> 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. >> >> 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 ? Best regards, Nicolas ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] slub: Fixes freepointer encoding for single free 2024-04-29 14:32 ` Nicolas Bouchinet @ 2024-04-29 14:52 ` Chengming Zhou 2024-04-29 16:16 ` Nicolas Bouchinet 0 siblings, 1 reply; 17+ messages in thread From: Chengming Zhou @ 2024-04-29 14:52 UTC (permalink / raw To: Nicolas Bouchinet, Vlastimil Babka, linux-mm, linux-kernel Cc: cl, penberg, rientjes, iamjoonsoo.kim, akpm, roman.gushchin, 42.hyeyoo, Xiongwei Song 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: >>> On 4/29/24 11:09, Nicolas Bouchinet wrote: >>>> 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 >> Ah, right, this ability seems important for debugging overflow problem. >> >>>>> 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. >> Agree. >> >>>>> 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. >>> >>> 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! ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] slub: Fixes freepointer encoding for single free 2024-04-29 14:52 ` Chengming Zhou @ 2024-04-29 16:16 ` Nicolas Bouchinet 2024-04-29 20:22 ` Vlastimil Babka 0 siblings, 1 reply; 17+ messages in thread From: Nicolas Bouchinet @ 2024-04-29 16:16 UTC (permalink / raw To: Chengming Zhou, Vlastimil Babka, linux-mm, linux-kernel Cc: cl, penberg, rientjes, iamjoonsoo.kim, akpm, roman.gushchin, 42.hyeyoo, Xiongwei Song 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: >>>> On 4/29/24 11:09, Nicolas Bouchinet wrote: >>>>> 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 >>> Ah, right, this ability seems important for debugging overflow problem. >>> >>>>>> 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. >>> Agree. >>> >>>>>> 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. >>>> 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") ? Thanks. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] slub: Fixes freepointer encoding for single free 2024-04-29 16:16 ` Nicolas Bouchinet @ 2024-04-29 20:22 ` Vlastimil Babka 2024-04-30 9:19 ` Nicolas Bouchinet 0 siblings, 1 reply; 17+ messages in thread From: Vlastimil Babka @ 2024-04-29 20:22 UTC (permalink / raw To: Nicolas Bouchinet, Chengming Zhou, linux-mm, linux-kernel Cc: cl, penberg, rientjes, iamjoonsoo.kim, akpm, roman.gushchin, 42.hyeyoo, Xiongwei Song 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? > Thanks. > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] slub: Fixes freepointer encoding for single free 2024-04-29 20:22 ` Vlastimil Babka @ 2024-04-30 9:19 ` Nicolas Bouchinet 0 siblings, 0 replies; 17+ messages in thread From: Nicolas Bouchinet @ 2024-04-30 9:19 UTC (permalink / raw To: Vlastimil Babka, Chengming Zhou, linux-mm, linux-kernel Cc: cl, penberg, rientjes, iamjoonsoo.kim, akpm, roman.gushchin, 42.hyeyoo, Xiongwei Song 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. >> > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] slub: Fixes freepointer encoding for single free 2024-04-24 12:47 [PATCH] slub: Fixes freepointer encoding for single free Nicolas Bouchinet 2024-04-25 8:36 ` Chengming Zhou @ 2024-04-26 9:20 ` Xiongwei Song 2024-04-26 12:18 ` Nicolas Bouchinet 1 sibling, 1 reply; 17+ messages in thread From: Xiongwei Song @ 2024-04-26 9:20 UTC (permalink / raw To: Nicolas Bouchinet Cc: linux-mm, linux-kernel, cl, penberg, rientjes, iamjoonsoo.kim, akpm, vbabka, roman.gushchin, 42.hyeyoo On Wed, Apr 24, 2024 at 8:48 PM Nicolas Bouchinet <nicolas.bouchinet@clip-os.org> 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(). > > 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(). > > 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 If init_on_free is set, slab_free_hook() zeros the object first, then do_slab_free() calls set_freepointer() to set the fp value, so there are 8 bytes non-zero at the moment? Hence, the issue is not related to init_on_free? The fp=0x7ee4f480ce0ecd7c here is beyond kernel memory space, is the issue from CONFIG_SLAB_FREELIST_HARDENED enabled? Thanks, Xiongwei > [ 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 > -- > 2.44.0 > > ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] slub: Fixes freepointer encoding for single free 2024-04-26 9:20 ` Xiongwei Song @ 2024-04-26 12:18 ` Nicolas Bouchinet 2024-04-26 13:14 ` Xiongwei Song 0 siblings, 1 reply; 17+ messages in thread From: Nicolas Bouchinet @ 2024-04-26 12:18 UTC (permalink / raw To: Xiongwei Song Cc: linux-mm, linux-kernel, cl, penberg, rientjes, iamjoonsoo.kim, akpm, vbabka, roman.gushchin, 42.hyeyoo On 4/26/24 11:20, Xiongwei Song wrote: > On Wed, Apr 24, 2024 at 8:48 PM Nicolas Bouchinet > <nicolas.bouchinet@clip-os.org> 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(). >> >> 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(). >> >> 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 > If init_on_free is set, slab_free_hook() zeros the object first, then > do_slab_free() calls > set_freepointer() to set the fp value, so there are 8 bytes non-zero > at the moment? > Hence, the issue is not related to init_on_free? > > The fp=0x7ee4f480ce0ecd7c here is beyond kernel memory space, is the issue from > CONFIG_SLAB_FREELIST_HARDENED enabled? My understanding of the bug is that slab_free_hook() indeed zeroes the object and its metadata first, then calls do_slab_free() and directly calls __slab_free(), head an tail parameters being set to the object. If `slub_debug=F` (SLAB_CONSISTENCY_CHECKS) is set, the following call path can be executed : free_to_partial_list() -> free_debug_processing() -> free_consistency_checks() -> check_object() -> check_valid_pointer(get_freepointer()) When check_valid_pointer() is called, its object parameter is first decoded by get_freepointer(), if CONFIG_SLAB_FREELIST_HARDENED is enabled, zeroed data is then decoded by freelist_ptr_decode() using the (ptr.v ^ s->random ^ swab(ptr_addr)) operation. Does that makes sense to you or do you think I'm missing something ? Best regards, Nicolas > Xiongwei > >> [ 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 >> -- >> 2.44.0 >> >> ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] slub: Fixes freepointer encoding for single free 2024-04-26 12:18 ` Nicolas Bouchinet @ 2024-04-26 13:14 ` Xiongwei Song 2024-04-29 7:55 ` Nicolas Bouchinet 0 siblings, 1 reply; 17+ messages in thread From: Xiongwei Song @ 2024-04-26 13:14 UTC (permalink / raw To: Nicolas Bouchinet Cc: linux-mm, linux-kernel, cl, penberg, rientjes, iamjoonsoo.kim, akpm, vbabka, roman.gushchin, 42.hyeyoo On Fri, Apr 26, 2024 at 8:18 PM Nicolas Bouchinet <nicolas.bouchinet@clip-os.org> wrote: > > On 4/26/24 11:20, Xiongwei Song wrote: > > On Wed, Apr 24, 2024 at 8:48 PM Nicolas Bouchinet > > <nicolas.bouchinet@clip-os.org> 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(). > >> > >> 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(). > >> > >> 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 > > If init_on_free is set, slab_free_hook() zeros the object first, then > > do_slab_free() calls > > set_freepointer() to set the fp value, so there are 8 bytes non-zero > > at the moment? > > Hence, the issue is not related to init_on_free? > > > > The fp=0x7ee4f480ce0ecd7c here is beyond kernel memory space, is the issue from > > CONFIG_SLAB_FREELIST_HARDENED enabled? > > My understanding of the bug is that slab_free_hook() indeed zeroes the > object and its metadata first, then calls do_slab_free() and directly > calls __slab_free(), head an tail parameters being set to the object. > > If `slub_debug=F` (SLAB_CONSISTENCY_CHECKS) is set, the following call > path can be executed : > > free_to_partial_list() -> > > free_debug_processing() -> > > free_consistency_checks() -> > > check_object() -> > > check_valid_pointer(get_freepointer()) I understand the call path. I meant here the freepointer is not ZERO but an illegal value( fp=0x7ee4f480ce0ecd7c), then check_valid_pointer return 1 with it's last line and then check_object() printed out the error message. I'm not sure if I misunderstood you. Thank, Xiongwei ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH] slub: Fixes freepointer encoding for single free 2024-04-26 13:14 ` Xiongwei Song @ 2024-04-29 7:55 ` Nicolas Bouchinet 0 siblings, 0 replies; 17+ messages in thread From: Nicolas Bouchinet @ 2024-04-29 7:55 UTC (permalink / raw To: Xiongwei Song Cc: linux-mm, linux-kernel, cl, penberg, rientjes, iamjoonsoo.kim, akpm, vbabka, roman.gushchin, 42.hyeyoo On 4/26/24 15:14, Xiongwei Song wrote: > On Fri, Apr 26, 2024 at 8:18 PM Nicolas Bouchinet > <nicolas.bouchinet@clip-os.org> wrote: >> On 4/26/24 11:20, Xiongwei Song wrote: >>> On Wed, Apr 24, 2024 at 8:48 PM Nicolas Bouchinet >>> <nicolas.bouchinet@clip-os.org> 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(). >>>> >>>> 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(). >>>> >>>> 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 >>> If init_on_free is set, slab_free_hook() zeros the object first, then >>> do_slab_free() calls >>> set_freepointer() to set the fp value, so there are 8 bytes non-zero >>> at the moment? >>> Hence, the issue is not related to init_on_free? >>> >>> The fp=0x7ee4f480ce0ecd7c here is beyond kernel memory space, is the issue from >>> CONFIG_SLAB_FREELIST_HARDENED enabled? >> My understanding of the bug is that slab_free_hook() indeed zeroes the >> object and its metadata first, then calls do_slab_free() and directly >> calls __slab_free(), head an tail parameters being set to the object. >> >> If `slub_debug=F` (SLAB_CONSISTENCY_CHECKS) is set, the following call >> path can be executed : >> >> free_to_partial_list() -> >> >> free_debug_processing() -> >> >> free_consistency_checks() -> >> >> check_object() -> >> >> check_valid_pointer(get_freepointer()) > I understand the call path. I meant here the freepointer is not ZERO > but an illegal > value( fp=0x7ee4f480ce0ecd7c), Yes this is the reason of this patch. The freepointer is an illegal value because the memory range where it sits has been overridden by zeroes, set_freepointer() is never called and thus the freepointer is never properly set. The illegal value is obtained after zeroes has been decoded by get_freepointer()->freelist_ptr_decode(). > then check_valid_pointer return 1 with > it's last line > and then check_object() printed out the error message. I'm not sure if I > misunderstood you. check_valid_pointer() returns 0 since object < base, the object being the decoded fp (0x7ee4f480ce0ecd7c < 0xffff.* base addr), hence check_object() returns 0, not 1. This is why the "Object at 0x%p not freed" slab_fix() is called in free_debug_processing(). > > Thank, > Xiongwei > ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2024-04-30 9:20 UTC | newest] Thread overview: 17+ messages (download: mbox.gz follow: Atom feed -- links below jump to the message on this page -- 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 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
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.