| Message ID | 20240819064115.385086-1-peng.fan@oss.nxp.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [RFC] mm, slub: avoid zeroing kmalloc redzone | expand |
On Mon, Aug 19, 2024 at 3:32 PM Peng Fan (OSS) <peng.fan@oss.nxp.com> wrote: > > From: Peng Fan <peng.fan@nxp.com> > > With "slub_debug=FUZ init_on_free=1 loglevel=7" set in bootargs and > CONFIG_SLAB_FREELIST_HARDENED is set. There is kernel dump: > [ 0.000000] ============================================================================= > [ 0.000000] BUG kmalloc-8 (Not tainted): kmalloc Redzone overwritten > [ 0.000000] ----------------------------------------------------------------------------- > [ 0.000000] > [ 0.000000] 0xffff000010032858-0xffff00001003285f @offset=2136. First byte 0x0 instead of 0xcc > [ 0.000000] FIX kmalloc-8: Restoring kmalloc Redzone 0xffff000010032858-0xffff00001003285f=0xcc > [ 0.000000] Slab 0xfffffdffc0400c80 objects=36 used=23 fp=0xffff000010032a18 flags=0x3fffe0000000200(workingset|node=0|zone=0|lastcpupid=0x1ffff) > [ 0.000000] Object 0xffff000010032858 @offset=2136 fp=0xffff0000100328c8 > [ 0.000000] > [ 0.000000] Redzone ffff000010032850: cc cc cc cc cc cc cc cc ........ > [ 0.000000] Object ffff000010032858: cc cc cc cc cc cc cc cc ........ > [ 0.000000] Redzone ffff000010032860: cc cc cc cc cc cc cc cc ........ > [ 0.000000] Padding ffff0000100328b4: 00 00 00 00 00 00 00 00 00 00 00 00 ............ > [ 0.000000] CPU: 0 UID: 0 PID: 0 Comm: swapper/0 Not tainted 6.11.0-rc3-next-20240814-00004-g61844c55c3f4 #144 > [ 0.000000] Hardware name: NXP i.MX95 19X19 board (DT) > [ 0.000000] Call trace: > [ 0.000000] dump_backtrace+0x90/0xe8 > [ 0.000000] show_stack+0x18/0x24 > [ 0.000000] dump_stack_lvl+0x74/0x8c > [ 0.000000] dump_stack+0x18/0x24 > [ 0.000000] print_trailer+0x150/0x218 > [ 0.000000] check_object+0xe4/0x454 > [ 0.000000] free_to_partial_list+0x2f8/0x5ec > > It is because the kmalloc redzone area is cleared or orig_size is > cleared. Setting orig_size treats the wasted space (object_size - orig_size) as redzones. (in check_object()) When orig_size is set to zero, the entire object is perceived as a redzone. Could you elaborate the explanation in the description? > When s->object_size is larger than orig_size, just clear the > orig_size area. And restore the value of orig_size. > > Fixes: d57a964e09c2 ("kasan, mm: integrate slab init_on_free with HW_TAGS") I think the proper 'Fixes' commit should be 946fa0dbf2d8 ("mm/slub: extend redzone check to extra allocated kmalloc space than requested") because it is the commit that extends redzone check, but did not address init_on_free=1 case. > Signed-off-by: Peng Fan <peng.fan@nxp.com> > --- > mm/slub.c | 12 ++++++++++-- > 1 file changed, 10 insertions(+), 2 deletions(-) > > diff --git a/mm/slub.c b/mm/slub.c > index 94f5a4143825..d03957d15bbf 100644 > --- a/mm/slub.c > +++ b/mm/slub.c > @@ -2282,14 +2282,22 @@ bool slab_free_hook(struct kmem_cache *s, void *x, bool init, > */ > if (unlikely(init)) { > int rsize; > - unsigned int inuse; > + unsigned int inuse, orig_size; > > inuse = get_info_end(s); > + orig_size = get_orig_size(s, x); > if (!kasan_has_integrated_init()) > - memset(kasan_reset_tag(x), 0, s->object_size); > + memset(kasan_reset_tag(x), 0, > + s->object_size > orig_size ? orig_size : s->object_size); the size can simply be orig_size, as orig_size returns object_size when it is not enabled and orig_size can never be bigger than object_size. > rsize = (s->flags & SLAB_RED_ZONE) ? s->red_left_pad : 0; > memset((char *)kasan_reset_tag(x) + inuse, 0, > s->size - inuse - rsize); > + /* > + * Restore orig_size, otherwize kmalloc redzone overwritten > + * would be reported > + */ > + set_orig_size(s, x, orig_size);
Hi Hyeonggon, Thanks for your quick reply. > Subject: Re: [RFC] mm, slub: avoid zeroing kmalloc redzone > > On Mon, Aug 19, 2024 at 3:32 PM Peng Fan (OSS) > <peng.fan@oss.nxp.com> wrote: > > > > From: Peng Fan <peng.fan@nxp.com> > > > > With "slub_debug=FUZ init_on_free=1 loglevel=7" set in bootargs and > > CONFIG_SLAB_FREELIST_HARDENED is set. There is kernel dump: > > [ 0.000000] > ============================================================ > ================= > > [ 0.000000] BUG kmalloc-8 (Not tainted): kmalloc Redzone > overwritten > > [ 0.000000] ----------------------------------------------------------------------------- > > [ 0.000000] > > [ 0.000000] 0xffff000010032858-0xffff00001003285f > @offset=2136. First byte 0x0 instead of 0xcc > > [ 0.000000] FIX kmalloc-8: Restoring kmalloc Redzone > 0xffff000010032858-0xffff00001003285f=0xcc > > [ 0.000000] Slab 0xfffffdffc0400c80 objects=36 used=23 > fp=0xffff000010032a18 > flags=0x3fffe0000000200(workingset|node=0|zone=0|lastcpupid=0x1f > fff) > > [ 0.000000] Object 0xffff000010032858 @offset=2136 > fp=0xffff0000100328c8 > > [ 0.000000] > > [ 0.000000] Redzone ffff000010032850: cc cc cc cc cc cc cc > cc ........ > > [ 0.000000] Object ffff000010032858: cc cc cc cc cc cc cc > cc ........ > > [ 0.000000] Redzone ffff000010032860: cc cc cc cc cc cc cc > cc ........ > > [ 0.000000] Padding ffff0000100328b4: 00 00 00 00 00 00 00 00 > 00 00 00 00 ............ > > [ 0.000000] CPU: 0 UID: 0 PID: 0 Comm: swapper/0 Not tainted > 6.11.0-rc3-next-20240814-00004-g61844c55c3f4 #144 > > [ 0.000000] Hardware name: NXP i.MX95 19X19 board (DT) > > [ 0.000000] Call trace: > > [ 0.000000] dump_backtrace+0x90/0xe8 > > [ 0.000000] show_stack+0x18/0x24 > > [ 0.000000] dump_stack_lvl+0x74/0x8c > > [ 0.000000] dump_stack+0x18/0x24 > > [ 0.000000] print_trailer+0x150/0x218 > > [ 0.000000] check_object+0xe4/0x454 > > [ 0.000000] free_to_partial_list+0x2f8/0x5ec > > > > It is because the kmalloc redzone area is cleared or orig_size is > > cleared. > > Setting orig_size treats the wasted space (object_size - orig_size) as > redzones. (in check_object()) When orig_size is set to zero, the entire > object is perceived as a redzone. > Could you elaborate the explanation in the description? Quote your reply here. Hope the following is good: " With commit 946fa0dbf2d8 ("mm/slub: extend redzone check to extra allocated kmalloc space than requested"), setting orig_size treats the wasted space (object_size - orig_size) as redzones. But (in check_object()) When orig_size is set to zero, the entire object is perceived as a redzone. To a valid allocated kmalloc space, when init_on_free=1, the wasted space and the orig_size should not be cleared to 0, otherwise there will be kernel dump: [kernel dump here] To address the issue, use orig_size to clear the used area. And restore the value of orig_size after clear the remaining area. " > > > When s->object_size is larger than orig_size, just clear the orig_size > > area. And restore the value of orig_size. > > > > Fixes: d57a964e09c2 ("kasan, mm: integrate slab init_on_free with > > HW_TAGS") > > I think the proper 'Fixes' commit should be 946fa0dbf2d8 ("mm/slub: > extend redzone check to extra allocated kmalloc space than requested") > because it is the commit that extends redzone check, but did not > address init_on_free=1 case. Yeah. I will correct in v2. > > > Signed-off-by: Peng Fan <peng.fan@nxp.com> > > --- > > mm/slub.c | 12 ++++++++++-- > > 1 file changed, 10 insertions(+), 2 deletions(-) > > > > diff --git a/mm/slub.c b/mm/slub.c > > index 94f5a4143825..d03957d15bbf 100644 > > --- a/mm/slub.c > > +++ b/mm/slub.c > > @@ -2282,14 +2282,22 @@ bool slab_free_hook(struct > kmem_cache *s, void *x, bool init, > > */ > > if (unlikely(init)) { > > int rsize; > > - unsigned int inuse; > > + unsigned int inuse, orig_size; > > > > inuse = get_info_end(s); > > + orig_size = get_orig_size(s, x); > > if (!kasan_has_integrated_init()) > > - memset(kasan_reset_tag(x), 0, s->object_size); > > + memset(kasan_reset_tag(x), 0, > > + s->object_size > orig_size ? orig_size > > + : s->object_size); > > the size can simply be orig_size, as orig_size returns object_size when it > is not enabled and orig_size can never be bigger than object_size. Fix in v2: "memset(kasan_reset_tag(x), 0, orig);" > > > rsize = (s->flags & SLAB_RED_ZONE) ? s->red_left_pad : 0; > > memset((char *)kasan_reset_tag(x) + inuse, 0, > > s->size - inuse - rsize); > > + /* > > + * Restore orig_size, otherwize kmalloc redzone overwritten > > + * would be reported > > + */ > > + set_orig_size(s, x, orig_size); Thanks for help reviewing. Thanks, Peng.
diff --git a/mm/slub.c b/mm/slub.c index 94f5a4143825..d03957d15bbf 100644 --- a/mm/slub.c +++ b/mm/slub.c @@ -2282,14 +2282,22 @@ bool slab_free_hook(struct kmem_cache *s, void *x, bool init, */ if (unlikely(init)) { int rsize; - unsigned int inuse; + unsigned int inuse, orig_size; inuse = get_info_end(s); + orig_size = get_orig_size(s, x); if (!kasan_has_integrated_init()) - memset(kasan_reset_tag(x), 0, s->object_size); + memset(kasan_reset_tag(x), 0, + s->object_size > orig_size ? orig_size : s->object_size); rsize = (s->flags & SLAB_RED_ZONE) ? s->red_left_pad : 0; memset((char *)kasan_reset_tag(x) + inuse, 0, s->size - inuse - rsize); + /* + * Restore orig_size, otherwize kmalloc redzone overwritten + * would be reported + */ + set_orig_size(s, x, orig_size); + } /* KASAN might put x into memory quarantine, delaying its reuse. */ return !kasan_slab_free(s, x, init, still_accessible);