Message-ID: <CANpmjNOup5JCjRpRkhsF3Z+dPX6_MQE5u6WhnMit84c1TyRK+A@mail.gmail.com>
Date:   Mon, 28 Feb 2022 12:24:51 +0100
From:   Marco Elver <elver@...gle.com>
To:     Kees Cook <keescook@...omium.org>
Cc:     llvm@...ts.linux.dev, Pekka Enberg <penberg@...nel.org>,
        David Rientjes <rientjes@...gle.com>,
        Joonsoo Kim <iamjoonsoo.kim@....com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Vlastimil Babka <vbabka@...e.cz>, linux-mm@...ck.org,
        stable@...r.kernel.org,
        Greg Kroah-Hartman <gregkh@...uxfoundation.org>,
        "Rafael J. Wysocki" <rafael@...nel.org>,
        Christoph Lameter <cl@...ux.com>,
        Nathan Chancellor <nathan@...nel.org>,
        Nick Desaulniers <ndesaulniers@...gle.com>,
        Daniel Micay <danielmicay@...il.com>,
        linux-kernel@...r.kernel.org, linux-hardening@...r.kernel.org
Subject: Re: [PATCH] mm: Handle ksize() vs __alloc_size by forgetting size

On Fri, 25 Feb 2022 at 23:16, Kees Cook <keescook@...omium.org> wrote:
>
> If ksize() is used on an allocation, the compiler cannot make any
> assumptions about its size any more (as hinted by __alloc_size). Force
> it to forget.
>
> One caller was using a container_of() construction that needed to be
> worked around.
>
> Cc: Marco Elver <elver@...gle.com>
> Cc: Pekka Enberg <penberg@...nel.org>
> Cc: David Rientjes <rientjes@...gle.com>
> Cc: Joonsoo Kim <iamjoonsoo.kim@....com>
> Cc: Andrew Morton <akpm@...ux-foundation.org>
> Cc: Vlastimil Babka <vbabka@...e.cz>
> Cc: linux-mm@...ck.org
> Link: https://github.com/ClangBuiltLinux/linux/issues/1599
> Fixes: c37495d6254c ("slab: add __alloc_size attributes for better bounds checking")
> Cc: stable@...r.kernel.org
> Signed-off-by: Kees Cook <keescook@...omium.org>
> ---
> This appears to work for me, but I'm waiting for more feedback on
> the specific instance got tripped over in Android.
> ---
>  drivers/base/devres.c |  4 +++-
>  include/linux/slab.h  | 26 +++++++++++++++++++++++++-
>  mm/slab_common.c      | 19 +++----------------
>  3 files changed, 31 insertions(+), 18 deletions(-)
>
> diff --git a/drivers/base/devres.c b/drivers/base/devres.c
> index eaa9a5cd1db9..1a2645bd7234 100644
> --- a/drivers/base/devres.c
> +++ b/drivers/base/devres.c
> @@ -855,6 +855,7 @@ void *devm_krealloc(struct device *dev, void *ptr, size_t new_size, gfp_t gfp)
>         size_t total_new_size, total_old_size;
>         struct devres *old_dr, *new_dr;
>         unsigned long flags;
> +       void *allocation;
>
>         if (unlikely(!new_size)) {
>                 devm_kfree(dev, ptr);
> @@ -874,7 +875,8 @@ void *devm_krealloc(struct device *dev, void *ptr, size_t new_size, gfp_t gfp)
>         if (!check_dr_size(new_size, &total_new_size))
>                 return NULL;
>
> -       total_old_size = ksize(container_of(ptr, struct devres, data));
> +       allocation = container_of(ptr, struct devres, data);
> +       total_old_size = ksize(allocation);
>         if (total_old_size == 0) {
>                 WARN(1, "Pointer doesn't point to dynamically allocated memory.");
>                 return NULL;
> diff --git a/include/linux/slab.h b/include/linux/slab.h
> index 37bde99b74af..a14f3bfa2f44 100644
> --- a/include/linux/slab.h
> +++ b/include/linux/slab.h
> @@ -182,8 +182,32 @@ int kmem_cache_shrink(struct kmem_cache *s);
>  void * __must_check krealloc(const void *objp, size_t new_size, gfp_t flags) __alloc_size(2);
>  void kfree(const void *objp);
>  void kfree_sensitive(const void *objp);
> +
> +/**
> + * ksize - get the actual amount of memory allocated for a given object
> + * @objp: Pointer to the object
> + *
> + * kmalloc may internally round up allocations and return more memory
> + * than requested. ksize() can be used to determine the actual amount of
> + * memory allocated. The caller may use this additional memory, even though
> + * a smaller amount of memory was initially specified with the kmalloc call.
> + * The caller must guarantee that objp points to a valid object previously
> + * allocated with either kmalloc() or kmem_cache_alloc(). The object
> + * must not be freed during the duration of the call.
> + *
> + * Return: size of the actual memory used by @objp in bytes
> + */
> +#define ksize(objp) ({                                                 \
> +       /*                                                              \
> +        * Getting the actual allocation size means the __alloc_size    \
> +        * hints are no longer valid, and the compiler needs to         \
> +        * forget about them.                                           \
> +        */                                                             \
> +       OPTIMIZER_HIDE_VAR(objp);                                       \
> +       _ksize(objp);                                                   \
> +})

So per that ClangBuiltLinux issue I'm gleaning that the __alloc_size
annotations are actually causing the compiler to generate wrong code?
Possibly due to the compiler thinking that the accesses must stay
within some bound, and anything beyond that will be "undefined
behaviour"? Clearly, per the slab APIs, in particular with the
provision of ksize(), the compiler is wrong.

At first I thought this was only related to UBSAN bounds checking
generating false positives, in which case a simple workaround as you
present above would probably take care of most cases.

But if the real issue is the compiler suddenly doing more aggressive
compiler optimizations because it thinks accesses beyond the object
size (per __alloc_size) is UB, but UB can never happen, and thus does
crazy things [1], I think the answer (at least with what we have right
now) should be to find a different solution that is more reliable.

[1] https://lore.kernel.org/all/20220218131358.3032912-1-gregkh@linuxfoundation.org/

Because who's to say that there's not some code that does:

   foo = kmalloc(...);
   ...
   bar = foo;
   s = ksize(bar);
   ... makes access address-dependent on 's' and 'foo' (but not 'bar') ...

This doesn't look like code anyone would write, but I fear with enough
macro and inline function magic, it's not too unlikely.

I can see a few options:

1. Dropping __alloc_size.
2. Somehow statically computing the size-class's size (kmalloc_index()
might help here), removing __alloc_size from allocation functions and
instead use some wrapper.
3. Teaching the compiler to drop *all* object sizes upon encountering a ksize().

So I think #1 is probably not what you want. #2 seems quite
complicated, and in many cases likely too relaxed and would miss bugs,
so also not ideal. #3 would be the most reliable, but
OPTIMIZER_HIDE_VAR() doesn't cut it, and we need something stronger.
The downside of #3 is that it might pessimize code generation, but
given ksize() is used sparingly, might be ok.

Thanks,
-- Marco

Powered by blists - more mailing lists