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
Signed-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(+)
diff --git a/include/linux/kref.h b/include/linux/kref.h
index 29220724bf1c..651a12d2425f 100644
--- a/include/linux/kref.h
+++ b/include/linux/kref.h
@@ -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;
}
--
2.7.4
--
Kees Cook
Pixel Security
On Tue, Jun 27, 2017 at 9:00 PM, Kees Cook <[email protected]> wrote:
> 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
>
> Signed-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(+)
>
> diff --git a/include/linux/kref.h b/include/linux/kref.h
> index 29220724bf1c..651a12d2425f 100644
> --- a/include/linux/kref.h
> +++ b/include/linux/kref.h
> @@ -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;
> }
> --
> 2.7.4
Looks good to me.
Signed-off-by: Jason A. Donenfeld <[email protected]>
On Tue, Jun 27, 2017 at 12:00:02PM -0700, Kees Cook wrote:
> 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.
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
On Tue, Jun 27, 2017 at 9:22 PM, Andi Kleen <[email protected]> wrote:
> Who would actually set mman_min_addr incorrectly?
Historically there have been quite a few bypasses of mmap_min_addr,
actually. This is well-trodden ground.
On Tue, Jun 27, 2017 at 12:26 PM, Jason A. Donenfeld <[email protected]> wrote:
> On Tue, Jun 27, 2017 at 9:22 PM, Andi Kleen <[email protected]> wrote:
>> Who would actually set mman_min_addr incorrectly?
>
> 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
--
Kees Cook
Pixel Security
> 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
Ok.
> 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
On Tue, 2017-06-27 at 12:34 -0700, Kees Cook wrote:
> On Tue, Jun 27, 2017 at 12:26 PM, Jason A. Donenfeld <[email protected]>
> wrote:
> > On Tue, Jun 27, 2017 at 9:22 PM, Andi Kleen <[email protected]>
> > wrote:
> > > Who would actually set mman_min_addr incorrectly?
> >
> > 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
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.