Re: [PATCH 4/5] slab: Set freed variables to NULL by default

[Kees Cook <kees@kernel.org>]

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);


-- 
Kees Cook