Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Mon, 28 Feb 2022 15:54:26 -0800
From:   Kees Cook <keescook@chromium.org>
To:     Marco Elver <elver@google.com>
Cc:     llvm@lists.linux.dev, Pekka Enberg <penberg@kernel.org>,
        David Rientjes <rientjes@google.com>,
        Joonsoo Kim <iamjoonsoo.kim@lge.com>,
        Andrew Morton <akpm@linux-foundation.org>,
        Vlastimil Babka <vbabka@suse.cz>, linux-mm@kvack.org,
        stable@vger.kernel.org,
        Greg Kroah-Hartman <gregkh@linuxfoundation.org>,
        "Rafael J. Wysocki" <rafael@kernel.org>,
        Christoph Lameter <cl@linux.com>,
        Nathan Chancellor <nathan@kernel.org>,
        Nick Desaulniers <ndesaulniers@google.com>,
        Daniel Micay <danielmicay@gmail.com>,
        linux-kernel@vger.kernel.org, linux-hardening@vger.kernel.org
Subject: Re: [PATCH] mm: Handle ksize() vs __alloc_size by forgetting size
Message-ID: <202202281516.19274C0@keescook>
References: <20220225221625.3531852-1-keescook@chromium.org>
 <CANpmjNOup5JCjRpRkhsF3Z+dPX6_MQE5u6WhnMit84c1TyRK+A@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CANpmjNOup5JCjRpRkhsF3Z+dPX6_MQE5u6WhnMit84c1TyRK+A@mail.gmail.com>
Precedence: bulk

On Mon, Feb 28, 2022 at 12:24:51PM +0100, Marco Elver wrote:
> On Fri, 25 Feb 2022 at 23:16, Kees Cook <keescook@chromium.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.
> > [...]
> > 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?

I would say "incompatible", but yes, the result in the same. The compiler
is doing exactly what we told it that it should do. Using __alloc_size
means it can assume (and enforce) the size of the allocation.

AFAICT, This became an issue due to:

	-fsanitize=bounds -fsanitize-undefined-trap-on-error

I would expect -Warray-bounds to warn about it, but since we don't have
-Warray-bounds enabled, this leads to a silent problem. (Note that I
and others been working very hard to get the code base well enough into
shape that we can enable -Warray-bounds.)

I have not, however, been able to reduce the issue to a minimal a test
case yet. This patch was proposed to see if Greg (or other Android folks)
could check it.

> 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.

Right. __alloc_size vs ksize() is the problem.

> 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/

I think it's -fsanitize-undefined-trap-on-error causing the "problem".
It seems to be doing exactly what it was told to do. :)

But again, I haven't got a good example, so maybe there is _also_ a
miscompilation happening?

> 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.

Right ... I would _hope_ the s-based access would be on _bar_ only. :P

> I can see a few options:
> 
> 1. Dropping __alloc_size.

Right; that's the big-hammer solution (and is what Greg has already done
in [1] for the _particular_ case, but there may be others).

> 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.

I want to do this for other reasons too, but yes, it looks a bit
finicky. And there are some plans to make this even MORE dynamic, but if
that happens, I think we could fall that condition back to "no
__alloc_size".

> 3. Teaching the compiler to drop *all* object sizes upon encountering a ksize().

size_t ksize(void *ptr) __just_kidding_about_the_alloc_size;

> 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.

Okay, so, using __alloc_size is currently very limited in scope, in the
sense that bounds checking against such an allocated pointer is only
within the resulting function (and inlining) bounds. This _has_ caught
flaws, though, so I remain interested in keeping it.

Using it within the same scope as with ksize(), however, is the
problem. Daniel suggests a reasonable solution here, which is "remove
ksize" (I'll call this #4), since what he points out is that the
users of ksize() follow a very specific pattern and don't benefit from
__alloc_size.

As I'd like to be expanding the scope of __alloc_size's effects even
more strong into runtime (via other users of the information like
__builtin_dynamic_object_size()), I think we need to either use the
patch I've proposed, make kmalloc have compile-time-visible buckets so
__alloc_size seems the "right" size, or remove ksize().

#4 seems maybe doable. Here's a slightly trimmed list:

arch/x86/kernel/cpu/microcode/amd.c:    memcpy(amd_ucode_patch, p->data, min_t(u32, ksize(p->data), PATCH_MAX_SIZE));
drivers/base/devres.c:  total_old_size = ksize(container_of(ptr, struct devres, data));
drivers/dma-buf/dma-resv.c:     list->shared_max = (ksize(list) - offsetof(typeof(*list), shared)) /
drivers/gpu/drm/drm_managed.c:  WARN_ON(dev + 1 > (struct drm_device *) (container + ksize(container)));
drivers/net/ethernet/intel/igb/igb_main.c:      } else if (size > ksize(q_vector)) {
drivers/net/ipa/gsi_trans.c:    pool->count = ksize(pool->base) / size;
drivers/net/wireless/intel/iwlwifi/mvm/scan.c:  memset(mvm->scan_cmd, 0, ksize(mvm->scan_cmd));
fs/btrfs/send.c:        p->buf_len = ksize(p->buf);
fs/coredump.c:  cn->size = ksize(corename);
include/linux/slab.h:size_t ksize(const void *objp);
kernel/bpf/verifier.c:  if (ksize(dst) < bytes) {
mm/mempool.c:           __check_element(pool, element, ksize(element));
mm/nommu.c:             return ksize(objp);
mm/slab_common.c:       ks = ksize(mem);
net/core/skbuff.c:      unsigned int size = frag_size ? : ksize(data);
net/core/skbuff.c:      osize = ksize(data);
net/core/skbuff.c:      size = SKB_WITH_OVERHEAD(ksize(data));
net/core/skbuff.c:      size = SKB_WITH_OVERHEAD(ksize(data));
net/core/skbuff.c:      size = SKB_WITH_OVERHEAD(ksize(data));
security/tomoyo/gc.c:   tomoyo_memory_used[TOMOYO_MEMORY_POLICY] -= ksize(ptr);
security/tomoyo/memory.c:               const size_t s = ksize(ptr);

I would really like to have #2, though, as I could use it for some
future kmalloc defenses that need compile-time sizing visibility.

-- 
Kees Cook