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@xxxxxxxxxx> 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);
--
Kees Cook
