From 7929e180aa47a2692ad4f053afac2857d7198758 Mon Sep 17 00:00:00 2001 From: Siddhesh Poyarekar Date: Tue, 13 Dec 2022 16:54:36 -0500 Subject: [PATCH] Use dummy allocator to make accesses defined as per standard systemd uses malloc_usable_size() everywhere to use memory blocks obtained through malloc, but that is abuse since the malloc_usable_size() interface isn't meant for this kind of use, it is for diagnostics only. This is also why systemd behaviour is flaky when built with _FORTIFY_SOURCE. One way to make this more standard (and hence safer) is to, at every malloc_usable_size() call, also 'reallocate' the block so that the compiler can see the larger size. This is done through a dummy reallocator whose only purpose is to tell the compiler about the larger usable size, it doesn't do any actual reallocation. Florian Weimer pointed out that this doesn't solve the problem of an allocator potentially growing usable size at will, which will break the implicit assumption in systemd use that the value returned remains constant as long as the object is valid. The safest way to fix that is for systemd to step away from using malloc_usable_size() like this. Resolves #22801. --- src/basic/alloc-util.c | 4 ++++ src/basic/alloc-util.h | 38 ++++++++++++++++++++++++++++---------- 2 files changed, 32 insertions(+), 10 deletions(-) diff --git a/src/basic/alloc-util.c b/src/basic/alloc-util.c index b030f454b2f6e..6063943c88ab5 100644 --- a/src/basic/alloc-util.c +++ b/src/basic/alloc-util.c @@ -102,3 +102,7 @@ void* greedy_realloc0( return q; } + +void *expand_to_usable(void *ptr, size_t newsize _unused_) { + return ptr; +} diff --git a/src/basic/alloc-util.h b/src/basic/alloc-util.h index b38db7d4737d1..eb53aae6f3e14 100644 --- a/src/basic/alloc-util.h +++ b/src/basic/alloc-util.h @@ -2,6 +2,7 @@ #pragma once #include +#include #include #include #include @@ -184,17 +185,34 @@ void* greedy_realloc0(void **p, size_t need, size_t size); # define msan_unpoison(r, s) #endif -/* This returns the number of usable bytes in a malloc()ed region as per malloc_usable_size(), in a way that - * is compatible with _FORTIFY_SOURCES. If _FORTIFY_SOURCES is used many memory operations will take the - * object size as returned by __builtin_object_size() into account. Hence, let's return the smaller size of - * malloc_usable_size() and __builtin_object_size() here, so that we definitely operate in safe territory by - * both the compiler's and libc's standards. Note that __builtin_object_size() evaluates to SIZE_MAX if the - * size cannot be determined, hence the MIN() expression should be safe with dynamically sized memory, - * too. Moreover, when NULL is passed malloc_usable_size() is documented to return zero, and - * __builtin_object_size() returns SIZE_MAX too, hence we also return a sensible value of 0 in this corner - * case. */ +/* Dummy allocator to tell the compiler that the new size of p is newsize. The implementation returns the + * pointer as is; the only reason for its existence is as a conduit for the _alloc_ attribute. This cannot be + * a static inline because gcc then loses the attributes on the function. + * See: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=96503 */ +void *expand_to_usable(void *p, size_t newsize) _alloc_(2) _returns_nonnull_; + +static inline size_t malloc_sizeof_safe(void **xp) { + if (_unlikely_(!xp || !*xp)) + return 0; + + size_t sz = malloc_usable_size(*xp); + *xp = expand_to_usable(*xp, sz); + /* GCC doesn't see the _returns_nonnull_ when built with ubsan, so yet another hint to make it doubly + * clear that expand_to_usable won't return NULL. + * See: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=79265 */ + if (!*xp) + assert_not_reached(); + return sz; +} + +/* This returns the number of usable bytes in a malloc()ed region as per malloc_usable_size(), which may + * return a value larger than the size that was actually allocated. Access to that additional memory is + * discouraged because it violates the C standard; a compiler cannot see that this as valid. To help the + * compiler out, the MALLOC_SIZEOF_SAFE macro 'allocates' the usable size using a dummy allocator function + * expand_to_usable. There is a possibility of malloc_usable_size() returning different values during the + * lifetime of an object, which may cause problems, but the glibc allocator does not do that at the moment. */ #define MALLOC_SIZEOF_SAFE(x) \ - MIN(malloc_usable_size(x), __builtin_object_size(x, 0)) + malloc_sizeof_safe((void**) &__builtin_choose_expr(__builtin_constant_p(x), (void*) { NULL }, (x))) /* Inspired by ELEMENTSOF() but operates on malloc()'ed memory areas: typesafely returns the number of items * that fit into the specified memory block */