kref: Avoid null pointer dereference after WARN
PATCH v2 1/1 kref: Avoid null pointer dereference after WARN
From: Daniel Micay <[email protected]>
The WARN_ON() checking for a NULL release pointer was (sensibly) removed in commit ec48c940da6c ("kref: remove WARN_ON for NULL release functions") since it offered no protection at all about calling a NULL release pointer. However, it should instead be a BUG() since continuing with a NULL release pointer will lead to a NULL pointer execution anyway. Systems with an incorrectly set mmap_min_addr and no PXN/SMEP protection would be left open to executing userspace memory.
The kref_put() case is extracted from PaX, and Kees Cook noted it should be extended to the other two cases.
Comparison of my build with WARN, with nothing, and with BUG:
text data bss dec hex filename
11300251 5586597 13955072 30841920 1d69c40 vmlinux.warn 11298136 5586597 13955072 30839805 1d693fd vmlinux.none 11300062 5586629 13955072 30841763 1d69ba3 vmlinux.bug
--- include/linux/kref.h | 3 +++ 1 file changed, 3 insertions(+)
include/linux/kref.h +3 -0
@@ -67,6 +67,7 @@ static inline void kref_get(struct kref *kref)static inline int kref_put (struct kref *kref,void (*release)(struct kref *kref)) {if (refcount_dec_and_test (&kref->refcount)) { +BUG_ON (release ==NULL );release (kref);return 1 ; }
@@ -78,6 +79,7 @@ static inline int kref_put_mutex(struct kref *kref, struct mutex *lock) {if (refcount_dec_and_mutex_lock (&kref->refcount, lock)) { +BUG_ON (release ==NULL );release (kref);return 1 ; }
@@ -89,6 +91,7 @@ static inline int kref_put_lock(struct kref *kref, spinlock_t *lock) {if (refcount_dec_and_lock (&kref->refcount, lock)) { +BUG_ON (release ==NULL );release (kref);return 1 ; }
Signature · 6 lines
2.7.4 -- Kees Cook Pixel Security
Quoted 57 lines · Kees Cook
From: Daniel Micay <[email protected]>
The
WARN_ON()checking for a NULL release pointer was (sensibly) removed in commitec48c940da6c("kref: removeWARN_ONfor NULL release functions") since it offered no protection at all about calling a NULL release pointer. However, it should instead be aBUG()since continuing with a NULL release pointer will lead to a NULL pointer execution anyway. Systems with an incorrectly setmmap_min_addrand no PXN/SMEP protection would be left open to executing userspace memory.The
kref_put()case is extracted from PaX, and Kees Cook noted it should be extended to the other two cases.Comparison of my build with WARN, with nothing, and with BUG:
text data bss dec hex filename
11300251 5586597 13955072 30841920 1d69c40 vmlinux.warn 11298136 5586597 13955072 30839805 1d693fd vmlinux.none 11300062 5586629 13955072 30841763 1d69ba3 vmlinux.bugSigned-off-by Daniel Micay <[email protected]>[kees: clarify commit log, refreshed diff, moved into if statement]Cc Andi Kleen <[email protected]>Signed-off-by Kees Cook <[email protected]>--- include/linux/kref.h | 3 +++ 1 file changed, 3 insertions(+)include/linux/kref.h +3
-1@@ -67,6 +67,7 @@ static inline void kref_get(struct kref *kref)static inline int kref_put (struct kref *kref,void (*release)(struct kref *kref)) {if (refcount_dec_and_test (&kref->refcount)) { +BUG_ON (release ==NULL );release (kref);return 1 ; }@@ -78,6 +79,7 @@ static inline int kref_put_mutex(struct kref *kref, struct mutex *lock) {if (refcount_dec_and_mutex_lock (&kref->refcount, lock)) { +BUG_ON (release ==NULL );release (kref);return 1 ; }@@ -89,6 +91,7 @@ static inline int kref_put_lock(struct kref *kref, spinlock_t *lock) {if (refcount_dec_and_lock (&kref->refcount, lock)) { +BUG_ON (release ==NULL );release (kref);return 1 ; } --
Quoted 1 line
2.7.4
Looks good to me.
Quoted 9 lines · Kees Cook
From: Daniel Micay <[email protected]>
The
WARN_ON()checking for a NULL release pointer was (sensibly) removed in commitec48c940da6c("kref: removeWARN_ONfor NULL release functions") since it offered no protection at all about calling a NULL release pointer. However, it should instead be aBUG()since continuing with a NULL release pointer will lead to a NULL pointer execution anyway. Systems with an incorrectly setmmap_min_addrand no PXN/SMEP protection would be left open to executing userspace memory.
There's still no evidence that actually would prevented anything exploitable?
Who would actually set mman_min_addr incorrectly? And of course near all modern systems have SMEP/SMAP.
Surviving minor problems is actually a feature, not a bug. Linux was always better than other Unixes here, which are typically far too panic happy.
If you really want it, I would rather add bug/panic_on_warn sysctl that does this for every warning but make it default to off.
That would actually cover more cases.
I could see panic_on_warn being moderately useful for debugging when crash dumps are enabled.
-Andi
Quoted 1 line · Andi Kleen
Who would actually set
mman_min_addrincorrectly?
Historically there have been quite a few bypasses of mmap_min_addr, actually. This is well-trodden ground.
Quoted 5 lines · Jason A. Donenfeld
Quoted 1 line · Andi Kleen
Who would actually set
mman_min_addrincorrectly?Historically there have been quite a few bypasses of
mmap_min_addr, actually. This is well-trodden ground.
Targeting things in /proc/sys via confused privileged helpers is extremely common. See Chrome OS pwn2own exploits (targetting modprobe sysctl), and plenty of others. Modern attack methodology is rarely a single-bug attack, but rather a chain of bugs, which may include producing or exploiting weak userspace configurations to soften the kernel.
Regardless, it's a fair point that checking this unconditionally is wasteful. Strangely this doesn't help:
- BUG_ON(release == NULL);
+ if (!__builtin_constant_p(release))
+ BUG_ON(release == NULL);
When nearly all callers pass a function directly:
...
drivers/block/rbd.c: kref_put(&spec->kref, rbd_spec_free); drivers/char/hw_random/core.c: kref_put(&rng->ref, cleanup_rng);
drivers/char/ipmi/ipmi_msghandler.c:kref_put(&e->intf->refcount, intf_free);
...
Hmmm
-Kees
Signature · 2 lines
Kees Cook Pixel Security
Quoted 3 lines
Targeting things in
/proc/sysvia confused privileged helpers is extremely common. See Chrome OS pwn2own exploits (targetting modprobe sysctl), and plenty of others. Modern attack methodology is rarely a
Ok.
Quoted 6 lines
single-bug attack, but rather a chain of bugs, which may include producing or exploiting weak userspace configurations to soften the kernel.
Regardless, it's a fair point that checking this unconditionally is wasteful. Strangely this doesn't help:
I doubt the runtime overhead is significant. The main issue I would see is that kernels crash more often.
But if the issue doesn't really happens that often it probably doesn't matter (apart from kernel size, which has been resolved)
Too bad we don't have kerneloops.org data left, so it's hard to check data on this.
However I still think you would get more leverage out of generic bug/panic_on_warn sysctls
-Andi
Quoted 28 lines · Kees Cook
Quoted 6 lines · Jason A. Donenfeld
Quoted 1 line · Andi Kleen
Who would actually set
mman_min_addrincorrectly?Historically there have been quite a few bypasses of
mmap_min_addr, actually. This is well-trodden ground.Targeting things in
/proc/sysvia confused privileged helpers is extremely common. See Chrome OS pwn2own exploits (targetting modprobe sysctl), and plenty of others. Modern attack methodology is rarely a single-bug attack, but rather a chain of bugs, which may include producing or exploiting weak userspace configurations to soften the kernel.Regardless, it's a fair point that checking this unconditionally is wasteful. Strangely this doesn't help:
(quoted diff) +2
-1- BUG_ON(release == NULL); + if (!__builtin_constant_p(release)) + BUG_ON(release == NULL);When nearly all callers pass a function directly:
...
drivers/block/rbd.c: kref_put(&spec->kref, rbd_spec_free); drivers/char/hw_random/core.c: kref_put(&rng->ref,
Quoted 8 lines
cleanup_rng);drivers/char/ipmi/ipmi_msghandler.c:kref_put(&e->intf->refcount, intf_free);
...Hmmm
-Kees
It doesn't mean the address is constant if there's a fixed function being passed to it. It's not known at compile-time and if the code can be relocated it's not known at link-time.
I don't personally care about checks like this but I split it out with some others just because it was there already.
Clang has a nullability attribute which is similar to nonnull but it doesn't cause UB when violated, so if GCC picked that up it could be added all over the place as an annotation on parameters to trigger warnings. There's a sanitizer for it, so it can be made to trap with -fsanitize=nullability -fsanitize-trap=nullability.