Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1676091 > unrolled thread

[PATCH v2] kref: Avoid null pointer dereference after WARN

Back to article view | Back to linux.kernel

Contents

  [PATCH v2] kref: Avoid null pointer dereference after WARN Kees Cook <keescook@chromium.org> - 2017-06-27 21:10 +0200
    Re: [PATCH v2] kref: Avoid null pointer dereference after WARN "Jason A. Donenfeld" <Jason@zx2c4.com> - 2017-06-27 21:20 +0200
    Re: [PATCH v2] kref: Avoid null pointer dereference after WARN Andi Kleen <ak@linux.intel.com> - 2017-06-27 21:30 +0200
      Re: [PATCH v2] kref: Avoid null pointer dereference after WARN "Jason A. Donenfeld" <Jason@zx2c4.com> - 2017-06-27 21:30 +0200
        Re: [PATCH v2] kref: Avoid null pointer dereference after WARN Kees Cook <keescook@chromium.org> - 2017-06-27 21:40 +0200
          Re: [PATCH v2] kref: Avoid null pointer dereference after WARN Andi Kleen <ak@linux.intel.com> - 2017-06-27 21:50 +0200
          Re: [PATCH v2] kref: Avoid null pointer dereference after WARN Daniel Micay <danielmicay@gmail.com> - 2017-06-27 22:20 +0200

#1676091 — [PATCH v2] kref: Avoid null pointer dereference after WARN

From: Daniel Micay <danielmicay@gmail.com>

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 <danielmicay@gmail.com>
[kees: clarify commit log, refreshed diff, moved into if statement]
Cc: Andi Kleen <ak@linux.intel.com>
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 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

[toc] | [next] | [standalone]

#1676094

On Tue, Jun 27, 2017 at 9:00 PM, Kees Cook <keescook@chromium.org> wrote:
> From: Daniel Micay <danielmicay@gmail.com>
>
> 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 <danielmicay@gmail.com>
> [kees: clarify commit log, refreshed diff, moved into if statement]
> Cc: Andi Kleen <ak@linux.intel.com>
> Signed-off-by: Kees Cook <keescook@chromium.org>
> ---
>  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 <Jason@zx2c4.com>

[toc] | [prev] | [next] | [standalone]

#1676099

On Tue, Jun 27, 2017 at 12:00:02PM -0700, Kees Cook wrote:
> From: Daniel Micay <danielmicay@gmail.com>
> 
> 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

[toc] | [prev] | [next] | [standalone]

#1676104

On Tue, Jun 27, 2017 at 9:22 PM, Andi Kleen <ak@linux.intel.com> 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.

[toc] | [prev] | [next] | [standalone]

#1676106

On Tue, Jun 27, 2017 at 12:26 PM, Jason A. Donenfeld <Jason@zx2c4.com> wrote:
> On Tue, Jun 27, 2017 at 9:22 PM, Andi Kleen <ak@linux.intel.com> 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

[toc] | [prev] | [next] | [standalone]

#1676111

> 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

[toc] | [prev] | [next] | [standalone]

#1676153

On Tue, 2017-06-27 at 12:34 -0700, Kees Cook wrote:
> On Tue, Jun 27, 2017 at 12:26 PM, Jason A. Donenfeld <Jason@zx2c4.com>
> wrote:
> > On Tue, Jun 27, 2017 at 9:22 PM, Andi Kleen <ak@linux.intel.com>
> > 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.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web