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