| Message ID | 20250321204105.1898507-4-kees@kernel.org (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | slab: Set freed variables to NULL by default | expand |
On Fri, Mar 21, 2025 at 9:41 PM Kees Cook <kees@kernel.org> wrote: > To defang a subset of "dangling pointer" use-after-free flaws[1], take the > address of any lvalues passed to kfree() and set them to NULL after > freeing. > > To do this manually, kfree_and_null() (and the "sensitive" variant) > are introduced. Unless callers of kfree() are allowed to rely on this behavior, we might want to have an option to use a poison value instead of NULL for this in debug builds. Hmm... in theory, we could have an object that points to its own type, like so: struct foo { struct foo *ptr; }; And if that was initialized like so: struct foo *obj = kmalloc(sizeof(struct foo)); obj->ptr = obj; Then the following is currently fine, but would turn into UAF with this patch: kfree(obj->ptr); That is a fairly contrived example; but maybe it would make sense to reorder the NULL assignment and the actual freeing to avoid this particular case? Another pattern that is theoretically currently fine but would be problematic after this would be if code continues to access a pointer after the pointed-to object has been freed, but just to check for NULL-ness to infer something about the state of the object containing the pointer, or to check pointer identity, or something like that. But I guess that's probably not very common? Something like: if (ptr) { mutex_lock(&some_lock); ... kfree(ptr); } ... if (ptr) { ... mutex_unlock(&some_lock); } But from scrolling through the kernel sources and glancing at a few hundred kfree() calls (not a lot compared to the ~40000 callsites we have), I haven't actually found any obvious instances like that. There is KMSAN test code that intentionally does a UAF, which might need to be adjusted to not hit a NULL deref instead. (And just an irrelevant sidenote: snd_gf1_dma_interrupt() has commented-out debugging code that would UAF the "block" variable if it was not commented out.)
On Sat, Mar 22, 2025 at 02:50:15AM +0100, Jann Horn wrote: > On Fri, Mar 21, 2025 at 9:41 PM Kees Cook <kees@kernel.org> wrote: > > To defang a subset of "dangling pointer" use-after-free flaws[1], take the > > address of any lvalues passed to kfree() and set them to NULL after > > freeing. > > > > To do this manually, kfree_and_null() (and the "sensitive" variant) > > are introduced. > > Unless callers of kfree() are allowed to rely on this behavior, we > might want to have an option to use a poison value instead of NULL for > this in debug builds. Sure -- we have many to choose from. Is there a specific one you think would be good? > > Hmm... in theory, we could have an object that points to its own type, like so: > > struct foo { > struct foo *ptr; > }; > > And if that was initialized like so: > > struct foo *obj = kmalloc(sizeof(struct foo)); > obj->ptr = obj; > > Then the following is currently fine, but would turn into UAF with this patch: > > kfree(obj->ptr); > > That is a fairly contrived example; but maybe it would make sense to > reorder the NULL assignment and the actual freeing to avoid this > particular case? Ew. Yeah. Reordering this looks okay, yes: diff --git a/include/linux/slab.h b/include/linux/slab.h index 3f8fb60963e3..8913b05eca33 100644 --- a/include/linux/slab.h +++ b/include/linux/slab.h @@ -473,13 +473,15 @@ extern atomic_t count_nulled; static inline void kfree_and_null(void **ptr) { - __kfree(*ptr); + void *addr = *ptr; *ptr = NULL; + __kfree(addr); } static inline void kfree_sensitive_and_null(void **ptr) { - __kfree_sensitive(*ptr); + void *addr = *ptr; *ptr = NULL; + __kfree_sensitive(addr); } #define __force_assignable(x) \ > Another pattern that is theoretically currently fine but would be > problematic after this would be if code continues to access a pointer > after the pointed-to object has been freed, but just to check for > NULL-ness to infer something about the state of the object containing > the pointer, or to check pointer identity, or something like that. But > I guess that's probably not very common? Something like: > > if (ptr) { > mutex_lock(&some_lock); > ... > kfree(ptr); > } > ... > if (ptr) { > ... > mutex_unlock(&some_lock); > } Yeah, this would be bad form IMO. But it's not impossible. This idea was mentioned on the Fediverse thread for this RFC too. https://fosstodon.org/@kees/114202495615591299 > But from scrolling through the kernel sources and glancing at a few > hundred kfree() calls (not a lot compared to the ~40000 callsites we > have), I haven't actually found any obvious instances like that. There > is KMSAN test code that intentionally does a UAF, which might need to > be adjusted to not hit a NULL deref instead. > (And just an irrelevant sidenote: snd_gf1_dma_interrupt() has > commented-out debugging code that would UAF the "block" variable if it > was not commented out.) Yeah, the heap LKDTM tests need to switch to __kfree() too. :) I'm going to see if I can build a sensible Coccinelle script to look for this. It's currently getting very confused by the may "for_each" helpers, but here's what I've got currently: @use_freed@ expression E, RHS; @@ ( kfree(E); ... when != E ? E = RHS | kfree(E); ... * E ) It is finding a few, though. For example: --- ./drivers/md/dm-ima.c +++ /tmp/nothing/drivers/md/dm-ima.c @@ -584,13 +584,11 @@ error: exit: kfree(md->ima.active_table.device_metadata); - if (md->ima.active_table.device_metadata != md->ima.inactive_table.device_metadata) kfree(md->ima.inactive_table.device_metadata);
diff --git a/include/linux/slab.h b/include/linux/slab.h index 3e807ccc8583..2717ad238fa2 100644 --- a/include/linux/slab.h +++ b/include/linux/slab.h @@ -465,11 +465,35 @@ void * __must_check krealloc_noprof(const void *objp, size_t new_size, gfp_t flags) __realloc_size(2); #define krealloc(...) alloc_hooks(krealloc_noprof(__VA_ARGS__)) -void kfree(const void *objp); -void kfree_sensitive(const void *objp); +void __kfree(const void *objp); +void __kfree_sensitive(const void *objp); size_t __ksize(const void *objp); -#define __kfree(x) kfree(x) +static inline void kfree_and_null(void **ptr) +{ + __kfree(*ptr); + *ptr = NULL; +} +static inline void kfree_sensitive_and_null(void **ptr) +{ + __kfree_sensitive(*ptr); + *ptr = NULL; +} + +#define __force_lvalue_expr(x) \ + __builtin_choose_expr(__is_lvalue(x), x, (void *){ NULL }) + +#define __free_and_null(__how, x) \ +({ \ + typeof(x) *__ptr = &(x); \ + __how ## _and_null((void **)__ptr); \ +}) +#define __free_and_maybe_null(__how, x) \ + __builtin_choose_expr(__is_lvalue(x), \ + __free_and_null(__how, __force_lvalue_expr(x)), \ + __kfree(x)) +#define kfree(x) __free_and_maybe_null(kfree, x) +#define kfree_sensitive(x) __free_and_maybe_null(kfree_sensitive, x) DEFINE_FREE(kfree, void *, if (!IS_ERR_OR_NULL(_T)) kfree(_T)) DEFINE_FREE(kfree_sensitive, void *, if (_T) kfree_sensitive(_T)) diff --git a/mm/slab_common.c b/mm/slab_common.c index 4030907b6b7d..9a82952ec266 100644 --- a/mm/slab_common.c +++ b/mm/slab_common.c @@ -1211,7 +1211,7 @@ module_init(slab_proc_init); #endif /* CONFIG_SLUB_DEBUG */ /** - * kfree_sensitive - Clear sensitive information in memory before freeing + * __kfree_sensitive - Clear sensitive information in memory before freeing * @p: object to free memory of * * The memory of the object @p points to is zeroed before freed. @@ -1221,7 +1221,7 @@ module_init(slab_proc_init); * deal bigger than the requested buffer size passed to kmalloc(). So be * careful when using this function in performance sensitive code. */ -void kfree_sensitive(const void *p) +void __kfree_sensitive(const void *p) { size_t ks; void *mem = (void *)p; @@ -1231,9 +1231,9 @@ void kfree_sensitive(const void *p) kasan_unpoison_range(mem, ks); memzero_explicit(mem, ks); } - kfree(mem); + __kfree(mem); } -EXPORT_SYMBOL(kfree_sensitive); +EXPORT_SYMBOL(__kfree_sensitive); size_t ksize(const void *objp) { diff --git a/mm/slub.c b/mm/slub.c index 1f50129dcfb3..38dd898667bf 100644 --- a/mm/slub.c +++ b/mm/slub.c @@ -4729,12 +4729,12 @@ static void free_large_kmalloc(struct folio *folio, void *object) } /** - * kfree - free previously allocated memory + * __kfree - free previously allocated memory * @object: pointer returned by kmalloc() or kmem_cache_alloc() * * If @object is NULL, no operation is performed. */ -void kfree(const void *object) +void __kfree(const void *object) { struct folio *folio; struct slab *slab; @@ -4756,7 +4756,7 @@ void kfree(const void *object) s = slab->slab_cache; slab_free(s, slab, x, _RET_IP_); } -EXPORT_SYMBOL(kfree); +EXPORT_SYMBOL(__kfree); static __always_inline __realloc_size(2) void * __do_krealloc(const void *p, size_t new_size, gfp_t flags)
To defang a subset of "dangling pointer" use-after-free flaws[1], take the address of any lvalues passed to kfree() and set them to NULL after freeing. To do this manually, kfree_and_null() (and the "sensitive" variant) are introduced. Link: https://github.com/KSPP/linux/issues/87 [1] Signed-off-by: Kees Cook <kees@kernel.org> --- Cc: Vlastimil Babka <vbabka@suse.cz> Cc: Christoph Lameter <cl@linux.com> Cc: Pekka Enberg <penberg@kernel.org> Cc: David Rientjes <rientjes@google.com> Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com> Cc: Andrew Morton <akpm@linux-foundation.org> Cc: Roman Gushchin <roman.gushchin@linux.dev> Cc: Hyeonggon Yoo <42.hyeyoo@gmail.com> Cc: linux-mm@kvack.org --- include/linux/slab.h | 30 +++++++++++++++++++++++++++--- mm/slab_common.c | 8 ++++---- mm/slub.c | 6 +++--- 3 files changed, 34 insertions(+), 10 deletions(-)