[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <CANpmjNNtLMX8cB-YS_u3TWq-v=2XFDwhQKi+SCoXKKSZf39qaw@mail.gmail.com>
Date: Thu, 1 Feb 2024 09:52:59 +0100
From: Marco Elver <elver@...gle.com>
To: elver@...gle.com, Andrew Morton <akpm@...ux-foundation.org>
Cc: Andrey Konovalov <andreyknvl@...il.com>, Alexander Potapenko <glider@...gle.com>,
Dmitry Vyukov <dvyukov@...gle.com>, Vlastimil Babka <vbabka@...e.cz>, linux-kernel@...r.kernel.org,
linux-mm@...ck.org, kasan-dev@...glegroups.com,
Stephen Rothwell <sfr@...b.auug.org.au>, "Gustavo A . R . Silva" <gustavoars@...nel.org>,
Kees Cook <keescook@...omium.org>
Subject: Re: [PATCH -mm] stackdepot: do not use flex_array_size() in memcpy()
On Thu, 1 Feb 2024 at 09:35, Marco Elver <elver@...gle.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@...b.auug.org.au>
> Signed-off-by: Marco Elver <elver@...gle.com>
> Cc: Gustavo A. R. Silva <gustavoars@...nel.org>
> Cc: Kees Cook <keescook@...omium.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.
Powered by blists - more mailing lists