Re: [PATCH v4 5/5] mm/kasan: Add object validation in ksize()

Kees Cook
2019-06-27T16:07:12Z

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