* [PATCH v7 0/3] mm/slub: extend redzone check for kmalloc objects @ 2022-10-21 3:24 Feng Tang 2022-10-21 3:24 ` [PATCH v7 1/3] mm/slub: only zero requested size of buffer for kzalloc when debug enabled Feng Tang ` (3 more replies) 0 siblings, 4 replies; 20+ messages in thread From: Feng Tang @ 2022-10-21 3:24 UTC (permalink / raw) To: Andrew Morton, Vlastimil Babka, Christoph Lameter, Pekka Enberg, David Rientjes, Joonsoo Kim, Roman Gushchin, Hyeonggon Yoo, Dmitry Vyukov, Andrey Konovalov, Kees Cook Cc: Dave Hansen, linux-mm, linux-kernel, kasan-dev, Feng Tang kmalloc's API family is critical for mm, and one of its nature is that it will round up the request size to a fixed one (mostly power of 2). When user requests memory for '2^n + 1' bytes, actually 2^(n+1) bytes could be allocated, so there is an extra space than what is originally requested. This patchset tries to extend the redzone sanity check to the extra kmalloced buffer than requested, to better detect un-legitimate access to it. (dependson SLAB_STORE_USER & SLAB_RED_ZONE) The redzone part has been tested with code below: for (shift = 3; shift <= 12; shift++) { size = 1 << shift; buf = kmalloc(size + 4, GFP_KERNEL); /* We have 96, 196 kmalloc size, which is not power of 2 */ if (size == 64 || size == 128) oob_size = 16; else oob_size = size - 4; memset(buf + size + 4, 0xee, oob_size); kfree(buf); } (This is against slab tree's 'for-6.2/slub-sysfs' branch, with HEAD 54736f702526) Please help to review, thanks! - Feng --- Changelogs: since v6: * 1/4 patch of kmalloc memory wastage debug patch was merged to 6.1-rc1, so drop it * refine the kasan patch by extending existing APIs and hiding kasan internal data structure info (Andrey Konovalov) * only reduce zeroing size when slub debug is enabled to avoid security risk (Kees Cook/Andrey Konovalov) * collect Acked-by tag from Hyeonggon Yoo since v5: * Refine code/comments and add more perf info in commit log for kzalloc change (Hyeonggoon Yoo) * change the kasan param name and refine comments about kasan+redzone handling (Andrey Konovalov) * put free pointer in meta data to make redzone check cover all kmalloc objects (Hyeonggoon Yoo) since v4: * fix a race issue in v3, by moving kmalloc debug init into alloc_debug_processing (Hyeonggon Yoo) * add 'partial_conext' for better parameter passing in get_partial() call chain (Vlastimil Babka) * update 'slub.rst' for 'alloc_traces' part (Hyeonggon Yoo) * update code comments for 'orig_size' since v3: * rebase against latest post 6.0-rc1 slab tree's 'for-next' branch * fix a bug reported by 0Day, that kmalloc-redzoned data and kasan's free meta data overlaps in the same kmalloc object data area since v2: * rebase against slab tree's 'for-next' branch * fix pointer handling (Kefeng Wang) * move kzalloc zeroing handling change to a separate patch (Vlastimil Babka) * make 'orig_size' only depend on KMALLOC & STORE_USER flag bits (Vlastimil Babka) since v1: * limit the 'orig_size' to kmalloc objects only, and save it after track in metadata (Vlastimil Babka) * fix a offset calculation problem in print_trailer since RFC: * fix problems in kmem_cache_alloc_bulk() and records sorting, improve the print format (Hyeonggon Yoo) * fix a compiling issue found by 0Day bot * update the commit log based info from iova developers Feng Tang (3): mm/slub: only zero requested size of buffer for kzalloc when debug enabled mm: kasan: Extend kasan_metadata_size() to also cover in-object size mm/slub: extend redzone check to extra allocated kmalloc space than requested include/linux/kasan.h | 5 ++-- mm/kasan/generic.c | 19 +++++++++---- mm/slab.c | 7 +++-- mm/slab.h | 22 +++++++++++++-- mm/slab_common.c | 4 +++ mm/slub.c | 65 +++++++++++++++++++++++++++++++++++++------ 6 files changed, 100 insertions(+), 22 deletions(-) -- 2.34.1 ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v7 1/3] mm/slub: only zero requested size of buffer for kzalloc when debug enabled 2022-10-21 3:24 [PATCH v7 0/3] mm/slub: extend redzone check for kmalloc objects Feng Tang @ 2022-10-21 3:24 ` Feng Tang 2022-10-24 14:00 ` Hyeonggon Yoo ` (2 more replies) 2022-10-21 3:24 ` [PATCH v7 2/3] mm: kasan: Extend kasan_metadata_size() to also cover in-object size Feng Tang ` (2 subsequent siblings) 3 siblings, 3 replies; 20+ messages in thread From: Feng Tang @ 2022-10-21 3:24 UTC (permalink / raw) To: Andrew Morton, Vlastimil Babka, Christoph Lameter, Pekka Enberg, David Rientjes, Joonsoo Kim, Roman Gushchin, Hyeonggon Yoo, Dmitry Vyukov, Andrey Konovalov, Kees Cook Cc: Dave Hansen, linux-mm, linux-kernel, kasan-dev, Feng Tang kzalloc/kmalloc will round up the request size to a fixed size (mostly power of 2), so the allocated memory could be more than requested. Currently kzalloc family APIs will zero all the allocated memory. To detect out-of-bound usage of the extra allocated memory, only zero the requested part, so that redzone sanity check could be added to the extra space later. For kzalloc users who will call ksize() later and utilize this extra space, please be aware that the space is not zeroed any more when debug is enabled. (Thanks to Kees Cook's effort to sanitize all ksize() user cases [1], this won't be a big issue). [1]. https://lore.kernel.org/all/20220922031013.2150682-1-keescook@chromium.org/#r Signed-off-by: Feng Tang <feng.tang@intel.com> --- mm/slab.c | 7 ++++--- mm/slab.h | 18 ++++++++++++++++-- mm/slub.c | 10 +++++++--- 3 files changed, 27 insertions(+), 8 deletions(-) diff --git a/mm/slab.c b/mm/slab.c index a5486ff8362a..4594de0e3d6b 100644 --- a/mm/slab.c +++ b/mm/slab.c @@ -3253,7 +3253,8 @@ slab_alloc_node(struct kmem_cache *cachep, struct list_lru *lru, gfp_t flags, init = slab_want_init_on_alloc(flags, cachep); out: - slab_post_alloc_hook(cachep, objcg, flags, 1, &objp, init); + slab_post_alloc_hook(cachep, objcg, flags, 1, &objp, init, + cachep->object_size); return objp; } @@ -3506,13 +3507,13 @@ int kmem_cache_alloc_bulk(struct kmem_cache *s, gfp_t flags, size_t size, * Done outside of the IRQ disabled section. */ slab_post_alloc_hook(s, objcg, flags, size, p, - slab_want_init_on_alloc(flags, s)); + slab_want_init_on_alloc(flags, s), s->object_size); /* FIXME: Trace call missing. Christoph would like a bulk variant */ return size; error: local_irq_enable(); cache_alloc_debugcheck_after_bulk(s, flags, i, p, _RET_IP_); - slab_post_alloc_hook(s, objcg, flags, i, p, false); + slab_post_alloc_hook(s, objcg, flags, i, p, false, s->object_size); kmem_cache_free_bulk(s, i, p); return 0; } diff --git a/mm/slab.h b/mm/slab.h index 0202a8c2f0d2..8b4ee02fc14a 100644 --- a/mm/slab.h +++ b/mm/slab.h @@ -720,12 +720,26 @@ static inline struct kmem_cache *slab_pre_alloc_hook(struct kmem_cache *s, static inline void slab_post_alloc_hook(struct kmem_cache *s, struct obj_cgroup *objcg, gfp_t flags, - size_t size, void **p, bool init) + size_t size, void **p, bool init, + unsigned int orig_size) { + unsigned int zero_size = s->object_size; size_t i; flags &= gfp_allowed_mask; + /* + * For kmalloc object, the allocated memory size(object_size) is likely + * larger than the requested size(orig_size). If redzone check is + * enabled for the extra space, don't zero it, as it will be redzoned + * soon. The redzone operation for this extra space could be seen as a + * replacement of current poisoning under certain debug option, and + * won't break other sanity checks. + */ + if (kmem_cache_debug_flags(s, SLAB_STORE_USER) && + (s->flags & SLAB_KMALLOC)) + zero_size = orig_size; + /* * As memory initialization might be integrated into KASAN, * kasan_slab_alloc and initialization memset must be @@ -736,7 +750,7 @@ static inline void slab_post_alloc_hook(struct kmem_cache *s, for (i = 0; i < size; i++) { p[i] = kasan_slab_alloc(s, p[i], flags, init); if (p[i] && init && !kasan_has_integrated_init()) - memset(p[i], 0, s->object_size); + memset(p[i], 0, zero_size); kmemleak_alloc_recursive(p[i], s->object_size, 1, s->flags, flags); kmsan_slab_alloc(s, p[i], flags); diff --git a/mm/slub.c b/mm/slub.c index 12354fb8d6e4..17292c2d3eee 100644 --- a/mm/slub.c +++ b/mm/slub.c @@ -3395,7 +3395,11 @@ static __always_inline void *slab_alloc_node(struct kmem_cache *s, struct list_l init = slab_want_init_on_alloc(gfpflags, s); out: - slab_post_alloc_hook(s, objcg, gfpflags, 1, &object, init); + /* + * When init equals 'true', like for kzalloc() family, only + * @orig_size bytes will be zeroed instead of s->object_size + */ + slab_post_alloc_hook(s, objcg, gfpflags, 1, &object, init, orig_size); return object; } @@ -3852,11 +3856,11 @@ int kmem_cache_alloc_bulk(struct kmem_cache *s, gfp_t flags, size_t size, * Done outside of the IRQ disabled fastpath loop. */ slab_post_alloc_hook(s, objcg, flags, size, p, - slab_want_init_on_alloc(flags, s)); + slab_want_init_on_alloc(flags, s), s->object_size); return i; error: slub_put_cpu_ptr(s->cpu_slab); - slab_post_alloc_hook(s, objcg, flags, i, p, false); + slab_post_alloc_hook(s, objcg, flags, i, p, false, s->object_size); kmem_cache_free_bulk(s, i, p); return 0; } -- 2.34.1 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH v7 1/3] mm/slub: only zero requested size of buffer for kzalloc when debug enabled 2022-10-21 3:24 ` [PATCH v7 1/3] mm/slub: only zero requested size of buffer for kzalloc when debug enabled Feng Tang @ 2022-10-24 14:00 ` Hyeonggon Yoo 2022-10-27 19:27 ` Andrey Konovalov 2022-11-09 14:28 ` Vlastimil Babka 2 siblings, 0 replies; 20+ messages in thread From: Hyeonggon Yoo @ 2022-10-24 14:00 UTC (permalink / raw) To: Feng Tang Cc: Andrew Morton, Vlastimil Babka, Christoph Lameter, Pekka Enberg, David Rientjes, Joonsoo Kim, Roman Gushchin, Dmitry Vyukov, Andrey Konovalov, Kees Cook, Dave Hansen, linux-mm, linux-kernel, kasan-dev On Fri, Oct 21, 2022 at 11:24:03AM +0800, Feng Tang wrote: > kzalloc/kmalloc will round up the request size to a fixed size > (mostly power of 2), so the allocated memory could be more than > requested. Currently kzalloc family APIs will zero all the > allocated memory. > > To detect out-of-bound usage of the extra allocated memory, only > zero the requested part, so that redzone sanity check could be > added to the extra space later. > > For kzalloc users who will call ksize() later and utilize this > extra space, please be aware that the space is not zeroed any > more when debug is enabled. (Thanks to Kees Cook's effort to > sanitize all ksize() user cases [1], this won't be a big issue). > > [1]. https://lore.kernel.org/all/20220922031013.2150682-1-keescook@chromium.org/#r > Signed-off-by: Feng Tang <feng.tang@intel.com> > --- > mm/slab.c | 7 ++++--- > mm/slab.h | 18 ++++++++++++++++-- > mm/slub.c | 10 +++++++--- > 3 files changed, 27 insertions(+), 8 deletions(-) > > diff --git a/mm/slab.c b/mm/slab.c > index a5486ff8362a..4594de0e3d6b 100644 > --- a/mm/slab.c > +++ b/mm/slab.c > @@ -3253,7 +3253,8 @@ slab_alloc_node(struct kmem_cache *cachep, struct list_lru *lru, gfp_t flags, > init = slab_want_init_on_alloc(flags, cachep); > > out: > - slab_post_alloc_hook(cachep, objcg, flags, 1, &objp, init); > + slab_post_alloc_hook(cachep, objcg, flags, 1, &objp, init, > + cachep->object_size); > return objp; > } > > @@ -3506,13 +3507,13 @@ int kmem_cache_alloc_bulk(struct kmem_cache *s, gfp_t flags, size_t size, > * Done outside of the IRQ disabled section. > */ > slab_post_alloc_hook(s, objcg, flags, size, p, > - slab_want_init_on_alloc(flags, s)); > + slab_want_init_on_alloc(flags, s), s->object_size); > /* FIXME: Trace call missing. Christoph would like a bulk variant */ > return size; > error: > local_irq_enable(); > cache_alloc_debugcheck_after_bulk(s, flags, i, p, _RET_IP_); > - slab_post_alloc_hook(s, objcg, flags, i, p, false); > + slab_post_alloc_hook(s, objcg, flags, i, p, false, s->object_size); > kmem_cache_free_bulk(s, i, p); > return 0; > } > diff --git a/mm/slab.h b/mm/slab.h > index 0202a8c2f0d2..8b4ee02fc14a 100644 > --- a/mm/slab.h > +++ b/mm/slab.h > @@ -720,12 +720,26 @@ static inline struct kmem_cache *slab_pre_alloc_hook(struct kmem_cache *s, > > static inline void slab_post_alloc_hook(struct kmem_cache *s, > struct obj_cgroup *objcg, gfp_t flags, > - size_t size, void **p, bool init) > + size_t size, void **p, bool init, > + unsigned int orig_size) > { > + unsigned int zero_size = s->object_size; > size_t i; > > flags &= gfp_allowed_mask; > > + /* > + * For kmalloc object, the allocated memory size(object_size) is likely > + * larger than the requested size(orig_size). If redzone check is > + * enabled for the extra space, don't zero it, as it will be redzoned > + * soon. The redzone operation for this extra space could be seen as a > + * replacement of current poisoning under certain debug option, and > + * won't break other sanity checks. > + */ > + if (kmem_cache_debug_flags(s, SLAB_STORE_USER) && > + (s->flags & SLAB_KMALLOC)) > + zero_size = orig_size; > + > /* > * As memory initialization might be integrated into KASAN, > * kasan_slab_alloc and initialization memset must be > @@ -736,7 +750,7 @@ static inline void slab_post_alloc_hook(struct kmem_cache *s, > for (i = 0; i < size; i++) { > p[i] = kasan_slab_alloc(s, p[i], flags, init); > if (p[i] && init && !kasan_has_integrated_init()) > - memset(p[i], 0, s->object_size); > + memset(p[i], 0, zero_size); > kmemleak_alloc_recursive(p[i], s->object_size, 1, > s->flags, flags); > kmsan_slab_alloc(s, p[i], flags); > diff --git a/mm/slub.c b/mm/slub.c > index 12354fb8d6e4..17292c2d3eee 100644 > --- a/mm/slub.c > +++ b/mm/slub.c > @@ -3395,7 +3395,11 @@ static __always_inline void *slab_alloc_node(struct kmem_cache *s, struct list_l > init = slab_want_init_on_alloc(gfpflags, s); > > out: > - slab_post_alloc_hook(s, objcg, gfpflags, 1, &object, init); > + /* > + * When init equals 'true', like for kzalloc() family, only > + * @orig_size bytes will be zeroed instead of s->object_size > + */ > + slab_post_alloc_hook(s, objcg, gfpflags, 1, &object, init, orig_size); > > return object; > } > @@ -3852,11 +3856,11 @@ int kmem_cache_alloc_bulk(struct kmem_cache *s, gfp_t flags, size_t size, > * Done outside of the IRQ disabled fastpath loop. > */ > slab_post_alloc_hook(s, objcg, flags, size, p, > - slab_want_init_on_alloc(flags, s)); > + slab_want_init_on_alloc(flags, s), s->object_size); > return i; > error: > slub_put_cpu_ptr(s->cpu_slab); > - slab_post_alloc_hook(s, objcg, flags, i, p, false); > + slab_post_alloc_hook(s, objcg, flags, i, p, false, s->object_size); > kmem_cache_free_bulk(s, i, p); > return 0; > } > -- > 2.34.1 Looks good to me. Acked-by: Hyeonggon Yoo <42.hyeyoo@gmail.com> -- Thanks, Hyeonggon ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v7 1/3] mm/slub: only zero requested size of buffer for kzalloc when debug enabled 2022-10-21 3:24 ` [PATCH v7 1/3] mm/slub: only zero requested size of buffer for kzalloc when debug enabled Feng Tang 2022-10-24 14:00 ` Hyeonggon Yoo @ 2022-10-27 19:27 ` Andrey Konovalov 2022-11-09 14:28 ` Vlastimil Babka 2 siblings, 0 replies; 20+ messages in thread From: Andrey Konovalov @ 2022-10-27 19:27 UTC (permalink / raw) To: Feng Tang Cc: Andrew Morton, Vlastimil Babka, Christoph Lameter, Pekka Enberg, David Rientjes, Joonsoo Kim, Roman Gushchin, Hyeonggon Yoo, Dmitry Vyukov, Kees Cook, Dave Hansen, linux-mm, linux-kernel, kasan-dev On Fri, Oct 21, 2022 at 5:24 AM Feng Tang <feng.tang@intel.com> wrote: > > kzalloc/kmalloc will round up the request size to a fixed size > (mostly power of 2), so the allocated memory could be more than > requested. Currently kzalloc family APIs will zero all the > allocated memory. > > To detect out-of-bound usage of the extra allocated memory, only > zero the requested part, so that redzone sanity check could be > added to the extra space later. > > For kzalloc users who will call ksize() later and utilize this > extra space, please be aware that the space is not zeroed any > more when debug is enabled. (Thanks to Kees Cook's effort to > sanitize all ksize() user cases [1], this won't be a big issue). > > [1]. https://lore.kernel.org/all/20220922031013.2150682-1-keescook@chromium.org/#r > Signed-off-by: Feng Tang <feng.tang@intel.com> > --- > mm/slab.c | 7 ++++--- > mm/slab.h | 18 ++++++++++++++++-- > mm/slub.c | 10 +++++++--- > 3 files changed, 27 insertions(+), 8 deletions(-) > > diff --git a/mm/slab.c b/mm/slab.c > index a5486ff8362a..4594de0e3d6b 100644 > --- a/mm/slab.c > +++ b/mm/slab.c > @@ -3253,7 +3253,8 @@ slab_alloc_node(struct kmem_cache *cachep, struct list_lru *lru, gfp_t flags, > init = slab_want_init_on_alloc(flags, cachep); > > out: > - slab_post_alloc_hook(cachep, objcg, flags, 1, &objp, init); > + slab_post_alloc_hook(cachep, objcg, flags, 1, &objp, init, > + cachep->object_size); > return objp; > } > > @@ -3506,13 +3507,13 @@ int kmem_cache_alloc_bulk(struct kmem_cache *s, gfp_t flags, size_t size, > * Done outside of the IRQ disabled section. > */ > slab_post_alloc_hook(s, objcg, flags, size, p, > - slab_want_init_on_alloc(flags, s)); > + slab_want_init_on_alloc(flags, s), s->object_size); > /* FIXME: Trace call missing. Christoph would like a bulk variant */ > return size; > error: > local_irq_enable(); > cache_alloc_debugcheck_after_bulk(s, flags, i, p, _RET_IP_); > - slab_post_alloc_hook(s, objcg, flags, i, p, false); > + slab_post_alloc_hook(s, objcg, flags, i, p, false, s->object_size); > kmem_cache_free_bulk(s, i, p); > return 0; > } > diff --git a/mm/slab.h b/mm/slab.h > index 0202a8c2f0d2..8b4ee02fc14a 100644 > --- a/mm/slab.h > +++ b/mm/slab.h > @@ -720,12 +720,26 @@ static inline struct kmem_cache *slab_pre_alloc_hook(struct kmem_cache *s, > > static inline void slab_post_alloc_hook(struct kmem_cache *s, > struct obj_cgroup *objcg, gfp_t flags, > - size_t size, void **p, bool init) > + size_t size, void **p, bool init, > + unsigned int orig_size) > { > + unsigned int zero_size = s->object_size; > size_t i; > > flags &= gfp_allowed_mask; > > + /* > + * For kmalloc object, the allocated memory size(object_size) is likely > + * larger than the requested size(orig_size). If redzone check is > + * enabled for the extra space, don't zero it, as it will be redzoned > + * soon. The redzone operation for this extra space could be seen as a > + * replacement of current poisoning under certain debug option, and > + * won't break other sanity checks. > + */ > + if (kmem_cache_debug_flags(s, SLAB_STORE_USER) && > + (s->flags & SLAB_KMALLOC)) > + zero_size = orig_size; > + > /* > * As memory initialization might be integrated into KASAN, > * kasan_slab_alloc and initialization memset must be > @@ -736,7 +750,7 @@ static inline void slab_post_alloc_hook(struct kmem_cache *s, > for (i = 0; i < size; i++) { > p[i] = kasan_slab_alloc(s, p[i], flags, init); > if (p[i] && init && !kasan_has_integrated_init()) > - memset(p[i], 0, s->object_size); > + memset(p[i], 0, zero_size); > kmemleak_alloc_recursive(p[i], s->object_size, 1, > s->flags, flags); > kmsan_slab_alloc(s, p[i], flags); > diff --git a/mm/slub.c b/mm/slub.c > index 12354fb8d6e4..17292c2d3eee 100644 > --- a/mm/slub.c > +++ b/mm/slub.c > @@ -3395,7 +3395,11 @@ static __always_inline void *slab_alloc_node(struct kmem_cache *s, struct list_l > init = slab_want_init_on_alloc(gfpflags, s); > > out: > - slab_post_alloc_hook(s, objcg, gfpflags, 1, &object, init); > + /* > + * When init equals 'true', like for kzalloc() family, only > + * @orig_size bytes will be zeroed instead of s->object_size > + */ > + slab_post_alloc_hook(s, objcg, gfpflags, 1, &object, init, orig_size); > > return object; > } > @@ -3852,11 +3856,11 @@ int kmem_cache_alloc_bulk(struct kmem_cache *s, gfp_t flags, size_t size, > * Done outside of the IRQ disabled fastpath loop. > */ > slab_post_alloc_hook(s, objcg, flags, size, p, > - slab_want_init_on_alloc(flags, s)); > + slab_want_init_on_alloc(flags, s), s->object_size); > return i; > error: > slub_put_cpu_ptr(s->cpu_slab); > - slab_post_alloc_hook(s, objcg, flags, i, p, false); > + slab_post_alloc_hook(s, objcg, flags, i, p, false, s->object_size); > kmem_cache_free_bulk(s, i, p); > return 0; > } > -- > 2.34.1 > For the KASAN part: Reviewed-by: Andrey Konovalov <andreyknvl@gmail.com> Thanks! ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v7 1/3] mm/slub: only zero requested size of buffer for kzalloc when debug enabled 2022-10-21 3:24 ` [PATCH v7 1/3] mm/slub: only zero requested size of buffer for kzalloc when debug enabled Feng Tang 2022-10-24 14:00 ` Hyeonggon Yoo 2022-10-27 19:27 ` Andrey Konovalov @ 2022-11-09 14:28 ` Vlastimil Babka 2022-11-10 3:20 ` Feng Tang 2 siblings, 1 reply; 20+ messages in thread From: Vlastimil Babka @ 2022-11-09 14:28 UTC (permalink / raw) To: Feng Tang, Andrew Morton, Christoph Lameter, Pekka Enberg, David Rientjes, Joonsoo Kim, Roman Gushchin, Hyeonggon Yoo, Dmitry Vyukov, Andrey Konovalov, Kees Cook Cc: Dave Hansen, linux-mm, linux-kernel, kasan-dev On 10/21/22 05:24, Feng Tang wrote: > kzalloc/kmalloc will round up the request size to a fixed size > (mostly power of 2), so the allocated memory could be more than > requested. Currently kzalloc family APIs will zero all the > allocated memory. > > To detect out-of-bound usage of the extra allocated memory, only > zero the requested part, so that redzone sanity check could be > added to the extra space later. > > For kzalloc users who will call ksize() later and utilize this > extra space, please be aware that the space is not zeroed any > more when debug is enabled. (Thanks to Kees Cook's effort to > sanitize all ksize() user cases [1], this won't be a big issue). > > [1]. https://lore.kernel.org/all/20220922031013.2150682-1-keescook@chromium.org/#r > Signed-off-by: Feng Tang <feng.tang@intel.com> > --- > mm/slab.c | 7 ++++--- > mm/slab.h | 18 ++++++++++++++++-- > mm/slub.c | 10 +++++++--- > 3 files changed, 27 insertions(+), 8 deletions(-) > > diff --git a/mm/slab.c b/mm/slab.c > index a5486ff8362a..4594de0e3d6b 100644 > --- a/mm/slab.c > +++ b/mm/slab.c > @@ -3253,7 +3253,8 @@ slab_alloc_node(struct kmem_cache *cachep, struct list_lru *lru, gfp_t flags, > init = slab_want_init_on_alloc(flags, cachep); > > out: > - slab_post_alloc_hook(cachep, objcg, flags, 1, &objp, init); > + slab_post_alloc_hook(cachep, objcg, flags, 1, &objp, init, > + cachep->object_size); > return objp; > } > > @@ -3506,13 +3507,13 @@ int kmem_cache_alloc_bulk(struct kmem_cache *s, gfp_t flags, size_t size, > * Done outside of the IRQ disabled section. > */ > slab_post_alloc_hook(s, objcg, flags, size, p, > - slab_want_init_on_alloc(flags, s)); > + slab_want_init_on_alloc(flags, s), s->object_size); > /* FIXME: Trace call missing. Christoph would like a bulk variant */ > return size; > error: > local_irq_enable(); > cache_alloc_debugcheck_after_bulk(s, flags, i, p, _RET_IP_); > - slab_post_alloc_hook(s, objcg, flags, i, p, false); > + slab_post_alloc_hook(s, objcg, flags, i, p, false, s->object_size); > kmem_cache_free_bulk(s, i, p); > return 0; > } > diff --git a/mm/slab.h b/mm/slab.h > index 0202a8c2f0d2..8b4ee02fc14a 100644 > --- a/mm/slab.h > +++ b/mm/slab.h > @@ -720,12 +720,26 @@ static inline struct kmem_cache *slab_pre_alloc_hook(struct kmem_cache *s, > > static inline void slab_post_alloc_hook(struct kmem_cache *s, > struct obj_cgroup *objcg, gfp_t flags, > - size_t size, void **p, bool init) > + size_t size, void **p, bool init, > + unsigned int orig_size) > { > + unsigned int zero_size = s->object_size; > size_t i; > > flags &= gfp_allowed_mask; > > + /* > + * For kmalloc object, the allocated memory size(object_size) is likely > + * larger than the requested size(orig_size). If redzone check is > + * enabled for the extra space, don't zero it, as it will be redzoned > + * soon. The redzone operation for this extra space could be seen as a > + * replacement of current poisoning under certain debug option, and > + * won't break other sanity checks. > + */ > + if (kmem_cache_debug_flags(s, SLAB_STORE_USER) && Shouldn't we check SLAB_RED_ZONE instead? Otherwise a debugging could be specified so that SLAB_RED_ZONE is set but SLAB_STORE_USER? > + (s->flags & SLAB_KMALLOC)) > + zero_size = orig_size; > + > /* > * As memory initialization might be integrated into KASAN, > * kasan_slab_alloc and initialization memset must be > @@ -736,7 +750,7 @@ static inline void slab_post_alloc_hook(struct kmem_cache *s, > for (i = 0; i < size; i++) { > p[i] = kasan_slab_alloc(s, p[i], flags, init); > if (p[i] && init && !kasan_has_integrated_init()) > - memset(p[i], 0, s->object_size); > + memset(p[i], 0, zero_size); > kmemleak_alloc_recursive(p[i], s->object_size, 1, > s->flags, flags); > kmsan_slab_alloc(s, p[i], flags); > diff --git a/mm/slub.c b/mm/slub.c > index 12354fb8d6e4..17292c2d3eee 100644 > --- a/mm/slub.c > +++ b/mm/slub.c > @@ -3395,7 +3395,11 @@ static __always_inline void *slab_alloc_node(struct kmem_cache *s, struct list_l > init = slab_want_init_on_alloc(gfpflags, s); > > out: > - slab_post_alloc_hook(s, objcg, gfpflags, 1, &object, init); > + /* > + * When init equals 'true', like for kzalloc() family, only > + * @orig_size bytes will be zeroed instead of s->object_size s/will be/might be/ because it depends on the debugging? > + */ > + slab_post_alloc_hook(s, objcg, gfpflags, 1, &object, init, orig_size); > > return object; > } > @@ -3852,11 +3856,11 @@ int kmem_cache_alloc_bulk(struct kmem_cache *s, gfp_t flags, size_t size, > * Done outside of the IRQ disabled fastpath loop. > */ > slab_post_alloc_hook(s, objcg, flags, size, p, > - slab_want_init_on_alloc(flags, s)); > + slab_want_init_on_alloc(flags, s), s->object_size); > return i; > error: > slub_put_cpu_ptr(s->cpu_slab); > - slab_post_alloc_hook(s, objcg, flags, i, p, false); > + slab_post_alloc_hook(s, objcg, flags, i, p, false, s->object_size); > kmem_cache_free_bulk(s, i, p); > return 0; > } ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v7 1/3] mm/slub: only zero requested size of buffer for kzalloc when debug enabled 2022-11-09 14:28 ` Vlastimil Babka @ 2022-11-10 3:20 ` Feng Tang 2022-11-10 12:57 ` Feng Tang 0 siblings, 1 reply; 20+ messages in thread From: Feng Tang @ 2022-11-10 3:20 UTC (permalink / raw) To: Vlastimil Babka Cc: Andrew Morton, Christoph Lameter, Pekka Enberg, David Rientjes, Joonsoo Kim, Roman Gushchin, Hyeonggon Yoo, Dmitry Vyukov, Andrey Konovalov, Kees Cook, Dave Hansen, linux-mm, linux-kernel, kasan-dev On Wed, Nov 09, 2022 at 03:28:19PM +0100, Vlastimil Babka wrote: > On 10/21/22 05:24, Feng Tang wrote: > > kzalloc/kmalloc will round up the request size to a fixed size > > (mostly power of 2), so the allocated memory could be more than > > requested. Currently kzalloc family APIs will zero all the > > allocated memory. > > > > To detect out-of-bound usage of the extra allocated memory, only > > zero the requested part, so that redzone sanity check could be > > added to the extra space later. > > > > For kzalloc users who will call ksize() later and utilize this > > extra space, please be aware that the space is not zeroed any > > more when debug is enabled. (Thanks to Kees Cook's effort to > > sanitize all ksize() user cases [1], this won't be a big issue). > > > > [1]. https://lore.kernel.org/all/20220922031013.2150682-1-keescook@chromium.org/#r > > Signed-off-by: Feng Tang <feng.tang@intel.com> > > --- [...] > > static inline void slab_post_alloc_hook(struct kmem_cache *s, > > struct obj_cgroup *objcg, gfp_t flags, > > - size_t size, void **p, bool init) > > + size_t size, void **p, bool init, > > + unsigned int orig_size) > > { > > + unsigned int zero_size = s->object_size; > > size_t i; > > > > flags &= gfp_allowed_mask; > > > > + /* > > + * For kmalloc object, the allocated memory size(object_size) is likely > > + * larger than the requested size(orig_size). If redzone check is > > + * enabled for the extra space, don't zero it, as it will be redzoned > > + * soon. The redzone operation for this extra space could be seen as a > > + * replacement of current poisoning under certain debug option, and > > + * won't break other sanity checks. > > + */ > > + if (kmem_cache_debug_flags(s, SLAB_STORE_USER) && > > Shouldn't we check SLAB_RED_ZONE instead? Otherwise a debugging could be > specified so that SLAB_RED_ZONE is set but SLAB_STORE_USER? Thanks for the catch! I will add check for SLAB_RED_ZONE. The SLAB_STORE_USER is for checking whether 'orig_size' field exists. In earlier discussion, we make 'orig_size' depend on STORE_USER, https://lore.kernel.org/lkml/1b0fa66c-f855-1c00-e024-b2b823b18678@suse.cz/ > > + (s->flags & SLAB_KMALLOC)) > > + zero_size = orig_size; > > + > > /* > > * As memory initialization might be integrated into KASAN, > > * kasan_slab_alloc and initialization memset must be > > @@ -736,7 +750,7 @@ static inline void slab_post_alloc_hook(struct kmem_cache *s, > > for (i = 0; i < size; i++) { > > p[i] = kasan_slab_alloc(s, p[i], flags, init); > > if (p[i] && init && !kasan_has_integrated_init()) > > - memset(p[i], 0, s->object_size); > > + memset(p[i], 0, zero_size); > > kmemleak_alloc_recursive(p[i], s->object_size, 1, > > s->flags, flags); > > kmsan_slab_alloc(s, p[i], flags); > > diff --git a/mm/slub.c b/mm/slub.c > > index 12354fb8d6e4..17292c2d3eee 100644 > > --- a/mm/slub.c > > +++ b/mm/slub.c > > @@ -3395,7 +3395,11 @@ static __always_inline void *slab_alloc_node(struct kmem_cache *s, struct list_l > > init = slab_want_init_on_alloc(gfpflags, s); > > > > out: > > - slab_post_alloc_hook(s, objcg, gfpflags, 1, &object, init); > > + /* > > + * When init equals 'true', like for kzalloc() family, only > > + * @orig_size bytes will be zeroed instead of s->object_size > > s/will be/might be/ because it depends on the debugging? Yes, will change. Thanks, Feng ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v7 1/3] mm/slub: only zero requested size of buffer for kzalloc when debug enabled 2022-11-10 3:20 ` Feng Tang @ 2022-11-10 12:57 ` Feng Tang 2022-11-10 15:44 ` Vlastimil Babka 0 siblings, 1 reply; 20+ messages in thread From: Feng Tang @ 2022-11-10 12:57 UTC (permalink / raw) To: Vlastimil Babka Cc: Andrew Morton, Christoph Lameter, Pekka Enberg, David Rientjes, Joonsoo Kim, Roman Gushchin, Hyeonggon Yoo, Dmitry Vyukov, Andrey Konovalov, Kees Cook, Hansen, Dave, linux-mm, linux-kernel, kasan-dev On Thu, Nov 10, 2022 at 11:20:34AM +0800, Tang, Feng wrote: > On Wed, Nov 09, 2022 at 03:28:19PM +0100, Vlastimil Babka wrote: [...] > > > + /* > > > + * For kmalloc object, the allocated memory size(object_size) is likely > > > + * larger than the requested size(orig_size). If redzone check is > > > + * enabled for the extra space, don't zero it, as it will be redzoned > > > + * soon. The redzone operation for this extra space could be seen as a > > > + * replacement of current poisoning under certain debug option, and > > > + * won't break other sanity checks. > > > + */ > > > + if (kmem_cache_debug_flags(s, SLAB_STORE_USER) && > > > > Shouldn't we check SLAB_RED_ZONE instead? Otherwise a debugging could be > > specified so that SLAB_RED_ZONE is set but SLAB_STORE_USER? > > Thanks for the catch! > > I will add check for SLAB_RED_ZONE. The SLAB_STORE_USER is for > checking whether 'orig_size' field exists. In earlier discussion, > we make 'orig_size' depend on STORE_USER, https://lore.kernel.org/lkml/1b0fa66c-f855-1c00-e024-b2b823b18678@suse.cz/ Below is the updated patch, please review, thanks! - Feng -----8>---- From b2a92f0c2518ef80fcda340f1ad37b418ee32d85 Mon Sep 17 00:00:00 2001 From: Feng Tang <feng.tang@intel.com> Date: Thu, 20 Oct 2022 20:47:31 +0800 Subject: [PATCH 1/3] mm/slub: only zero requested size of buffer for kzalloc when debug enabled kzalloc/kmalloc will round up the request size to a fixed size (mostly power of 2), so the allocated memory could be more than requested. Currently kzalloc family APIs will zero all the allocated memory. To detect out-of-bound usage of the extra allocated memory, only zero the requested part, so that redzone sanity check could be added to the extra space later. For kzalloc users who will call ksize() later and utilize this extra space, please be aware that the space is not zeroed any more when debug is enabled. (Thanks to Kees Cook's effort to sanitize all ksize() user cases [1], this won't be a big issue). [1]. https://lore.kernel.org/all/20220922031013.2150682-1-keescook@chromium.org/#r Signed-off-by: Feng Tang <feng.tang@intel.com> Acked-by: Hyeonggon Yoo <42.hyeyoo@gmail.com> --- mm/slab.c | 7 ++++--- mm/slab.h | 19 +++++++++++++++++-- mm/slub.c | 10 +++++++--- 3 files changed, 28 insertions(+), 8 deletions(-) diff --git a/mm/slab.c b/mm/slab.c index 4b265174b6d5..1eddec4a50e4 100644 --- a/mm/slab.c +++ b/mm/slab.c @@ -3258,7 +3258,8 @@ slab_alloc_node(struct kmem_cache *cachep, struct list_lru *lru, gfp_t flags, init = slab_want_init_on_alloc(flags, cachep); out: - slab_post_alloc_hook(cachep, objcg, flags, 1, &objp, init); + slab_post_alloc_hook(cachep, objcg, flags, 1, &objp, init, + cachep->object_size); return objp; } @@ -3511,13 +3512,13 @@ int kmem_cache_alloc_bulk(struct kmem_cache *s, gfp_t flags, size_t size, * Done outside of the IRQ disabled section. */ slab_post_alloc_hook(s, objcg, flags, size, p, - slab_want_init_on_alloc(flags, s)); + slab_want_init_on_alloc(flags, s), s->object_size); /* FIXME: Trace call missing. Christoph would like a bulk variant */ return size; error: local_irq_enable(); cache_alloc_debugcheck_after_bulk(s, flags, i, p, _RET_IP_); - slab_post_alloc_hook(s, objcg, flags, i, p, false); + slab_post_alloc_hook(s, objcg, flags, i, p, false, s->object_size); kmem_cache_free_bulk(s, i, p); return 0; } diff --git a/mm/slab.h b/mm/slab.h index 8c4aafb00bd6..2551214392c7 100644 --- a/mm/slab.h +++ b/mm/slab.h @@ -730,12 +730,27 @@ static inline struct kmem_cache *slab_pre_alloc_hook(struct kmem_cache *s, static inline void slab_post_alloc_hook(struct kmem_cache *s, struct obj_cgroup *objcg, gfp_t flags, - size_t size, void **p, bool init) + size_t size, void **p, bool init, + unsigned int orig_size) { + unsigned int zero_size = s->object_size; size_t i; flags &= gfp_allowed_mask; + /* + * For kmalloc object, the allocated memory size(object_size) is likely + * larger than the requested size(orig_size). If redzone check is + * enabled for the extra space, don't zero it, as it will be redzoned + * soon. The redzone operation for this extra space could be seen as a + * replacement of current poisoning under certain debug option, and + * won't break other sanity checks. + */ + if (kmem_cache_debug_flags(s, SLAB_STORE_USER) && + (s->flags & SLAB_RED_ZONE) && + (s->flags & SLAB_KMALLOC)) + zero_size = orig_size; + /* * As memory initialization might be integrated into KASAN, * kasan_slab_alloc and initialization memset must be @@ -746,7 +761,7 @@ static inline void slab_post_alloc_hook(struct kmem_cache *s, for (i = 0; i < size; i++) { p[i] = kasan_slab_alloc(s, p[i], flags, init); if (p[i] && init && !kasan_has_integrated_init()) - memset(p[i], 0, s->object_size); + memset(p[i], 0, zero_size); kmemleak_alloc_recursive(p[i], s->object_size, 1, s->flags, flags); kmsan_slab_alloc(s, p[i], flags); diff --git a/mm/slub.c b/mm/slub.c index 0a14e7bc278c..13490f317f5f 100644 --- a/mm/slub.c +++ b/mm/slub.c @@ -3387,7 +3387,11 @@ static __always_inline void *slab_alloc_node(struct kmem_cache *s, struct list_l init = slab_want_init_on_alloc(gfpflags, s); out: - slab_post_alloc_hook(s, objcg, gfpflags, 1, &object, init); + /* + * When init equals 'true', like for kzalloc() family, only + * @orig_size bytes might be zeroed instead of s->object_size + */ + slab_post_alloc_hook(s, objcg, gfpflags, 1, &object, init, orig_size); return object; } @@ -3844,11 +3848,11 @@ int kmem_cache_alloc_bulk(struct kmem_cache *s, gfp_t flags, size_t size, * Done outside of the IRQ disabled fastpath loop. */ slab_post_alloc_hook(s, objcg, flags, size, p, - slab_want_init_on_alloc(flags, s)); + slab_want_init_on_alloc(flags, s), s->object_size); return i; error: slub_put_cpu_ptr(s->cpu_slab); - slab_post_alloc_hook(s, objcg, flags, i, p, false); + slab_post_alloc_hook(s, objcg, flags, i, p, false, s->object_size); kmem_cache_free_bulk(s, i, p); return 0; } -- 2.34.1 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH v7 1/3] mm/slub: only zero requested size of buffer for kzalloc when debug enabled 2022-11-10 12:57 ` Feng Tang @ 2022-11-10 15:44 ` Vlastimil Babka 2022-11-11 6:19 ` Feng Tang 0 siblings, 1 reply; 20+ messages in thread From: Vlastimil Babka @ 2022-11-10 15:44 UTC (permalink / raw) To: Feng Tang Cc: Andrew Morton, Christoph Lameter, Pekka Enberg, David Rientjes, Joonsoo Kim, Roman Gushchin, Hyeonggon Yoo, Dmitry Vyukov, Andrey Konovalov, Kees Cook, Hansen, Dave, linux-mm, linux-kernel, kasan-dev On 11/10/22 13:57, Feng Tang wrote: > On Thu, Nov 10, 2022 at 11:20:34AM +0800, Tang, Feng wrote: >> On Wed, Nov 09, 2022 at 03:28:19PM +0100, Vlastimil Babka wrote: > [...] >> > > + /* >> > > + * For kmalloc object, the allocated memory size(object_size) is likely >> > > + * larger than the requested size(orig_size). If redzone check is >> > > + * enabled for the extra space, don't zero it, as it will be redzoned >> > > + * soon. The redzone operation for this extra space could be seen as a >> > > + * replacement of current poisoning under certain debug option, and >> > > + * won't break other sanity checks. >> > > + */ >> > > + if (kmem_cache_debug_flags(s, SLAB_STORE_USER) && >> > >> > Shouldn't we check SLAB_RED_ZONE instead? Otherwise a debugging could be >> > specified so that SLAB_RED_ZONE is set but SLAB_STORE_USER? >> >> Thanks for the catch! >> >> I will add check for SLAB_RED_ZONE. The SLAB_STORE_USER is for >> checking whether 'orig_size' field exists. In earlier discussion, >> we make 'orig_size' depend on STORE_USER, https://lore.kernel.org/lkml/1b0fa66c-f855-1c00-e024-b2b823b18678@suse.cz/ > > Below is the updated patch, please review, thanks! Thanks, grabbing it including Andrey's review, with a small change below: > - Feng > > -----8>---- > From b2a92f0c2518ef80fcda340f1ad37b418ee32d85 Mon Sep 17 00:00:00 2001 > From: Feng Tang <feng.tang@intel.com> > Date: Thu, 20 Oct 2022 20:47:31 +0800 > Subject: [PATCH 1/3] mm/slub: only zero requested size of buffer for kzalloc > when debug enabled > > kzalloc/kmalloc will round up the request size to a fixed size > (mostly power of 2), so the allocated memory could be more than > requested. Currently kzalloc family APIs will zero all the > allocated memory. > > To detect out-of-bound usage of the extra allocated memory, only > zero the requested part, so that redzone sanity check could be > added to the extra space later. > > For kzalloc users who will call ksize() later and utilize this > extra space, please be aware that the space is not zeroed any > more when debug is enabled. (Thanks to Kees Cook's effort to > sanitize all ksize() user cases [1], this won't be a big issue). > > [1]. https://lore.kernel.org/all/20220922031013.2150682-1-keescook@chromium.org/#r > Signed-off-by: Feng Tang <feng.tang@intel.com> > Acked-by: Hyeonggon Yoo <42.hyeyoo@gmail.com> > --- > mm/slab.c | 7 ++++--- > mm/slab.h | 19 +++++++++++++++++-- > mm/slub.c | 10 +++++++--- > 3 files changed, 28 insertions(+), 8 deletions(-) > > diff --git a/mm/slab.c b/mm/slab.c > index 4b265174b6d5..1eddec4a50e4 100644 > --- a/mm/slab.c > +++ b/mm/slab.c > @@ -3258,7 +3258,8 @@ slab_alloc_node(struct kmem_cache *cachep, struct list_lru *lru, gfp_t flags, > init = slab_want_init_on_alloc(flags, cachep); > > out: > - slab_post_alloc_hook(cachep, objcg, flags, 1, &objp, init); > + slab_post_alloc_hook(cachep, objcg, flags, 1, &objp, init, > + cachep->object_size); > return objp; > } > > @@ -3511,13 +3512,13 @@ int kmem_cache_alloc_bulk(struct kmem_cache *s, gfp_t flags, size_t size, > * Done outside of the IRQ disabled section. > */ > slab_post_alloc_hook(s, objcg, flags, size, p, > - slab_want_init_on_alloc(flags, s)); > + slab_want_init_on_alloc(flags, s), s->object_size); > /* FIXME: Trace call missing. Christoph would like a bulk variant */ > return size; > error: > local_irq_enable(); > cache_alloc_debugcheck_after_bulk(s, flags, i, p, _RET_IP_); > - slab_post_alloc_hook(s, objcg, flags, i, p, false); > + slab_post_alloc_hook(s, objcg, flags, i, p, false, s->object_size); > kmem_cache_free_bulk(s, i, p); > return 0; > } > diff --git a/mm/slab.h b/mm/slab.h > index 8c4aafb00bd6..2551214392c7 100644 > --- a/mm/slab.h > +++ b/mm/slab.h > @@ -730,12 +730,27 @@ static inline struct kmem_cache *slab_pre_alloc_hook(struct kmem_cache *s, > > static inline void slab_post_alloc_hook(struct kmem_cache *s, > struct obj_cgroup *objcg, gfp_t flags, > - size_t size, void **p, bool init) > + size_t size, void **p, bool init, > + unsigned int orig_size) > { > + unsigned int zero_size = s->object_size; > size_t i; > > flags &= gfp_allowed_mask; > > + /* > + * For kmalloc object, the allocated memory size(object_size) is likely > + * larger than the requested size(orig_size). If redzone check is > + * enabled for the extra space, don't zero it, as it will be redzoned > + * soon. The redzone operation for this extra space could be seen as a > + * replacement of current poisoning under certain debug option, and > + * won't break other sanity checks. > + */ > + if (kmem_cache_debug_flags(s, SLAB_STORE_USER) && > + (s->flags & SLAB_RED_ZONE) && Combined the two above to: if (kmem_cache_debug_flags(s, SLAB_STORE_USER | SLAB_RED_ZONE) > + (s->flags & SLAB_KMALLOC)) > + zero_size = orig_size; > + > /* > * As memory initialization might be integrated into KASAN, > * kasan_slab_alloc and initialization memset must be > @@ -746,7 +761,7 @@ static inline void slab_post_alloc_hook(struct kmem_cache *s, > for (i = 0; i < size; i++) { > p[i] = kasan_slab_alloc(s, p[i], flags, init); > if (p[i] && init && !kasan_has_integrated_init()) > - memset(p[i], 0, s->object_size); > + memset(p[i], 0, zero_size); > kmemleak_alloc_recursive(p[i], s->object_size, 1, > s->flags, flags); > kmsan_slab_alloc(s, p[i], flags); > diff --git a/mm/slub.c b/mm/slub.c > index 0a14e7bc278c..13490f317f5f 100644 > --- a/mm/slub.c > +++ b/mm/slub.c > @@ -3387,7 +3387,11 @@ static __always_inline void *slab_alloc_node(struct kmem_cache *s, struct list_l > init = slab_want_init_on_alloc(gfpflags, s); > > out: > - slab_post_alloc_hook(s, objcg, gfpflags, 1, &object, init); > + /* > + * When init equals 'true', like for kzalloc() family, only > + * @orig_size bytes might be zeroed instead of s->object_size > + */ > + slab_post_alloc_hook(s, objcg, gfpflags, 1, &object, init, orig_size); > > return object; > } > @@ -3844,11 +3848,11 @@ int kmem_cache_alloc_bulk(struct kmem_cache *s, gfp_t flags, size_t size, > * Done outside of the IRQ disabled fastpath loop. > */ > slab_post_alloc_hook(s, objcg, flags, size, p, > - slab_want_init_on_alloc(flags, s)); > + slab_want_init_on_alloc(flags, s), s->object_size); > return i; > error: > slub_put_cpu_ptr(s->cpu_slab); > - slab_post_alloc_hook(s, objcg, flags, i, p, false); > + slab_post_alloc_hook(s, objcg, flags, i, p, false, s->object_size); > kmem_cache_free_bulk(s, i, p); > return 0; > } ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v7 1/3] mm/slub: only zero requested size of buffer for kzalloc when debug enabled 2022-11-10 15:44 ` Vlastimil Babka @ 2022-11-11 6:19 ` Feng Tang 0 siblings, 0 replies; 20+ messages in thread From: Feng Tang @ 2022-11-11 6:19 UTC (permalink / raw) To: Vlastimil Babka Cc: Andrew Morton, Christoph Lameter, Pekka Enberg, David Rientjes, Joonsoo Kim, Roman Gushchin, Hyeonggon Yoo, Dmitry Vyukov, Andrey Konovalov, Kees Cook, Hansen, Dave, linux-mm, linux-kernel, kasan-dev On Thu, Nov 10, 2022 at 04:44:59PM +0100, Vlastimil Babka wrote: > On 11/10/22 13:57, Feng Tang wrote: > > On Thu, Nov 10, 2022 at 11:20:34AM +0800, Tang, Feng wrote: > >> On Wed, Nov 09, 2022 at 03:28:19PM +0100, Vlastimil Babka wrote: > > [...] > >> > > + /* > >> > > + * For kmalloc object, the allocated memory size(object_size) is likely > >> > > + * larger than the requested size(orig_size). If redzone check is > >> > > + * enabled for the extra space, don't zero it, as it will be redzoned > >> > > + * soon. The redzone operation for this extra space could be seen as a > >> > > + * replacement of current poisoning under certain debug option, and > >> > > + * won't break other sanity checks. > >> > > + */ > >> > > + if (kmem_cache_debug_flags(s, SLAB_STORE_USER) && > >> > > >> > Shouldn't we check SLAB_RED_ZONE instead? Otherwise a debugging could be > >> > specified so that SLAB_RED_ZONE is set but SLAB_STORE_USER? > >> > >> Thanks for the catch! > >> > >> I will add check for SLAB_RED_ZONE. The SLAB_STORE_USER is for > >> checking whether 'orig_size' field exists. In earlier discussion, > >> we make 'orig_size' depend on STORE_USER, https://lore.kernel.org/lkml/1b0fa66c-f855-1c00-e024-b2b823b18678@suse.cz/ > > > > Below is the updated patch, please review, thanks! > > Thanks, grabbing it including Andrey's review, with a small change below: > > > - Feng > > > > -----8>---- > > From b2a92f0c2518ef80fcda340f1ad37b418ee32d85 Mon Sep 17 00:00:00 2001 > > From: Feng Tang <feng.tang@intel.com> > > Date: Thu, 20 Oct 2022 20:47:31 +0800 > > Subject: [PATCH 1/3] mm/slub: only zero requested size of buffer for kzalloc > > when debug enabled [...] > > + /* > > + * For kmalloc object, the allocated memory size(object_size) is likely > > + * larger than the requested size(orig_size). If redzone check is > > + * enabled for the extra space, don't zero it, as it will be redzoned > > + * soon. The redzone operation for this extra space could be seen as a > > + * replacement of current poisoning under certain debug option, and > > + * won't break other sanity checks. > > + */ > > + if (kmem_cache_debug_flags(s, SLAB_STORE_USER) && > > + (s->flags & SLAB_RED_ZONE) && > > Combined the two above to: > > if (kmem_cache_debug_flags(s, SLAB_STORE_USER | SLAB_RED_ZONE) Yes, this is cleaner, thanks! - Feng ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v7 2/3] mm: kasan: Extend kasan_metadata_size() to also cover in-object size 2022-10-21 3:24 [PATCH v7 0/3] mm/slub: extend redzone check for kmalloc objects Feng Tang 2022-10-21 3:24 ` [PATCH v7 1/3] mm/slub: only zero requested size of buffer for kzalloc when debug enabled Feng Tang @ 2022-10-21 3:24 ` Feng Tang 2022-10-27 19:27 ` Andrey Konovalov 2022-10-21 3:24 ` [PATCH v7 3/3] mm/slub: extend redzone check to extra allocated kmalloc space than requested Feng Tang 2022-11-11 8:16 ` [PATCH v7 0/3] mm/slub: extend redzone check for kmalloc objects Vlastimil Babka 3 siblings, 1 reply; 20+ messages in thread From: Feng Tang @ 2022-10-21 3:24 UTC (permalink / raw) To: Andrew Morton, Vlastimil Babka, Christoph Lameter, Pekka Enberg, David Rientjes, Joonsoo Kim, Roman Gushchin, Hyeonggon Yoo, Dmitry Vyukov, Andrey Konovalov, Kees Cook Cc: Dave Hansen, linux-mm, linux-kernel, kasan-dev, Feng Tang, kernel test robot, Andrey Ryabinin, Alexander Potapenko, Vincenzo Frascino When kasan is enabled for slab/slub, it may save kasan' free_meta data in the former part of slab object data area in slab object's free path, which works fine. There is ongoing effort to extend slub's debug function which will redzone the latter part of kmalloc object area, and when both of the debug are enabled, there is possible conflict, especially when the kmalloc object has small size, as caught by 0Day bot [1]. To solve it, slub code needs to know the in-object kasan's meta data size. Currently, there is existing kasan_metadata_size() which returns the kasan's metadata size inside slub's metadata area, so extend it to also cover the in-object meta size by adding a boolean flag 'in_object'. There is no functional change to existing code logic. [1]. https://lore.kernel.org/lkml/YuYm3dWwpZwH58Hu@xsang-OptiPlex-9020/ Reported-by: kernel test robot <oliver.sang@intel.com> Suggested-by: Andrey Konovalov <andreyknvl@gmail.com> Signed-off-by: Feng Tang <feng.tang@intel.com> Reviewed-by: Andrey Konovalov <andreyknvl@gmail.com> Cc: Andrey Ryabinin <ryabinin.a.a@gmail.com> Cc: Alexander Potapenko <glider@google.com> Cc: Dmitry Vyukov <dvyukov@google.com> Cc: Vincenzo Frascino <vincenzo.frascino@arm.com> --- include/linux/kasan.h | 5 +++-- mm/kasan/generic.c | 19 +++++++++++++------ mm/slub.c | 4 ++-- 3 files changed, 18 insertions(+), 10 deletions(-) diff --git a/include/linux/kasan.h b/include/linux/kasan.h index d811b3d7d2a1..96c9d56e5510 100644 --- a/include/linux/kasan.h +++ b/include/linux/kasan.h @@ -302,7 +302,7 @@ static inline void kasan_unpoison_task_stack(struct task_struct *task) {} #ifdef CONFIG_KASAN_GENERIC -size_t kasan_metadata_size(struct kmem_cache *cache); +size_t kasan_metadata_size(struct kmem_cache *cache, bool in_object); slab_flags_t kasan_never_merge(void); void kasan_cache_create(struct kmem_cache *cache, unsigned int *size, slab_flags_t *flags); @@ -315,7 +315,8 @@ void kasan_record_aux_stack_noalloc(void *ptr); #else /* CONFIG_KASAN_GENERIC */ /* Tag-based KASAN modes do not use per-object metadata. */ -static inline size_t kasan_metadata_size(struct kmem_cache *cache) +static inline size_t kasan_metadata_size(struct kmem_cache *cache, + bool in_object) { return 0; } diff --git a/mm/kasan/generic.c b/mm/kasan/generic.c index d8b5590f9484..b076f597a378 100644 --- a/mm/kasan/generic.c +++ b/mm/kasan/generic.c @@ -450,15 +450,22 @@ void kasan_init_object_meta(struct kmem_cache *cache, const void *object) __memset(alloc_meta, 0, sizeof(*alloc_meta)); } -size_t kasan_metadata_size(struct kmem_cache *cache) +size_t kasan_metadata_size(struct kmem_cache *cache, bool in_object) { + struct kasan_cache *info = &cache->kasan_info; + if (!kasan_requires_meta()) return 0; - return (cache->kasan_info.alloc_meta_offset ? - sizeof(struct kasan_alloc_meta) : 0) + - ((cache->kasan_info.free_meta_offset && - cache->kasan_info.free_meta_offset != KASAN_NO_FREE_META) ? - sizeof(struct kasan_free_meta) : 0); + + if (in_object) + return (info->free_meta_offset ? + 0 : sizeof(struct kasan_free_meta)); + else + return (info->alloc_meta_offset ? + sizeof(struct kasan_alloc_meta) : 0) + + ((info->free_meta_offset && + info->free_meta_offset != KASAN_NO_FREE_META) ? + sizeof(struct kasan_free_meta) : 0); } static void __kasan_record_aux_stack(void *addr, bool can_alloc) diff --git a/mm/slub.c b/mm/slub.c index 17292c2d3eee..adff7553b54e 100644 --- a/mm/slub.c +++ b/mm/slub.c @@ -910,7 +910,7 @@ static void print_trailer(struct kmem_cache *s, struct slab *slab, u8 *p) if (slub_debug_orig_size(s)) off += sizeof(unsigned int); - off += kasan_metadata_size(s); + off += kasan_metadata_size(s, false); if (off != size_from_object(s)) /* Beginning of the filler is the free pointer */ @@ -1070,7 +1070,7 @@ static int check_pad_bytes(struct kmem_cache *s, struct slab *slab, u8 *p) off += sizeof(unsigned int); } - off += kasan_metadata_size(s); + off += kasan_metadata_size(s, false); if (size_from_object(s) == off) return 1; -- 2.34.1 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH v7 2/3] mm: kasan: Extend kasan_metadata_size() to also cover in-object size 2022-10-21 3:24 ` [PATCH v7 2/3] mm: kasan: Extend kasan_metadata_size() to also cover in-object size Feng Tang @ 2022-10-27 19:27 ` Andrey Konovalov 0 siblings, 0 replies; 20+ messages in thread From: Andrey Konovalov @ 2022-10-27 19:27 UTC (permalink / raw) To: Feng Tang Cc: Andrew Morton, Vlastimil Babka, Christoph Lameter, Pekka Enberg, David Rientjes, Joonsoo Kim, Roman Gushchin, Hyeonggon Yoo, Dmitry Vyukov, Kees Cook, Dave Hansen, linux-mm, linux-kernel, kasan-dev, kernel test robot, Andrey Ryabinin, Alexander Potapenko, Vincenzo Frascino On Fri, Oct 21, 2022 at 5:24 AM Feng Tang <feng.tang@intel.com> wrote: > > When kasan is enabled for slab/slub, it may save kasan' free_meta > data in the former part of slab object data area in slab object's > free path, which works fine. > > There is ongoing effort to extend slub's debug function which will > redzone the latter part of kmalloc object area, and when both of > the debug are enabled, there is possible conflict, especially when > the kmalloc object has small size, as caught by 0Day bot [1]. > > To solve it, slub code needs to know the in-object kasan's meta > data size. Currently, there is existing kasan_metadata_size() > which returns the kasan's metadata size inside slub's metadata > area, so extend it to also cover the in-object meta size by > adding a boolean flag 'in_object'. > > There is no functional change to existing code logic. > > [1]. https://lore.kernel.org/lkml/YuYm3dWwpZwH58Hu@xsang-OptiPlex-9020/ > Reported-by: kernel test robot <oliver.sang@intel.com> > Suggested-by: Andrey Konovalov <andreyknvl@gmail.com> > Signed-off-by: Feng Tang <feng.tang@intel.com> > Reviewed-by: Andrey Konovalov <andreyknvl@gmail.com> > Cc: Andrey Ryabinin <ryabinin.a.a@gmail.com> > Cc: Alexander Potapenko <glider@google.com> > Cc: Dmitry Vyukov <dvyukov@google.com> > Cc: Vincenzo Frascino <vincenzo.frascino@arm.com> > --- > include/linux/kasan.h | 5 +++-- > mm/kasan/generic.c | 19 +++++++++++++------ > mm/slub.c | 4 ++-- > 3 files changed, 18 insertions(+), 10 deletions(-) > > diff --git a/include/linux/kasan.h b/include/linux/kasan.h > index d811b3d7d2a1..96c9d56e5510 100644 > --- a/include/linux/kasan.h > +++ b/include/linux/kasan.h > @@ -302,7 +302,7 @@ static inline void kasan_unpoison_task_stack(struct task_struct *task) {} > > #ifdef CONFIG_KASAN_GENERIC > > -size_t kasan_metadata_size(struct kmem_cache *cache); > +size_t kasan_metadata_size(struct kmem_cache *cache, bool in_object); > slab_flags_t kasan_never_merge(void); > void kasan_cache_create(struct kmem_cache *cache, unsigned int *size, > slab_flags_t *flags); > @@ -315,7 +315,8 @@ void kasan_record_aux_stack_noalloc(void *ptr); > #else /* CONFIG_KASAN_GENERIC */ > > /* Tag-based KASAN modes do not use per-object metadata. */ > -static inline size_t kasan_metadata_size(struct kmem_cache *cache) > +static inline size_t kasan_metadata_size(struct kmem_cache *cache, > + bool in_object) > { > return 0; > } > diff --git a/mm/kasan/generic.c b/mm/kasan/generic.c > index d8b5590f9484..b076f597a378 100644 > --- a/mm/kasan/generic.c > +++ b/mm/kasan/generic.c > @@ -450,15 +450,22 @@ void kasan_init_object_meta(struct kmem_cache *cache, const void *object) > __memset(alloc_meta, 0, sizeof(*alloc_meta)); > } > > -size_t kasan_metadata_size(struct kmem_cache *cache) > +size_t kasan_metadata_size(struct kmem_cache *cache, bool in_object) > { > + struct kasan_cache *info = &cache->kasan_info; > + > if (!kasan_requires_meta()) > return 0; > - return (cache->kasan_info.alloc_meta_offset ? > - sizeof(struct kasan_alloc_meta) : 0) + > - ((cache->kasan_info.free_meta_offset && > - cache->kasan_info.free_meta_offset != KASAN_NO_FREE_META) ? > - sizeof(struct kasan_free_meta) : 0); > + > + if (in_object) > + return (info->free_meta_offset ? > + 0 : sizeof(struct kasan_free_meta)); > + else > + return (info->alloc_meta_offset ? > + sizeof(struct kasan_alloc_meta) : 0) + > + ((info->free_meta_offset && > + info->free_meta_offset != KASAN_NO_FREE_META) ? > + sizeof(struct kasan_free_meta) : 0); > } > > static void __kasan_record_aux_stack(void *addr, bool can_alloc) > diff --git a/mm/slub.c b/mm/slub.c > index 17292c2d3eee..adff7553b54e 100644 > --- a/mm/slub.c > +++ b/mm/slub.c > @@ -910,7 +910,7 @@ static void print_trailer(struct kmem_cache *s, struct slab *slab, u8 *p) > if (slub_debug_orig_size(s)) > off += sizeof(unsigned int); > > - off += kasan_metadata_size(s); > + off += kasan_metadata_size(s, false); > > if (off != size_from_object(s)) > /* Beginning of the filler is the free pointer */ > @@ -1070,7 +1070,7 @@ static int check_pad_bytes(struct kmem_cache *s, struct slab *slab, u8 *p) > off += sizeof(unsigned int); > } > > - off += kasan_metadata_size(s); > + off += kasan_metadata_size(s, false); > > if (size_from_object(s) == off) > return 1; > -- > 2.34.1 > Reviewed-by: Andrey Konovalov <andreyknvl@gmail.com> Thanks! ^ permalink raw reply [flat|nested] 20+ messages in thread
* [PATCH v7 3/3] mm/slub: extend redzone check to extra allocated kmalloc space than requested 2022-10-21 3:24 [PATCH v7 0/3] mm/slub: extend redzone check for kmalloc objects Feng Tang 2022-10-21 3:24 ` [PATCH v7 1/3] mm/slub: only zero requested size of buffer for kzalloc when debug enabled Feng Tang 2022-10-21 3:24 ` [PATCH v7 2/3] mm: kasan: Extend kasan_metadata_size() to also cover in-object size Feng Tang @ 2022-10-21 3:24 ` Feng Tang 2022-11-10 15:48 ` Vlastimil Babka 2022-11-11 8:16 ` [PATCH v7 0/3] mm/slub: extend redzone check for kmalloc objects Vlastimil Babka 3 siblings, 1 reply; 20+ messages in thread From: Feng Tang @ 2022-10-21 3:24 UTC (permalink / raw) To: Andrew Morton, Vlastimil Babka, Christoph Lameter, Pekka Enberg, David Rientjes, Joonsoo Kim, Roman Gushchin, Hyeonggon Yoo, Dmitry Vyukov, Andrey Konovalov, Kees Cook Cc: Dave Hansen, linux-mm, linux-kernel, kasan-dev, Feng Tang kmalloc will round up the request size to a fixed size (mostly power of 2), so there could be a extra space than what is requested, whose size is the actual buffer size minus original request size. To better detect out of bound access or abuse of this space, add redzone sanity check for it. In current kernel, some kmalloc user already knows the existence of the space and utilizes it after calling 'ksize()' to know the real size of the allocated buffer. So we skip the sanity check for objects which have been called with ksize(), as treating them as legitimate users. In some cases, the free pointer could be saved inside the latter part of object data area, which may overlap the redzone part(for small sizes of kmalloc objects). As suggested by Hyeonggon Yoo, force the free pointer to be in meta data area when kmalloc redzone debug is enabled, to make all kmalloc objects covered by redzone check. Suggested-by: Vlastimil Babka <vbabka@suse.cz> Signed-off-by: Feng Tang <feng.tang@intel.com> Acked-by: Hyeonggon Yoo <42.hyeyoo@gmail.com> --- mm/slab.h | 4 ++++ mm/slab_common.c | 4 ++++ mm/slub.c | 51 ++++++++++++++++++++++++++++++++++++++++++++---- 3 files changed, 55 insertions(+), 4 deletions(-) diff --git a/mm/slab.h b/mm/slab.h index 8b4ee02fc14a..1dd773afd0c4 100644 --- a/mm/slab.h +++ b/mm/slab.h @@ -885,4 +885,8 @@ void __check_heap_object(const void *ptr, unsigned long n, } #endif +#ifdef CONFIG_SLUB_DEBUG +void skip_orig_size_check(struct kmem_cache *s, const void *object); +#endif + #endif /* MM_SLAB_H */ diff --git a/mm/slab_common.c b/mm/slab_common.c index 33b1886b06eb..0bb4625f10a2 100644 --- a/mm/slab_common.c +++ b/mm/slab_common.c @@ -1037,6 +1037,10 @@ size_t __ksize(const void *object) return folio_size(folio); } +#ifdef CONFIG_SLUB_DEBUG + skip_orig_size_check(folio_slab(folio)->slab_cache, object); +#endif + return slab_ksize(folio_slab(folio)->slab_cache); } diff --git a/mm/slub.c b/mm/slub.c index adff7553b54e..76581da6b9df 100644 --- a/mm/slub.c +++ b/mm/slub.c @@ -829,6 +829,17 @@ static inline void set_orig_size(struct kmem_cache *s, if (!slub_debug_orig_size(s)) return; +#ifdef CONFIG_KASAN_GENERIC + /* + * KASAN could save its free meta data in object's data area at + * offset 0, if the size is larger than 'orig_size', it will + * overlap the data redzone in [orig_size+1, object_size], and + * the check should be skipped. + */ + if (kasan_metadata_size(s, true) > orig_size) + orig_size = s->object_size; +#endif + p += get_info_end(s); p += sizeof(struct track) * 2; @@ -848,6 +859,11 @@ static inline unsigned int get_orig_size(struct kmem_cache *s, void *object) return *(unsigned int *)p; } +void skip_orig_size_check(struct kmem_cache *s, const void *object) +{ + set_orig_size(s, (void *)object, s->object_size); +} + static void slab_bug(struct kmem_cache *s, char *fmt, ...) { struct va_format vaf; @@ -966,13 +982,27 @@ static __printf(3, 4) void slab_err(struct kmem_cache *s, struct slab *slab, static void init_object(struct kmem_cache *s, void *object, u8 val) { u8 *p = kasan_reset_tag(object); + unsigned int orig_size = s->object_size; - if (s->flags & SLAB_RED_ZONE) + if (s->flags & SLAB_RED_ZONE) { memset(p - s->red_left_pad, val, s->red_left_pad); + if (slub_debug_orig_size(s) && val == SLUB_RED_ACTIVE) { + orig_size = get_orig_size(s, object); + + /* + * Redzone the extra allocated space by kmalloc + * than requested. + */ + if (orig_size < s->object_size) + memset(p + orig_size, val, + s->object_size - orig_size); + } + } + if (s->flags & __OBJECT_POISON) { - memset(p, POISON_FREE, s->object_size - 1); - p[s->object_size - 1] = POISON_END; + memset(p, POISON_FREE, orig_size - 1); + p[orig_size - 1] = POISON_END; } if (s->flags & SLAB_RED_ZONE) @@ -1120,6 +1150,7 @@ static int check_object(struct kmem_cache *s, struct slab *slab, { u8 *p = object; u8 *endobject = object + s->object_size; + unsigned int orig_size; if (s->flags & SLAB_RED_ZONE) { if (!check_bytes_and_report(s, slab, object, "Left Redzone", @@ -1129,6 +1160,17 @@ static int check_object(struct kmem_cache *s, struct slab *slab, if (!check_bytes_and_report(s, slab, object, "Right Redzone", endobject, val, s->inuse - s->object_size)) return 0; + + if (slub_debug_orig_size(s) && val == SLUB_RED_ACTIVE) { + orig_size = get_orig_size(s, object); + + if (s->object_size > orig_size && + !check_bytes_and_report(s, slab, object, + "kmalloc Redzone", p + orig_size, + val, s->object_size - orig_size)) { + return 0; + } + } } else { if ((s->flags & SLAB_POISON) && s->object_size < s->inuse) { check_bytes_and_report(s, slab, p, "Alignment padding", @@ -4206,7 +4248,8 @@ static int calculate_sizes(struct kmem_cache *s) */ s->inuse = size; - if ((flags & (SLAB_TYPESAFE_BY_RCU | SLAB_POISON)) || + if (slub_debug_orig_size(s) || + (flags & (SLAB_TYPESAFE_BY_RCU | SLAB_POISON)) || ((flags & SLAB_RED_ZONE) && s->object_size < sizeof(void *)) || s->ctor) { /* -- 2.34.1 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH v7 3/3] mm/slub: extend redzone check to extra allocated kmalloc space than requested 2022-10-21 3:24 ` [PATCH v7 3/3] mm/slub: extend redzone check to extra allocated kmalloc space than requested Feng Tang @ 2022-11-10 15:48 ` Vlastimil Babka 2022-11-11 6:46 ` Feng Tang 0 siblings, 1 reply; 20+ messages in thread From: Vlastimil Babka @ 2022-11-10 15:48 UTC (permalink / raw) To: Feng Tang, Andrew Morton, Christoph Lameter, Pekka Enberg, David Rientjes, Joonsoo Kim, Roman Gushchin, Hyeonggon Yoo, Dmitry Vyukov, Andrey Konovalov, Kees Cook Cc: Dave Hansen, linux-mm, linux-kernel, kasan-dev On 10/21/22 05:24, Feng Tang wrote: > kmalloc will round up the request size to a fixed size (mostly power > of 2), so there could be a extra space than what is requested, whose > size is the actual buffer size minus original request size. > > To better detect out of bound access or abuse of this space, add > redzone sanity check for it. > > In current kernel, some kmalloc user already knows the existence of > the space and utilizes it after calling 'ksize()' to know the real > size of the allocated buffer. So we skip the sanity check for objects > which have been called with ksize(), as treating them as legitimate > users. Hm so once Kees's effort is finished and all ksize() users behave correctly, we can drop all that skip_orig_size_check() code, right? > In some cases, the free pointer could be saved inside the latter > part of object data area, which may overlap the redzone part(for > small sizes of kmalloc objects). As suggested by Hyeonggon Yoo, > force the free pointer to be in meta data area when kmalloc redzone > debug is enabled, to make all kmalloc objects covered by redzone > check. > > Suggested-by: Vlastimil Babka <vbabka@suse.cz> > Signed-off-by: Feng Tang <feng.tang@intel.com> > Acked-by: Hyeonggon Yoo <42.hyeyoo@gmail.com> Looks fine, but a suggestion below: > --- > mm/slab.h | 4 ++++ > mm/slab_common.c | 4 ++++ > mm/slub.c | 51 ++++++++++++++++++++++++++++++++++++++++++++---- > 3 files changed, 55 insertions(+), 4 deletions(-) > > diff --git a/mm/slab.h b/mm/slab.h > index 8b4ee02fc14a..1dd773afd0c4 100644 > --- a/mm/slab.h > +++ b/mm/slab.h > @@ -885,4 +885,8 @@ void __check_heap_object(const void *ptr, unsigned long n, > } > #endif > > +#ifdef CONFIG_SLUB_DEBUG > +void skip_orig_size_check(struct kmem_cache *s, const void *object); > +#endif > + > #endif /* MM_SLAB_H */ > diff --git a/mm/slab_common.c b/mm/slab_common.c > index 33b1886b06eb..0bb4625f10a2 100644 > --- a/mm/slab_common.c > +++ b/mm/slab_common.c > @@ -1037,6 +1037,10 @@ size_t __ksize(const void *object) > return folio_size(folio); > } > > +#ifdef CONFIG_SLUB_DEBUG > + skip_orig_size_check(folio_slab(folio)->slab_cache, object); > +#endif > + > return slab_ksize(folio_slab(folio)->slab_cache); > } > > diff --git a/mm/slub.c b/mm/slub.c > index adff7553b54e..76581da6b9df 100644 > --- a/mm/slub.c > +++ b/mm/slub.c > @@ -829,6 +829,17 @@ static inline void set_orig_size(struct kmem_cache *s, > if (!slub_debug_orig_size(s)) > return; > > +#ifdef CONFIG_KASAN_GENERIC > + /* > + * KASAN could save its free meta data in object's data area at > + * offset 0, if the size is larger than 'orig_size', it will > + * overlap the data redzone in [orig_size+1, object_size], and > + * the check should be skipped. > + */ > + if (kasan_metadata_size(s, true) > orig_size) > + orig_size = s->object_size; > +#endif > + > p += get_info_end(s); > p += sizeof(struct track) * 2; > > @@ -848,6 +859,11 @@ static inline unsigned int get_orig_size(struct kmem_cache *s, void *object) > return *(unsigned int *)p; > } > > +void skip_orig_size_check(struct kmem_cache *s, const void *object) > +{ > + set_orig_size(s, (void *)object, s->object_size); > +} > + > static void slab_bug(struct kmem_cache *s, char *fmt, ...) > { > struct va_format vaf; > @@ -966,13 +982,27 @@ static __printf(3, 4) void slab_err(struct kmem_cache *s, struct slab *slab, > static void init_object(struct kmem_cache *s, void *object, u8 val) > { > u8 *p = kasan_reset_tag(object); > + unsigned int orig_size = s->object_size; > > - if (s->flags & SLAB_RED_ZONE) > + if (s->flags & SLAB_RED_ZONE) { > memset(p - s->red_left_pad, val, s->red_left_pad); > > + if (slub_debug_orig_size(s) && val == SLUB_RED_ACTIVE) { > + orig_size = get_orig_size(s, object); > + > + /* > + * Redzone the extra allocated space by kmalloc > + * than requested. > + */ > + if (orig_size < s->object_size) > + memset(p + orig_size, val, > + s->object_size - orig_size); Wondering if we can remove this if - memset and instead below: > + } > + } > + > if (s->flags & __OBJECT_POISON) { > - memset(p, POISON_FREE, s->object_size - 1); > - p[s->object_size - 1] = POISON_END; > + memset(p, POISON_FREE, orig_size - 1); > + p[orig_size - 1] = POISON_END; > } > > if (s->flags & SLAB_RED_ZONE) This continues by: memset(p + s->object_size, val, s->inuse - s->object_size); Instead we could do this, no? memset(p + orig_size, val, s->inuse - orig_size); > @@ -1120,6 +1150,7 @@ static int check_object(struct kmem_cache *s, struct slab *slab, > { > u8 *p = object; > u8 *endobject = object + s->object_size; > + unsigned int orig_size; > > if (s->flags & SLAB_RED_ZONE) { > if (!check_bytes_and_report(s, slab, object, "Left Redzone", > @@ -1129,6 +1160,17 @@ static int check_object(struct kmem_cache *s, struct slab *slab, > if (!check_bytes_and_report(s, slab, object, "Right Redzone", > endobject, val, s->inuse - s->object_size)) > return 0; > + > + if (slub_debug_orig_size(s) && val == SLUB_RED_ACTIVE) { > + orig_size = get_orig_size(s, object); > + > + if (s->object_size > orig_size && > + !check_bytes_and_report(s, slab, object, > + "kmalloc Redzone", p + orig_size, > + val, s->object_size - orig_size)) { > + return 0; > + } > + } > } else { > if ((s->flags & SLAB_POISON) && s->object_size < s->inuse) { > check_bytes_and_report(s, slab, p, "Alignment padding", > @@ -4206,7 +4248,8 @@ static int calculate_sizes(struct kmem_cache *s) > */ > s->inuse = size; > > - if ((flags & (SLAB_TYPESAFE_BY_RCU | SLAB_POISON)) || > + if (slub_debug_orig_size(s) || > + (flags & (SLAB_TYPESAFE_BY_RCU | SLAB_POISON)) || > ((flags & SLAB_RED_ZONE) && s->object_size < sizeof(void *)) || > s->ctor) { > /* ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v7 3/3] mm/slub: extend redzone check to extra allocated kmalloc space than requested 2022-11-10 15:48 ` Vlastimil Babka @ 2022-11-11 6:46 ` Feng Tang 2022-11-11 8:12 ` Vlastimil Babka 0 siblings, 1 reply; 20+ messages in thread From: Feng Tang @ 2022-11-11 6:46 UTC (permalink / raw) To: Vlastimil Babka Cc: Andrew Morton, Christoph Lameter, Pekka Enberg, David Rientjes, Joonsoo Kim, Roman Gushchin, Hyeonggon Yoo, Dmitry Vyukov, Andrey Konovalov, Kees Cook, Dave Hansen, linux-mm, linux-kernel, kasan-dev On Thu, Nov 10, 2022 at 04:48:35PM +0100, Vlastimil Babka wrote: > On 10/21/22 05:24, Feng Tang wrote: > > kmalloc will round up the request size to a fixed size (mostly power > > of 2), so there could be a extra space than what is requested, whose > > size is the actual buffer size minus original request size. > > > > To better detect out of bound access or abuse of this space, add > > redzone sanity check for it. > > > > In current kernel, some kmalloc user already knows the existence of > > the space and utilizes it after calling 'ksize()' to know the real > > size of the allocated buffer. So we skip the sanity check for objects > > which have been called with ksize(), as treating them as legitimate > > users. > > Hm so once Kees's effort is finished and all ksize() users behave correctly, > we can drop all that skip_orig_size_check() code, right? Yes, will update the commit log. > > In some cases, the free pointer could be saved inside the latter > > part of object data area, which may overlap the redzone part(for > > small sizes of kmalloc objects). As suggested by Hyeonggon Yoo, > > force the free pointer to be in meta data area when kmalloc redzone > > debug is enabled, to make all kmalloc objects covered by redzone > > check. > > > > Suggested-by: Vlastimil Babka <vbabka@suse.cz> > > Signed-off-by: Feng Tang <feng.tang@intel.com> > > Acked-by: Hyeonggon Yoo <42.hyeyoo@gmail.com> > > Looks fine, but a suggestion below: > [...] > > @@ -966,13 +982,27 @@ static __printf(3, 4) void slab_err(struct kmem_cache *s, struct slab *slab, > > static void init_object(struct kmem_cache *s, void *object, u8 val) > > { > > u8 *p = kasan_reset_tag(object); > > + unsigned int orig_size = s->object_size; > > > > - if (s->flags & SLAB_RED_ZONE) > > + if (s->flags & SLAB_RED_ZONE) { > > memset(p - s->red_left_pad, val, s->red_left_pad); > > > > + if (slub_debug_orig_size(s) && val == SLUB_RED_ACTIVE) { > > + orig_size = get_orig_size(s, object); > > + > > + /* > > + * Redzone the extra allocated space by kmalloc > > + * than requested. > > + */ > > + if (orig_size < s->object_size) > > + memset(p + orig_size, val, > > + s->object_size - orig_size); > > Wondering if we can remove this if - memset and instead below: > > > + } > > + } > > + > > if (s->flags & __OBJECT_POISON) { > > - memset(p, POISON_FREE, s->object_size - 1); > > - p[s->object_size - 1] = POISON_END; > > + memset(p, POISON_FREE, orig_size - 1); > > + p[orig_size - 1] = POISON_END; > > } > > > > if (s->flags & SLAB_RED_ZONE) > > This continues by: > memset(p + s->object_size, val, s->inuse - s->object_size); > Instead we could do this, no? > memset(p + orig_size, val, s->inuse - orig_size); Yep, the code is much simpler and cleaner! thanks I also change the name from 'orig_size' to 'poison_size', as below: Thanks, Feng -----8>----- From 21dc7a27bb9206937ec5cc584a70da452fc249c6 Mon Sep 17 00:00:00 2001 From: Feng Tang <feng.tang@intel.com> Date: Wed, 12 Oct 2022 13:39:09 +0800 Subject: [PATCH 3/3] mm/slub: extend redzone check to extra allocated kmalloc space than requested kmalloc will round up the request size to a fixed size (mostly power of 2), so there could be a extra space than what is requested, whose size is the actual buffer size minus original request size. To better detect out of bound access or abuse of this space, add redzone sanity check for it. In current kernel, some kmalloc user already knows the existence of the space and utilizes it after calling 'ksize()' to know the real size of the allocated buffer. So we skip the sanity check for objects which have been called with ksize(), as treating them as legitimate users. Kees Cook is working on sanitizing all these user cases, by using kmalloc_size_roundup() to avoid ambiguous usages. And after this is done, this special handling for ksize() can be removed. In some cases, the free pointer could be saved inside the latter part of object data area, which may overlap the redzone part(for small sizes of kmalloc objects). As suggested by Hyeonggon Yoo, force the free pointer to be in meta data area when kmalloc redzone debug is enabled, to make all kmalloc objects covered by redzone check. Suggested-by: Vlastimil Babka <vbabka@suse.cz> Signed-off-by: Feng Tang <feng.tang@intel.com> Acked-by: Hyeonggon Yoo <42.hyeyoo@gmail.com> --- mm/slab.h | 4 ++++ mm/slab_common.c | 4 ++++ mm/slub.c | 50 +++++++++++++++++++++++++++++++++++++++++++----- 3 files changed, 53 insertions(+), 5 deletions(-) diff --git a/mm/slab.h b/mm/slab.h index 2551214392c7..de9ef5b4931e 100644 --- a/mm/slab.h +++ b/mm/slab.h @@ -896,4 +896,8 @@ void __check_heap_object(const void *ptr, unsigned long n, } #endif +#ifdef CONFIG_SLUB_DEBUG +void skip_orig_size_check(struct kmem_cache *s, const void *object); +#endif + #endif /* MM_SLAB_H */ diff --git a/mm/slab_common.c b/mm/slab_common.c index 0042fb2730d1..8276022f0da4 100644 --- a/mm/slab_common.c +++ b/mm/slab_common.c @@ -1037,6 +1037,10 @@ size_t __ksize(const void *object) return folio_size(folio); } +#ifdef CONFIG_SLUB_DEBUG + skip_orig_size_check(folio_slab(folio)->slab_cache, object); +#endif + return slab_ksize(folio_slab(folio)->slab_cache); } diff --git a/mm/slub.c b/mm/slub.c index 8d26187de915..03b7f4056619 100644 --- a/mm/slub.c +++ b/mm/slub.c @@ -829,6 +829,17 @@ static inline void set_orig_size(struct kmem_cache *s, if (!slub_debug_orig_size(s)) return; +#ifdef CONFIG_KASAN_GENERIC + /* + * KASAN could save its free meta data in object's data area at + * offset 0, if the size is larger than 'orig_size', it will + * overlap the data redzone in [orig_size+1, object_size], and + * the check should be skipped. + */ + if (kasan_metadata_size(s, true) > orig_size) + orig_size = s->object_size; +#endif + p += get_info_end(s); p += sizeof(struct track) * 2; @@ -848,6 +859,11 @@ static inline unsigned int get_orig_size(struct kmem_cache *s, void *object) return *(unsigned int *)p; } +void skip_orig_size_check(struct kmem_cache *s, const void *object) +{ + set_orig_size(s, (void *)object, s->object_size); +} + static void slab_bug(struct kmem_cache *s, char *fmt, ...) { struct va_format vaf; @@ -966,17 +982,28 @@ static __printf(3, 4) void slab_err(struct kmem_cache *s, struct slab *slab, static void init_object(struct kmem_cache *s, void *object, u8 val) { u8 *p = kasan_reset_tag(object); + unsigned int poison_size = s->object_size; - if (s->flags & SLAB_RED_ZONE) + if (s->flags & SLAB_RED_ZONE) { memset(p - s->red_left_pad, val, s->red_left_pad); + if (slub_debug_orig_size(s) && val == SLUB_RED_ACTIVE) { + /* + * Redzone the extra allocated space by kmalloc than + * requested, and the poison size will be limited to + * the original request size accordingly. + */ + poison_size = get_orig_size(s, object); + } + } + if (s->flags & __OBJECT_POISON) { - memset(p, POISON_FREE, s->object_size - 1); - p[s->object_size - 1] = POISON_END; + memset(p, POISON_FREE, poison_size - 1); + p[poison_size - 1] = POISON_END; } if (s->flags & SLAB_RED_ZONE) - memset(p + s->object_size, val, s->inuse - s->object_size); + memset(p + poison_size, val, s->inuse - poison_size); } static void restore_bytes(struct kmem_cache *s, char *message, u8 data, @@ -1120,6 +1147,7 @@ static int check_object(struct kmem_cache *s, struct slab *slab, { u8 *p = object; u8 *endobject = object + s->object_size; + unsigned int orig_size; if (s->flags & SLAB_RED_ZONE) { if (!check_bytes_and_report(s, slab, object, "Left Redzone", @@ -1129,6 +1157,17 @@ static int check_object(struct kmem_cache *s, struct slab *slab, if (!check_bytes_and_report(s, slab, object, "Right Redzone", endobject, val, s->inuse - s->object_size)) return 0; + + if (slub_debug_orig_size(s) && val == SLUB_RED_ACTIVE) { + orig_size = get_orig_size(s, object); + + if (s->object_size > orig_size && + !check_bytes_and_report(s, slab, object, + "kmalloc Redzone", p + orig_size, + val, s->object_size - orig_size)) { + return 0; + } + } } else { if ((s->flags & SLAB_POISON) && s->object_size < s->inuse) { check_bytes_and_report(s, slab, p, "Alignment padding", @@ -4199,7 +4238,8 @@ static int calculate_sizes(struct kmem_cache *s) */ s->inuse = size; - if ((flags & (SLAB_TYPESAFE_BY_RCU | SLAB_POISON)) || + if (slub_debug_orig_size(s) || + (flags & (SLAB_TYPESAFE_BY_RCU | SLAB_POISON)) || ((flags & SLAB_RED_ZONE) && s->object_size < sizeof(void *)) || s->ctor) { /* -- 2.34.1 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH v7 3/3] mm/slub: extend redzone check to extra allocated kmalloc space than requested 2022-11-11 6:46 ` Feng Tang @ 2022-11-11 8:12 ` Vlastimil Babka 0 siblings, 0 replies; 20+ messages in thread From: Vlastimil Babka @ 2022-11-11 8:12 UTC (permalink / raw) To: Feng Tang Cc: Andrew Morton, Christoph Lameter, Pekka Enberg, David Rientjes, Joonsoo Kim, Roman Gushchin, Hyeonggon Yoo, Dmitry Vyukov, Andrey Konovalov, Kees Cook, Dave Hansen, linux-mm, linux-kernel, kasan-dev On 11/11/22 07:46, Feng Tang wrote: > On Thu, Nov 10, 2022 at 04:48:35PM +0100, Vlastimil Babka wrote: >> On 10/21/22 05:24, Feng Tang wrote: >> > kmalloc will round up the request size to a fixed size (mostly power >> > of 2), so there could be a extra space than what is requested, whose >> > size is the actual buffer size minus original request size. >> > >> > To better detect out of bound access or abuse of this space, add >> > redzone sanity check for it. >> > >> > In current kernel, some kmalloc user already knows the existence of >> > the space and utilizes it after calling 'ksize()' to know the real >> > size of the allocated buffer. So we skip the sanity check for objects >> > which have been called with ksize(), as treating them as legitimate >> > users. >> >> Hm so once Kees's effort is finished and all ksize() users behave correctly, >> we can drop all that skip_orig_size_check() code, right? > > Yes, will update the commit log. > >> > In some cases, the free pointer could be saved inside the latter >> > part of object data area, which may overlap the redzone part(for >> > small sizes of kmalloc objects). As suggested by Hyeonggon Yoo, >> > force the free pointer to be in meta data area when kmalloc redzone >> > debug is enabled, to make all kmalloc objects covered by redzone >> > check. >> > >> > Suggested-by: Vlastimil Babka <vbabka@suse.cz> >> > Signed-off-by: Feng Tang <feng.tang@intel.com> >> > Acked-by: Hyeonggon Yoo <42.hyeyoo@gmail.com> >> >> Looks fine, but a suggestion below: >> > [...] >> > @@ -966,13 +982,27 @@ static __printf(3, 4) void slab_err(struct kmem_cache *s, struct slab *slab, >> > static void init_object(struct kmem_cache *s, void *object, u8 val) >> > { >> > u8 *p = kasan_reset_tag(object); >> > + unsigned int orig_size = s->object_size; >> > >> > - if (s->flags & SLAB_RED_ZONE) >> > + if (s->flags & SLAB_RED_ZONE) { >> > memset(p - s->red_left_pad, val, s->red_left_pad); >> > >> > + if (slub_debug_orig_size(s) && val == SLUB_RED_ACTIVE) { >> > + orig_size = get_orig_size(s, object); >> > + >> > + /* >> > + * Redzone the extra allocated space by kmalloc >> > + * than requested. >> > + */ >> > + if (orig_size < s->object_size) >> > + memset(p + orig_size, val, >> > + s->object_size - orig_size); >> >> Wondering if we can remove this if - memset and instead below: >> >> > + } >> > + } >> > + >> > if (s->flags & __OBJECT_POISON) { >> > - memset(p, POISON_FREE, s->object_size - 1); >> > - p[s->object_size - 1] = POISON_END; >> > + memset(p, POISON_FREE, orig_size - 1); >> > + p[orig_size - 1] = POISON_END; >> > } >> > >> > if (s->flags & SLAB_RED_ZONE) >> >> This continues by: >> memset(p + s->object_size, val, s->inuse - s->object_size); >> Instead we could do this, no? >> memset(p + orig_size, val, s->inuse - orig_size); > > Yep, the code is much simpler and cleaner! thanks > > I also change the name from 'orig_size' to 'poison_size', as below: > > Thanks, > Feng Thanks! Now merged all to slab/for-6.2/kmalloc_redzone and for-next ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v7 0/3] mm/slub: extend redzone check for kmalloc objects 2022-10-21 3:24 [PATCH v7 0/3] mm/slub: extend redzone check for kmalloc objects Feng Tang ` (2 preceding siblings ...) 2022-10-21 3:24 ` [PATCH v7 3/3] mm/slub: extend redzone check to extra allocated kmalloc space than requested Feng Tang @ 2022-11-11 8:16 ` Vlastimil Babka 2022-11-11 8:29 ` Feng Tang 3 siblings, 1 reply; 20+ messages in thread From: Vlastimil Babka @ 2022-11-11 8:16 UTC (permalink / raw) To: Feng Tang, Andrew Morton, Christoph Lameter, Pekka Enberg, David Rientjes, Joonsoo Kim, Roman Gushchin, Hyeonggon Yoo, Dmitry Vyukov, Andrey Konovalov, Kees Cook Cc: Dave Hansen, linux-mm, linux-kernel, kasan-dev On 10/21/22 05:24, Feng Tang wrote: > kmalloc's API family is critical for mm, and one of its nature is that > it will round up the request size to a fixed one (mostly power of 2). > When user requests memory for '2^n + 1' bytes, actually 2^(n+1) bytes > could be allocated, so there is an extra space than what is originally > requested. > > This patchset tries to extend the redzone sanity check to the extra > kmalloced buffer than requested, to better detect un-legitimate access > to it. (dependson SLAB_STORE_USER & SLAB_RED_ZONE) > > The redzone part has been tested with code below: > > for (shift = 3; shift <= 12; shift++) { > size = 1 << shift; > buf = kmalloc(size + 4, GFP_KERNEL); > /* We have 96, 196 kmalloc size, which is not power of 2 */ > if (size == 64 || size == 128) > oob_size = 16; > else > oob_size = size - 4; > memset(buf + size + 4, 0xee, oob_size); > kfree(buf); > } Sounds like a new slub_kunit test would be useful? :) doesn't need to be that exhaustive wrt all sizes, we could just pick one and check that a write beyond requested kmalloc size is detected? Thanks! ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v7 0/3] mm/slub: extend redzone check for kmalloc objects 2022-11-11 8:16 ` [PATCH v7 0/3] mm/slub: extend redzone check for kmalloc objects Vlastimil Babka @ 2022-11-11 8:29 ` Feng Tang 2022-11-21 6:38 ` Feng Tang 0 siblings, 1 reply; 20+ messages in thread From: Feng Tang @ 2022-11-11 8:29 UTC (permalink / raw) To: Vlastimil Babka Cc: Andrew Morton, Christoph Lameter, Pekka Enberg, David Rientjes, Joonsoo Kim, Roman Gushchin, Hyeonggon Yoo, Dmitry Vyukov, Andrey Konovalov, Kees Cook, Hansen, Dave, linux-mm, linux-kernel, kasan-dev On Fri, Nov 11, 2022 at 04:16:32PM +0800, Vlastimil Babka wrote: > On 10/21/22 05:24, Feng Tang wrote: > > kmalloc's API family is critical for mm, and one of its nature is that > > it will round up the request size to a fixed one (mostly power of 2). > > When user requests memory for '2^n + 1' bytes, actually 2^(n+1) bytes > > could be allocated, so there is an extra space than what is originally > > requested. > > > > This patchset tries to extend the redzone sanity check to the extra > > kmalloced buffer than requested, to better detect un-legitimate access > > to it. (dependson SLAB_STORE_USER & SLAB_RED_ZONE) > > > > The redzone part has been tested with code below: > > > > for (shift = 3; shift <= 12; shift++) { > > size = 1 << shift; > > buf = kmalloc(size + 4, GFP_KERNEL); > > /* We have 96, 196 kmalloc size, which is not power of 2 */ > > if (size == 64 || size == 128) > > oob_size = 16; > > else > > oob_size = size - 4; > > memset(buf + size + 4, 0xee, oob_size); > > kfree(buf); > > } > > Sounds like a new slub_kunit test would be useful? :) doesn't need to be > that exhaustive wrt all sizes, we could just pick one and check that a write > beyond requested kmalloc size is detected? Just git-grepped out slub_kunit.c :), will try to add a case to it. I'll also check if the case will also be caught by other sanitizer tools like kasan/kfence etc. Thanks, Feng > Thanks! > ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v7 0/3] mm/slub: extend redzone check for kmalloc objects 2022-11-11 8:29 ` Feng Tang @ 2022-11-21 6:38 ` Feng Tang 2022-11-23 9:48 ` Vlastimil Babka 0 siblings, 1 reply; 20+ messages in thread From: Feng Tang @ 2022-11-21 6:38 UTC (permalink / raw) To: Vlastimil Babka Cc: Andrew Morton, Christoph Lameter, Pekka Enberg, David Rientjes, Joonsoo Kim, Roman Gushchin, Hyeonggon Yoo, Dmitry Vyukov, Andrey Konovalov, Kees Cook, Hansen, Dave, linux-mm, linux-kernel, kasan-dev On Fri, Nov 11, 2022 at 04:29:43PM +0800, Tang, Feng wrote: > On Fri, Nov 11, 2022 at 04:16:32PM +0800, Vlastimil Babka wrote: > > > for (shift = 3; shift <= 12; shift++) { > > > size = 1 << shift; > > > buf = kmalloc(size + 4, GFP_KERNEL); > > > /* We have 96, 196 kmalloc size, which is not power of 2 */ > > > if (size == 64 || size == 128) > > > oob_size = 16; > > > else > > > oob_size = size - 4; > > > memset(buf + size + 4, 0xee, oob_size); > > > kfree(buf); > > > } > > > > Sounds like a new slub_kunit test would be useful? :) doesn't need to be > > that exhaustive wrt all sizes, we could just pick one and check that a write > > beyond requested kmalloc size is detected? > > Just git-grepped out slub_kunit.c :), will try to add a case to it. > I'll also check if the case will also be caught by other sanitizer > tools like kasan/kfence etc. Just checked, kasan has already has API to disable kasan check temporarily, and I did see sometime kfence can chime in (4 out of 178 runs) so we need skip kfenced address. Here is the draft patch, thanks! From 45bf8d0072e532f43063dbda44c6bb3adcc388b6 Mon Sep 17 00:00:00 2001 From: Feng Tang <feng.tang@intel.com> Date: Mon, 21 Nov 2022 13:17:11 +0800 Subject: [PATCH] mm/slub, kunit: Add a case for kmalloc redzone functionality kmalloc redzone check for slub has been merged, and it's better to add a kunit case for it, which is inspired by a real-world case as described in commit 120ee599b5bf ("staging: octeon-usb: prevent memory corruption"): " octeon-hcd will crash the kernel when SLOB is used. This usually happens after the 18-byte control transfer when a device descriptor is read. The DMA engine is always transfering full 32-bit words and if the transfer is shorter, some random garbage appears after the buffer. The problem is not visible with SLUB since it rounds up the allocations to word boundary, and the extra bytes will go undetected. " Suggested-by: Vlastimil Babka <vbabka@suse.cz> Signed-off-by: Feng Tang <feng.tang@intel.com> --- lib/slub_kunit.c | 42 ++++++++++++++++++++++++++++++++++++++++++ mm/slab.h | 15 +++++++++++++++ mm/slub.c | 4 ++-- 3 files changed, 59 insertions(+), 2 deletions(-) diff --git a/lib/slub_kunit.c b/lib/slub_kunit.c index 7a0564d7cb7a..0653eed19bff 100644 --- a/lib/slub_kunit.c +++ b/lib/slub_kunit.c @@ -120,6 +120,47 @@ static void test_clobber_redzone_free(struct kunit *test) kmem_cache_destroy(s); } + +/* + * This case is simulating a real world case, that a device driver + * requests 18 bytes buffer, but the device HW has obligation to + * operate on 32 bits granularity, so it may actually read or write + * 20 bytes to the buffer, and possibly pollute 2 extra bytes after + * the requested space. + */ +static void test_kmalloc_redzone_access(struct kunit *test) +{ + u8 *p; + + if (!is_slub_debug_flags_enabled(SLAB_STORE_USER | SLAB_RED_ZONE)) + kunit_skip(test, "Test required SLAB_STORE_USER & SLAB_RED_ZONE flags on"); + + p = kmalloc(18, GFP_KERNEL); + +#ifdef CONFIG_KFENCE + { + int max_retry = 10; + + while (is_kfence_address(p) && max_retry--) { + kfree(p); + p = kmalloc(18, GFP_KERNEL); + } + + if (!max_retry) + kunit_skip(test, "Fail to get non-kfenced memory"); + } +#endif + + kasan_disable_current(); + + p[18] = 0xab; + p[19] = 0xab; + kfree(p); + + KUNIT_EXPECT_EQ(test, 3, slab_errors); + kasan_enable_current(); +} + static int test_init(struct kunit *test) { slab_errors = 0; @@ -139,6 +180,7 @@ static struct kunit_case test_cases[] = { #endif KUNIT_CASE(test_clobber_redzone_free), + KUNIT_CASE(test_kmalloc_redzone_access), {} }; diff --git a/mm/slab.h b/mm/slab.h index e3b3231af742..72f7a85e01ab 100644 --- a/mm/slab.h +++ b/mm/slab.h @@ -413,6 +413,17 @@ static inline bool __slub_debug_enabled(void) { return static_branch_unlikely(&slub_debug_enabled); } + +extern slab_flags_t slub_debug; + +/* + * This should only be used in post-boot time, after 'slub_debug' + * gets initialized. + */ +static inline bool is_slub_debug_flags_enabled(slab_flags_t flags) +{ + return (slub_debug & flags) == flags; +} #else static inline void print_tracking(struct kmem_cache *s, void *object) { @@ -421,6 +432,10 @@ static inline bool __slub_debug_enabled(void) { return false; } +static inline bool is_slub_debug_flags_enabled(slab_flags_t flags) +{ + return false; +} #endif /* diff --git a/mm/slub.c b/mm/slub.c index a24b71041b26..6ef72b8f6291 100644 --- a/mm/slub.c +++ b/mm/slub.c @@ -638,9 +638,9 @@ static inline void *restore_red_left(struct kmem_cache *s, void *p) * Debug settings: */ #if defined(CONFIG_SLUB_DEBUG_ON) -static slab_flags_t slub_debug = DEBUG_DEFAULT_FLAGS; +slab_flags_t slub_debug = DEBUG_DEFAULT_FLAGS; #else -static slab_flags_t slub_debug; +slab_flags_t slub_debug; #endif static char *slub_debug_string; -- 2.34.1 ^ permalink raw reply related [flat|nested] 20+ messages in thread
* Re: [PATCH v7 0/3] mm/slub: extend redzone check for kmalloc objects 2022-11-21 6:38 ` Feng Tang @ 2022-11-23 9:48 ` Vlastimil Babka 2022-11-28 5:43 ` Feng Tang 0 siblings, 1 reply; 20+ messages in thread From: Vlastimil Babka @ 2022-11-23 9:48 UTC (permalink / raw) To: Feng Tang Cc: Andrew Morton, Christoph Lameter, Pekka Enberg, David Rientjes, Joonsoo Kim, Roman Gushchin, Hyeonggon Yoo, Dmitry Vyukov, Andrey Konovalov, Kees Cook, Hansen, Dave, linux-mm, linux-kernel, kasan-dev On 11/21/22 07:38, Feng Tang wrote: > On Fri, Nov 11, 2022 at 04:29:43PM +0800, Tang, Feng wrote: >> On Fri, Nov 11, 2022 at 04:16:32PM +0800, Vlastimil Babka wrote: >> > > for (shift = 3; shift <= 12; shift++) { >> > > size = 1 << shift; >> > > buf = kmalloc(size + 4, GFP_KERNEL); >> > > /* We have 96, 196 kmalloc size, which is not power of 2 */ >> > > if (size == 64 || size == 128) >> > > oob_size = 16; >> > > else >> > > oob_size = size - 4; >> > > memset(buf + size + 4, 0xee, oob_size); >> > > kfree(buf); >> > > } >> > >> > Sounds like a new slub_kunit test would be useful? :) doesn't need to be >> > that exhaustive wrt all sizes, we could just pick one and check that a write >> > beyond requested kmalloc size is detected? >> >> Just git-grepped out slub_kunit.c :), will try to add a case to it. >> I'll also check if the case will also be caught by other sanitizer >> tools like kasan/kfence etc. > > Just checked, kasan has already has API to disable kasan check > temporarily, and I did see sometime kfence can chime in (4 out of 178 > runs) so we need skip kfenced address. > > Here is the draft patch, thanks! > > From 45bf8d0072e532f43063dbda44c6bb3adcc388b6 Mon Sep 17 00:00:00 2001 > From: Feng Tang <feng.tang@intel.com> > Date: Mon, 21 Nov 2022 13:17:11 +0800 > Subject: [PATCH] mm/slub, kunit: Add a case for kmalloc redzone functionality > > kmalloc redzone check for slub has been merged, and it's better to add > a kunit case for it, which is inspired by a real-world case as described > in commit 120ee599b5bf ("staging: octeon-usb: prevent memory corruption"): > > " > octeon-hcd will crash the kernel when SLOB is used. This usually happens > after the 18-byte control transfer when a device descriptor is read. > The DMA engine is always transfering full 32-bit words and if the > transfer is shorter, some random garbage appears after the buffer. > The problem is not visible with SLUB since it rounds up the allocations > to word boundary, and the extra bytes will go undetected. > " > Suggested-by: Vlastimil Babka <vbabka@suse.cz> > Signed-off-by: Feng Tang <feng.tang@intel.com> > --- > lib/slub_kunit.c | 42 ++++++++++++++++++++++++++++++++++++++++++ > mm/slab.h | 15 +++++++++++++++ > mm/slub.c | 4 ++-- > 3 files changed, 59 insertions(+), 2 deletions(-) > > diff --git a/lib/slub_kunit.c b/lib/slub_kunit.c > index 7a0564d7cb7a..0653eed19bff 100644 > --- a/lib/slub_kunit.c > +++ b/lib/slub_kunit.c > @@ -120,6 +120,47 @@ static void test_clobber_redzone_free(struct kunit *test) > kmem_cache_destroy(s); > } > > + > +/* > + * This case is simulating a real world case, that a device driver > + * requests 18 bytes buffer, but the device HW has obligation to > + * operate on 32 bits granularity, so it may actually read or write > + * 20 bytes to the buffer, and possibly pollute 2 extra bytes after > + * the requested space. > + */ > +static void test_kmalloc_redzone_access(struct kunit *test) > +{ > + u8 *p; > + > + if (!is_slub_debug_flags_enabled(SLAB_STORE_USER | SLAB_RED_ZONE)) > + kunit_skip(test, "Test required SLAB_STORE_USER & SLAB_RED_ZONE flags on"); Hrmm, this is not great. I didn't realize that we're testing kmalloc() specific code, so we can't simply create test-specific caches as in the other kunit tests. What if we did create a fake kmalloc cache with the necessary flags and used it with kmalloc_trace() instead of kmalloc()? We would be bypassing the kmalloc() inline layer so theoretically orig_size handling bugs could be introduced there that the test wouldn't catch, but I think that's rather unlikely. Importantly we would still be stressing the orig_size saving and the adjusted redzone check using this info. > + p = kmalloc(18, GFP_KERNEL); > + > +#ifdef CONFIG_KFENCE > + { > + int max_retry = 10; > + > + while (is_kfence_address(p) && max_retry--) { > + kfree(p); > + p = kmalloc(18, GFP_KERNEL); > + } > + > + if (!max_retry) > + kunit_skip(test, "Fail to get non-kfenced memory"); > + } > +#endif With the test-specific cache we could also pass SLAB_SKIP_KFENCE there to handle this. BTW, don't all slub kunit test need to do that in fact? Thanks, Vlastimil > + > + kasan_disable_current(); > + > + p[18] = 0xab; > + p[19] = 0xab; > + kfree(p); > + > + KUNIT_EXPECT_EQ(test, 3, slab_errors); > + kasan_enable_current(); > +} > + > static int test_init(struct kunit *test) > { > slab_errors = 0; > @@ -139,6 +180,7 @@ static struct kunit_case test_cases[] = { > #endif > > KUNIT_CASE(test_clobber_redzone_free), > + KUNIT_CASE(test_kmalloc_redzone_access), > {} > }; > > diff --git a/mm/slab.h b/mm/slab.h > index e3b3231af742..72f7a85e01ab 100644 > --- a/mm/slab.h > +++ b/mm/slab.h > @@ -413,6 +413,17 @@ static inline bool __slub_debug_enabled(void) > { > return static_branch_unlikely(&slub_debug_enabled); > } > + > +extern slab_flags_t slub_debug; > + > +/* > + * This should only be used in post-boot time, after 'slub_debug' > + * gets initialized. > + */ > +static inline bool is_slub_debug_flags_enabled(slab_flags_t flags) > +{ > + return (slub_debug & flags) == flags; > +} > #else > static inline void print_tracking(struct kmem_cache *s, void *object) > { > @@ -421,6 +432,10 @@ static inline bool __slub_debug_enabled(void) > { > return false; > } > +static inline bool is_slub_debug_flags_enabled(slab_flags_t flags) > +{ > + return false; > +} > #endif > > /* > diff --git a/mm/slub.c b/mm/slub.c > index a24b71041b26..6ef72b8f6291 100644 > --- a/mm/slub.c > +++ b/mm/slub.c > @@ -638,9 +638,9 @@ static inline void *restore_red_left(struct kmem_cache *s, void *p) > * Debug settings: > */ > #if defined(CONFIG_SLUB_DEBUG_ON) > -static slab_flags_t slub_debug = DEBUG_DEFAULT_FLAGS; > +slab_flags_t slub_debug = DEBUG_DEFAULT_FLAGS; > #else > -static slab_flags_t slub_debug; > +slab_flags_t slub_debug; > #endif > > static char *slub_debug_string; ^ permalink raw reply [flat|nested] 20+ messages in thread
* Re: [PATCH v7 0/3] mm/slub: extend redzone check for kmalloc objects 2022-11-23 9:48 ` Vlastimil Babka @ 2022-11-28 5:43 ` Feng Tang 0 siblings, 0 replies; 20+ messages in thread From: Feng Tang @ 2022-11-28 5:43 UTC (permalink / raw) To: Vlastimil Babka Cc: Andrew Morton, Christoph Lameter, Pekka Enberg, David Rientjes, Joonsoo Kim, Roman Gushchin, Hyeonggon Yoo, Dmitry Vyukov, Andrey Konovalov, Kees Cook, Hansen, Dave, linux-mm, linux-kernel, kasan-dev On Wed, Nov 23, 2022 at 10:48:50AM +0100, Vlastimil Babka wrote: > On 11/21/22 07:38, Feng Tang wrote: > > On Fri, Nov 11, 2022 at 04:29:43PM +0800, Tang, Feng wrote: > >> On Fri, Nov 11, 2022 at 04:16:32PM +0800, Vlastimil Babka wrote: > >> > > for (shift = 3; shift <= 12; shift++) { > >> > > size = 1 << shift; > >> > > buf = kmalloc(size + 4, GFP_KERNEL); > >> > > /* We have 96, 196 kmalloc size, which is not power of 2 */ > >> > > if (size == 64 || size == 128) > >> > > oob_size = 16; > >> > > else > >> > > oob_size = size - 4; > >> > > memset(buf + size + 4, 0xee, oob_size); > >> > > kfree(buf); > >> > > } > >> > > >> > Sounds like a new slub_kunit test would be useful? :) doesn't need to be > >> > that exhaustive wrt all sizes, we could just pick one and check that a write > >> > beyond requested kmalloc size is detected? > >> > >> Just git-grepped out slub_kunit.c :), will try to add a case to it. > >> I'll also check if the case will also be caught by other sanitizer > >> tools like kasan/kfence etc. > > > > Just checked, kasan has already has API to disable kasan check > > temporarily, and I did see sometime kfence can chime in (4 out of 178 > > runs) so we need skip kfenced address. > > > > Here is the draft patch, thanks! > > > > From 45bf8d0072e532f43063dbda44c6bb3adcc388b6 Mon Sep 17 00:00:00 2001 > > From: Feng Tang <feng.tang@intel.com> > > Date: Mon, 21 Nov 2022 13:17:11 +0800 > > Subject: [PATCH] mm/slub, kunit: Add a case for kmalloc redzone functionality > > > > kmalloc redzone check for slub has been merged, and it's better to add > > a kunit case for it, which is inspired by a real-world case as described > > in commit 120ee599b5bf ("staging: octeon-usb: prevent memory corruption"): > > > > " > > octeon-hcd will crash the kernel when SLOB is used. This usually happens > > after the 18-byte control transfer when a device descriptor is read. > > The DMA engine is always transfering full 32-bit words and if the > > transfer is shorter, some random garbage appears after the buffer. > > The problem is not visible with SLUB since it rounds up the allocations > > to word boundary, and the extra bytes will go undetected. > > " > > Suggested-by: Vlastimil Babka <vbabka@suse.cz> > > Signed-off-by: Feng Tang <feng.tang@intel.com> > > --- > > lib/slub_kunit.c | 42 ++++++++++++++++++++++++++++++++++++++++++ > > mm/slab.h | 15 +++++++++++++++ > > mm/slub.c | 4 ++-- > > 3 files changed, 59 insertions(+), 2 deletions(-) > > > > diff --git a/lib/slub_kunit.c b/lib/slub_kunit.c > > index 7a0564d7cb7a..0653eed19bff 100644 > > --- a/lib/slub_kunit.c > > +++ b/lib/slub_kunit.c > > @@ -120,6 +120,47 @@ static void test_clobber_redzone_free(struct kunit *test) > > kmem_cache_destroy(s); > > } > > > > + > > +/* > > + * This case is simulating a real world case, that a device driver > > + * requests 18 bytes buffer, but the device HW has obligation to > > + * operate on 32 bits granularity, so it may actually read or write > > + * 20 bytes to the buffer, and possibly pollute 2 extra bytes after > > + * the requested space. > > + */ > > +static void test_kmalloc_redzone_access(struct kunit *test) > > +{ > > + u8 *p; > > + > > + if (!is_slub_debug_flags_enabled(SLAB_STORE_USER | SLAB_RED_ZONE)) > > + kunit_skip(test, "Test required SLAB_STORE_USER & SLAB_RED_ZONE flags on"); > > Hrmm, this is not great. I didn't realize that we're testing kmalloc() > specific code, so we can't simply create test-specific caches as in the > other kunit tests. > What if we did create a fake kmalloc cache with the necessary flags and used > it with kmalloc_trace() instead of kmalloc()? We would be bypassing the > kmalloc() inline layer so theoretically orig_size handling bugs could be > introduced there that the test wouldn't catch, but I think that's rather > unlikely. Importantly we would still be stressing the orig_size saving and > the adjusted redzone check using this info. Nice trick! Will go this way. > > + p = kmalloc(18, GFP_KERNEL); > > + > > +#ifdef CONFIG_KFENCE > > + { > > + int max_retry = 10; > > + > > + while (is_kfence_address(p) && max_retry--) { > > + kfree(p); > > + p = kmalloc(18, GFP_KERNEL); > > + } > > + > > + if (!max_retry) > > + kunit_skip(test, "Fail to get non-kfenced memory"); > > + } > > +#endif > > With the test-specific cache we could also pass SLAB_SKIP_KFENCE there to > handle this. Yep, the handling will be much simpler, thanks > > BTW, don't all slub kunit test need to do that in fact? Yes, I think they also need. With default kfence setting test, kence address wasn't hit in 250 times of boot test. And by changing CONFIG_KFENCE_NUM_OBJECTS from 255 to 16383, and CONFIG_KFENCE_SAMPLE_INTERVAL from 100 to 5, the kfence allocation did hit once in about 300 tims of boot test. Will add the flag bit for all kmem_cache creation. Thanks, Feng > Thanks, > Vlastimil ^ permalink raw reply [flat|nested] 20+ messages in thread
end of thread, other threads:[~2022-11-28 5:46 UTC | newest] Thread overview: 20+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-10-21 3:24 [PATCH v7 0/3] mm/slub: extend redzone check for kmalloc objects Feng Tang 2022-10-21 3:24 ` [PATCH v7 1/3] mm/slub: only zero requested size of buffer for kzalloc when debug enabled Feng Tang 2022-10-24 14:00 ` Hyeonggon Yoo 2022-10-27 19:27 ` Andrey Konovalov 2022-11-09 14:28 ` Vlastimil Babka 2022-11-10 3:20 ` Feng Tang 2022-11-10 12:57 ` Feng Tang 2022-11-10 15:44 ` Vlastimil Babka 2022-11-11 6:19 ` Feng Tang 2022-10-21 3:24 ` [PATCH v7 2/3] mm: kasan: Extend kasan_metadata_size() to also cover in-object size Feng Tang 2022-10-27 19:27 ` Andrey Konovalov 2022-10-21 3:24 ` [PATCH v7 3/3] mm/slub: extend redzone check to extra allocated kmalloc space than requested Feng Tang 2022-11-10 15:48 ` Vlastimil Babka 2022-11-11 6:46 ` Feng Tang 2022-11-11 8:12 ` Vlastimil Babka 2022-11-11 8:16 ` [PATCH v7 0/3] mm/slub: extend redzone check for kmalloc objects Vlastimil Babka 2022-11-11 8:29 ` Feng Tang 2022-11-21 6:38 ` Feng Tang 2022-11-23 9:48 ` Vlastimil Babka 2022-11-28 5:43 ` Feng Tang
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.