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....
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:
- 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).
- 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".
- 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.