Received-SPF: pass (google.com: best guess record for domain of linux-kernel-owner@vger.kernel.org designates 209.132.180.67 as permitted sender) client-ip=209.132.180.67;
Date:   Thu, 27 Jun 2019 09:07:08 -0700
From:   Kees Cook <keescook@chromium.org>
To:     Marco Elver <elver@google.com>
Cc:     linux-kernel@vger.kernel.org,
        Andrey Ryabinin <aryabinin@virtuozzo.com>,
        Dmitry Vyukov <dvyukov@google.com>,
        Alexander Potapenko <glider@google.com>,
        Andrey Konovalov <andreyknvl@google.com>,
        Christoph Lameter <cl@linux.com>,
        Pekka Enberg <penberg@kernel.org>,
        David Rientjes <rientjes@google.com>,
        Joonsoo Kim <iamjoonsoo.kim@lge.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Mark Rutland <mark.rutland@arm.com>,
        kasan-dev@googlegroups.com, linux-mm@kvack.org
Subject: Re: [PATCH v4 5/5] mm/kasan: Add object validation in ksize()
Message-ID: <201906270906.9EE619600@keescook>
References: <20190627094445.216365-1-elver@google.com>
 <20190627094445.216365-6-elver@google.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20190627094445.216365-6-elver@google.com>
Sender: linux-kernel-owner@vger.kernel.org
Precedence: bulk

On Thu, Jun 27, 2019 at 11:44:45AM +0200, Marco Elver wrote:
> ksize() has been unconditionally unpoisoning the whole shadow memory region
> associated with an allocation. This can lead to various undetected bugs,
> for example, double-kzfree().
> 
> Specifically, kzfree() uses ksize() to determine the actual allocation
> size, and subsequently zeroes the memory. Since ksize() used to just
> unpoison the whole shadow memory region, no invalid free was detected.
> 
> This patch addresses this as follows:
> 
> 1. Add a check in ksize(), and only then unpoison the memory region.
> 
> 2. Preserve kasan_unpoison_slab() semantics by explicitly unpoisoning
>    the shadow memory region using the size obtained from __ksize().
> 
> Tested:
> 1. With SLAB allocator: a) normal boot without warnings; b) verified the
>    added double-kzfree() is detected.
> 2. With SLUB allocator: a) normal boot without warnings; b) verified the
>    added double-kzfree() is detected.
> 
> Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=199359
> Signed-off-by: Marco Elver <elver@google.com>

Acked-by: Kees Cook <keescook@chromium.org>

-Kees

> Cc: Andrey Ryabinin <aryabinin@virtuozzo.com>
> Cc: Dmitry Vyukov <dvyukov@google.com>
> Cc: Alexander Potapenko <glider@google.com>
> Cc: Andrey Konovalov <andreyknvl@google.com>
> Cc: Christoph Lameter <cl@linux.com>
> Cc: Pekka Enberg <penberg@kernel.org>
> Cc: David Rientjes <rientjes@google.com>
> Cc: Joonsoo Kim <iamjoonsoo.kim@lge.com>
> Cc: Andrew Morton <akpm@linux-foundation.org>
> Cc: Mark Rutland <mark.rutland@arm.com>
> Cc: Kees Cook <keescook@chromium.org>
> Cc: kasan-dev@googlegroups.com
> Cc: linux-kernel@vger.kernel.org
> Cc: linux-mm@kvack.org
> ---
> v4:
> * Prefer WARN_ON_ONCE() instead of BUG_ON().
> ---
>  include/linux/kasan.h |  7 +++++--
>  mm/slab_common.c      | 22 +++++++++++++++++++++-
>  2 files changed, 26 insertions(+), 3 deletions(-)
> 
> diff --git a/include/linux/kasan.h b/include/linux/kasan.h
> index b40ea104dd36..cc8a03cc9674 100644
> --- a/include/linux/kasan.h
> +++ b/include/linux/kasan.h
> @@ -76,8 +76,11 @@ void kasan_free_shadow(const struct vm_struct *vm);
>  int kasan_add_zero_shadow(void *start, unsigned long size);
>  void kasan_remove_zero_shadow(void *start, unsigned long size);
>  
> -size_t ksize(const void *);
> -static inline void kasan_unpoison_slab(const void *ptr) { ksize(ptr); }
> +size_t __ksize(const void *);
> +static inline void kasan_unpoison_slab(const void *ptr)
> +{
> +	kasan_unpoison_shadow(ptr, __ksize(ptr));
> +}
>  size_t kasan_metadata_size(struct kmem_cache *cache);
>  
>  bool kasan_save_enable_multi_shot(void);
> diff --git a/mm/slab_common.c b/mm/slab_common.c
> index b7c6a40e436a..a09bb10aa026 100644
> --- a/mm/slab_common.c
> +++ b/mm/slab_common.c
> @@ -1613,7 +1613,27 @@ EXPORT_SYMBOL(kzfree);
>   */
>  size_t ksize(const void *objp)
>  {
> -	size_t size = __ksize(objp);
> +	size_t size;
> +
> +	if (WARN_ON_ONCE(!objp))
> +		return 0;
> +	/*
> +	 * We need to check that the pointed to object is valid, and only then
> +	 * unpoison the shadow memory below. We use __kasan_check_read(), to
> +	 * generate a more useful report at the time ksize() is called (rather
> +	 * than later where behaviour is undefined due to potential
> +	 * use-after-free or double-free).
> +	 *
> +	 * If the pointed to memory is invalid we return 0, to avoid users of
> +	 * ksize() writing to and potentially corrupting the memory region.
> +	 *
> +	 * We want to perform the check before __ksize(), to avoid potentially
> +	 * crashing in __ksize() due to accessing invalid metadata.
> +	 */
> +	if (unlikely(objp == ZERO_SIZE_PTR) || !__kasan_check_read(objp, 1))
> +		return 0;
> +
> +	size = __ksize(objp);
>  	/*
>  	 * We assume that ksize callers could use whole allocated area,
>  	 * so we need to unpoison this area.
> -- 
> 2.22.0.410.gd8fdbe21b5-goog
> 

-- 
Kees Cook