| Message ID | 20240201083259.1734865-1-elver@google.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | [-mm] stackdepot: do not use flex_array_size() in memcpy() | expand |
On Thu, 1 Feb 2024 at 09:35, Marco Elver <elver@google.com> wrote: > > Since 113a61863ecb ("Makefile: Enable -Wstringop-overflow globally") > string overflow checking is enabled by default. Unfortunately the > compiler still isn't smart enough to always see that the size will never > overflow. > > Specifically, in stackdepot, we have this before memcpy()'ing a > stacktrace: > > if (nr_entries > CONFIG_STACKDEPOT_MAX_FRAMES) > nr_entries = CONFIG_STACKDEPOT_MAX_FRAMES; > ... > memcpy(stack->entries, entries, flex_array_size(stack, entries, nr_entries)); > > Where 'entries' is an array of unsigned long, and STACKDEPOT_MAX_FRAMES > is 64 by default (configurable up to 256), thus the maximum size in > bytes (on 32-bit) would be 1024. For some reason the compiler (GCC > 13.2.0) assumes that an overflow may be possible and flex_array_size() > can return SIZE_MAX (4294967295 on 32-bit), resulting in this warning: > > In function 'depot_alloc_stack', > inlined from 'stack_depot_save_flags' at lib/stackdepot.c:688:4: > arch/x86/include/asm/string_32.h:150:25: error: '__builtin_memcpy' specified bound 4294967295 exceeds maximum object size 2147483647 [-Werror=stringop-overflow=] > 150 | #define memcpy(t, f, n) __builtin_memcpy(t, f, n) > | ^~~~~~~~~~~~~~~~~~~~~~~~~ > lib/stackdepot.c:459:9: note: in expansion of macro 'memcpy' > 459 | memcpy(stack->entries, entries, flex_array_size(stack, entries, nr_entries)); > | ^~~~~~ > cc1: all warnings being treated as errors > > Silence the false positive warning by inlining the multiplication > ourselves. > > Link: https://lore.kernel.org/all/20240201135747.18eca98e@canb.auug.org.au/ > Fixes: d869d3fb362c ("stackdepot: use variable size records for non-evictable entries") > Reported-by: Stephen Rothwell <sfr@canb.auug.org.au> > Signed-off-by: Marco Elver <elver@google.com> > Cc: Gustavo A. R. Silva <gustavoars@kernel.org> > Cc: Kees Cook <keescook@chromium.org> > --- > lib/stackdepot.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/lib/stackdepot.c b/lib/stackdepot.c > index 8f3b2c84ec2d..e6047f58ad62 100644 > --- a/lib/stackdepot.c > +++ b/lib/stackdepot.c > @@ -456,7 +456,7 @@ depot_alloc_stack(unsigned long *entries, int nr_entries, u32 hash, depot_flags_ Sigh, switching this 'int nr_entries' to 'unsigned int' also fixes it - please disregard this patch.
diff --git a/lib/stackdepot.c b/lib/stackdepot.c index 8f3b2c84ec2d..e6047f58ad62 100644 --- a/lib/stackdepot.c +++ b/lib/stackdepot.c @@ -456,7 +456,7 @@ depot_alloc_stack(unsigned long *entries, int nr_entries, u32 hash, depot_flags_ stack->hash = hash; stack->size = nr_entries; /* stack->handle is already filled in by depot_pop_free_pool(). */ - memcpy(stack->entries, entries, flex_array_size(stack, entries, nr_entries)); + memcpy(stack->entries, entries, nr_entries * sizeof(entries[0])); if (flags & STACK_DEPOT_FLAG_GET) { refcount_set(&stack->count, 1);
Since 113a61863ecb ("Makefile: Enable -Wstringop-overflow globally") string overflow checking is enabled by default. Unfortunately the compiler still isn't smart enough to always see that the size will never overflow. Specifically, in stackdepot, we have this before memcpy()'ing a stacktrace: if (nr_entries > CONFIG_STACKDEPOT_MAX_FRAMES) nr_entries = CONFIG_STACKDEPOT_MAX_FRAMES; ... memcpy(stack->entries, entries, flex_array_size(stack, entries, nr_entries)); Where 'entries' is an array of unsigned long, and STACKDEPOT_MAX_FRAMES is 64 by default (configurable up to 256), thus the maximum size in bytes (on 32-bit) would be 1024. For some reason the compiler (GCC 13.2.0) assumes that an overflow may be possible and flex_array_size() can return SIZE_MAX (4294967295 on 32-bit), resulting in this warning: In function 'depot_alloc_stack', inlined from 'stack_depot_save_flags' at lib/stackdepot.c:688:4: arch/x86/include/asm/string_32.h:150:25: error: '__builtin_memcpy' specified bound 4294967295 exceeds maximum object size 2147483647 [-Werror=stringop-overflow=] 150 | #define memcpy(t, f, n) __builtin_memcpy(t, f, n) | ^~~~~~~~~~~~~~~~~~~~~~~~~ lib/stackdepot.c:459:9: note: in expansion of macro 'memcpy' 459 | memcpy(stack->entries, entries, flex_array_size(stack, entries, nr_entries)); | ^~~~~~ cc1: all warnings being treated as errors Silence the false positive warning by inlining the multiplication ourselves. Link: https://lore.kernel.org/all/20240201135747.18eca98e@canb.auug.org.au/ Fixes: d869d3fb362c ("stackdepot: use variable size records for non-evictable entries") Reported-by: Stephen Rothwell <sfr@canb.auug.org.au> Signed-off-by: Marco Elver <elver@google.com> Cc: Gustavo A. R. Silva <gustavoars@kernel.org> Cc: Kees Cook <keescook@chromium.org> --- lib/stackdepot.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)