Messages in this thread | | | From | "Jason A. Donenfeld" <> | Date | Tue, 27 Jun 2017 21:15:13 +0200 | Subject | Re: [PATCH v2] kref: Avoid null pointer dereference after WARN |
| |
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>
| |