| Message ID | 20220913065423.520159-3-feng.tang@intel.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | mm/slub: some debug enhancements for kmalloc | expand |
On Tue, Sep 13, 2022 at 8:54 AM Feng Tang <feng.tang@intel.com> wrote: > Hi Feng, > 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 sanity check could be added to > the extra space later. I still don't like the idea of only zeroing the requested memory and not the whole object. Considering potential info-leak vulnerabilities. Can we only do this when SLAB_DEBUG is enabled? > Performance wise, smaller zeroing length also brings shorter > execution time, as shown from test data on various server/desktop > platforms. > > For kzalloc users who will call ksize() later and utilize this > extra space, please be aware that the space is not zeroed any > more. CC Kees > > Signed-off-by: Feng Tang <feng.tang@intel.com> > --- > mm/slab.c | 7 ++++--- > mm/slab.h | 5 +++-- > mm/slub.c | 10 +++++++--- > 3 files changed, 14 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 d0ef9dd44b71..3cf5adf63f48 100644 > --- a/mm/slab.h > +++ b/mm/slab.h > @@ -730,7 +730,8 @@ 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) > { > size_t i; > > @@ -746,7 +747,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, orig_size); Note that when KASAN is enabled and has integrated init, it will initialize the whole object, which leads to an inconsistency with this change. > kmemleak_alloc_recursive(p[i], s->object_size, 1, > s->flags, flags); > } > diff --git a/mm/slub.c b/mm/slub.c > index c8ba16b3a4db..6f823e99d8b4 100644 > --- a/mm/slub.c > +++ b/mm/slub.c > @@ -3376,7 +3376,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; > } > @@ -3833,11 +3837,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 >
On Mon, Sep 26, 2022 at 09:11:24PM +0200, Andrey Konovalov wrote: > On Tue, Sep 13, 2022 at 8:54 AM Feng Tang <feng.tang@intel.com> wrote: > > > > Hi Feng, > > > 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 sanity check could be added to > > the extra space later. > > I still don't like the idea of only zeroing the requested memory and > not the whole object. Considering potential info-leak vulnerabilities. I really really do not like reducing the zeroing size. We're trying to be proactive against _flaws_, which means that when there's a memory over-read (or uninitialized use), suddenly the scope of the exposure (or control) is wider/looser. Imagine the (unfortunately very common) case of use-after-free attacks, which leverage type confusion: some object is located in kmalloc-128 because it's 126 bytes. That slot gets freed and reallocated to, say, a 97 byte object going through kzalloc() or zero-on-init. With this patch the bytes above the 97 don't get zeroed, and the stale data from the prior 126 byte object say there happily to be used again later through a dangling pointer, or whatever. Without the proposed patch, the entire 128 bytes is wiped, which makes stale data re-use more difficult. > > Performance wise, smaller zeroing length also brings shorter > > execution time, as shown from test data on various server/desktop > > platforms. For these cases, I think a much better solution is to provide those sensitive allocations their own dedicated kmem_cache. > > > > For kzalloc users who will call ksize() later and utilize this > > extra space, please be aware that the space is not zeroed any > > more. > > CC Kees Thanks! Well, the good news is that ksize() side-effects is hopefully going to vanish soon, but my objections about stale memory remain. -Kees
On Tue, Sep 27, 2022 at 04:15:02AM +0800, Kees Cook wrote: > On Mon, Sep 26, 2022 at 09:11:24PM +0200, Andrey Konovalov wrote: > > On Tue, Sep 13, 2022 at 8:54 AM Feng Tang <feng.tang@intel.com> wrote: > > > > > > > Hi Feng, > > > > > 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 sanity check could be added to > > > the extra space later. > > > > I still don't like the idea of only zeroing the requested memory and > > not the whole object. Considering potential info-leak vulnerabilities. > > I really really do not like reducing the zeroing size. We're trying to > be proactive against _flaws_, which means that when there's a memory > over-read (or uninitialized use), suddenly the scope of the exposure (or > control) is wider/looser. > > Imagine the (unfortunately very common) case of use-after-free attacks, > which leverage type confusion: some object is located in kmalloc-128 > because it's 126 bytes. That slot gets freed and reallocated to, say, a > 97 byte object going through kzalloc() or zero-on-init. With this patch > the bytes above the 97 don't get zeroed, and the stale data from the > prior 126 byte object say there happily to be used again later through > a dangling pointer, or whatever. Without the proposed patch, the entire > 128 bytes is wiped, which makes stale data re-use more difficult. Thanks for the details explaination, which is a valid concern. And Andrey's suggestion is a good solution: only reduce the zeroing size for kmalloc-redzone enabled objects, as the extra space will be redzoned, and no info will be leaked. Thanks, Feng
On Tue, Sep 27, 2022 at 03:11:24AM +0800, Andrey Konovalov wrote: > On Tue, Sep 13, 2022 at 8:54 AM Feng Tang <feng.tang@intel.com> wrote: > > > > Hi Feng, > > > 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 sanity check could be added to > > the extra space later. > > I still don't like the idea of only zeroing the requested memory and > not the whole object. Considering potential info-leak vulnerabilities. > > Can we only do this when SLAB_DEBUG is enabled? Good point! will add slub_debug_orig_size(s) check. > > Performance wise, smaller zeroing length also brings shorter > > execution time, as shown from test data on various server/desktop > > platforms. > > > > For kzalloc users who will call ksize() later and utilize this > > extra space, please be aware that the space is not zeroed any > > more. > > CC Kees Thanks for adding Kees, who provided review from security point of review. > > > > Signed-off-by: Feng Tang <feng.tang@intel.com> > > --- > > mm/slab.c | 7 ++++--- > > mm/slab.h | 5 +++-- > > mm/slub.c | 10 +++++++--- [...] > > @@ -730,7 +730,8 @@ 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) > > { > > size_t i; > > > > @@ -746,7 +747,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, orig_size); > > Note that when KASAN is enabled and has integrated init, it will > initialize the whole object, which leads to an inconsistency with this > change. Do you mean for kzalloc() only? or there is some kasan check newly added? I'm not familiar with kasan code, and during development, I usually enabled KASAN and KFENCE configs and did catch some bugs, while 0Day bot also reported some. And with latest v6 patchset, I haven't seen kasan/kfence failed cases. And for generic slub objects, when slub_debug is enabled, the object data area could be already modified like in init_object() if (s->flags & __OBJECT_POISON) { memset(p, POISON_FREE, s->object_size - 1); p[s->object_size - 1] = POISON_END; } slub-redzone check actually splitis it into 2 regions [0, orig_size-1], and [orig_size, object_size-1], and adds different sanity check to them. Anyway, I'll go check the latest linux-next tree. Thanks, Feng
On Tue, Sep 27, 2022 at 4:42 AM Feng Tang <feng.tang@intel.com> wrote: > > > > @@ -746,7 +747,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, orig_size); > > > > Note that when KASAN is enabled and has integrated init, it will > > initialize the whole object, which leads to an inconsistency with this > > change. > > Do you mean for kzalloc() only? or there is some kasan check newly added? Hi Feng, I mean that when init is true and kasan_has_integrated_init() is true (with HW_TAGS mode), kasan_slab_alloc() initializes the whole object. Which is inconsistent with the memset() of only orig_size when !kasan_has_integrated_init(). But I think this is fine assuming SLAB poisoning happens later. But please add a comment. Thanks!
On Thu, Oct 13, 2022 at 10:00:57PM +0800, Andrey Konovalov wrote: > On Tue, Sep 27, 2022 at 4:42 AM Feng Tang <feng.tang@intel.com> wrote: > > > > > > @@ -746,7 +747,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, orig_size); > > > > > > Note that when KASAN is enabled and has integrated init, it will > > > initialize the whole object, which leads to an inconsistency with this > > > change. > > > > Do you mean for kzalloc() only? or there is some kasan check newly added? > > Hi Feng, > > I mean that when init is true and kasan_has_integrated_init() is true > (with HW_TAGS mode), kasan_slab_alloc() initializes the whole object. > Which is inconsistent with the memset() of only orig_size when > !kasan_has_integrated_init(). But I think this is fine assuming SLAB > poisoning happens later. But please add a comment. I see now. Will add some comment. thanks! - Feng > Thanks!
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 d0ef9dd44b71..3cf5adf63f48 100644 --- a/mm/slab.h +++ b/mm/slab.h @@ -730,7 +730,8 @@ 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) { size_t i; @@ -746,7 +747,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, orig_size); kmemleak_alloc_recursive(p[i], s->object_size, 1, s->flags, flags); } diff --git a/mm/slub.c b/mm/slub.c index c8ba16b3a4db..6f823e99d8b4 100644 --- a/mm/slub.c +++ b/mm/slub.c @@ -3376,7 +3376,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; } @@ -3833,11 +3837,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; }
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 sanity check could be added to the extra space later. Performance wise, smaller zeroing length also brings shorter execution time, as shown from test data on various server/desktop platforms. For kzalloc users who will call ksize() later and utilize this extra space, please be aware that the space is not zeroed any more. Signed-off-by: Feng Tang <feng.tang@intel.com> --- mm/slab.c | 7 ++++--- mm/slab.h | 5 +++-- mm/slub.c | 10 +++++++--- 3 files changed, 14 insertions(+), 8 deletions(-)