Re: [PATCHv2] cpumask: work around false-postive stringop-overread errors

From: Nilay Shroff
Date: Fri Dec 06 2024 - 04:10:35 EST


Thank you Yuri for insightful comments! Please see my responses inline...

On 12/5/24 21:53, Yury Norov wrote:
> On Thu, Dec 05, 2024 at 06:04:09PM +0530, Nilay Shroff wrote:
>> While building the powerpc code using gcc 13, I came across following
>> errors generated for kernel/padata.c file:
>>
>> CC kernel/padata.o
>> In file included from ./include/linux/string.h:390,
>> from ./arch/powerpc/include/asm/paca.h:16,
>> from ./arch/powerpc/include/asm/current.h:13,
>> from ./include/linux/thread_info.h:23,
>> from ./include/asm-generic/preempt.h:5,
>> from ./arch/powerpc/include/generated/asm/preempt.h:1,
>> from ./include/linux/preempt.h:79,
>> from ./include/linux/spinlock.h:56,
>> from ./include/linux/swait.h:7,
>> from ./include/linux/completion.h:12,
>> from kernel/padata.c:14:
>> In function ‘bitmap_copy’,
>> inlined from ‘cpumask_copy’ at ./include/linux/cpumask.h:839:2,
>> inlined from ‘__padata_set_cpumasks’ at kernel/padata.c:730:2:
>> ./include/linux/fortify-string.h:114:33: error: ‘__builtin_memcpy’ reading between 257 and 536870904 bytes from a region of size 256 [-Werror=stringop-overread]
>> 114 | #define __underlying_memcpy __builtin_memcpy
>> | ^
>> ./include/linux/fortify-string.h:633:9: note: in expansion of macro ‘__underlying_memcpy’
>> 633 | __underlying_##op(p, q, __fortify_size); \
>> | ^~~~~~~~~~~~~
>> ./include/linux/fortify-string.h:678:26: note: in expansion of macro ‘__fortify_memcpy_chk’
>> 678 | #define memcpy(p, q, s) __fortify_memcpy_chk(p, q, s, \
>> | ^~~~~~~~~~~~~~~~~~~~
>> ./include/linux/bitmap.h:259:17: note: in expansion of macro ‘memcpy’
>> 259 | memcpy(dst, src, len);
>> | ^~~~~~
>> kernel/padata.c: In function ‘__padata_set_cpumasks’:
>> kernel/padata.c:713:48: note: source object ‘pcpumask’ of size [0, 256]
>> 713 | cpumask_var_t pcpumask,
>> | ~~~~~~~~~~~~~~^~~~~~~~
>> In function ‘bitmap_copy’,
>> inlined from ‘cpumask_copy’ at ./include/linux/cpumask.h:839:2,
>> inlined from ‘__padata_set_cpumasks’ at kernel/padata.c:730:2:
>> ./include/linux/fortify-string.h:114:33: error: ‘__builtin_memcpy’ reading between 257 and 536870904 bytes from a region of size 256 [-Werror=stringop-overread]
>> 114 | #define __underlying_memcpy __builtin_memcpy
>> | ^
>> ./include/linux/fortify-string.h:633:9: note: in expansion of macro ‘__underlying_memcpy’
>> 633 | __underlying_##op(p, q, __fortify_size); \
>> | ^~~~~~~~~~~~~
>> ./include/linux/fortify-string.h:678:26: note: in expansion of macro ‘__fortify_memcpy_chk’
>> 678 | #define memcpy(p, q, s) __fortify_memcpy_chk(p, q, s, \
>> | ^~~~~~~~~~~~~~~~~~~~
>> ./include/linux/bitmap.h:259:17: note: in expansion of macro ‘memcpy’
>> 259 | memcpy(dst, src, len);
>> | ^~~~~~
>> kernel/padata.c: In function ‘__padata_set_cpumasks’:
>> kernel/padata.c:713:48: note: source object ‘pcpumask’ of size [0, 256]
>> 713 | cpumask_var_t pcpumask,
>> | ~~~~~~~~~~~~~~^~~~~~~~
>>
>> Apparently, above errors only manifests with GCC 13.x and config option
>> CONFIG_FORTIFY_SOURCE. Furthermore, if I use gcc 11.x or gcc 12.x then I
>> don't encounter above errors. Prima facie, these errors appear to be false-
>
> If it works for pre-GCC13, and likely for clang, you shouldn't disable it
> for them. It should be enabled for CONFIG_FORTIFY_SOURCE=n, as well.
>
> Check config CC_NO_ARRAY_BOUNDS for an example of how versioned flags
> are implemented.
>

>> positive. Brian informed me that currently some efforts are underway by
>> GCC developers to emit more verbose information when GCC detects string
>> overflow errors and that might help to further narrow down the root cause
>> of this error.
>
> I'm 100% sure that Brian is a great person and his information is
> absolutely correct and complete. But this is just not how we write
> commit messages.
>
> Please avoid personal references, vague statements and news from
> the future.
>
Sure, I would do the needful for future patches.

>> So for now, silence these errors using -Wno-stringop-
>> overread gcc option while building kernel/padata.c file until we find the
>> root cause.
>
> You didn't provide any evidence that this warning is specific for padata.
>

Let me just show you the test matrix for the stringop-overread error:

ARCH PowerPC:
compiler CONFIG_FORTIFY_SOURCE -Werror=stringop-overread
gcc 11.x Y N
gcc 11.x N N
gcc 12.x Y N
gcc 12.x N N
gcc 13.x Y Y
gcc 13.x N N
clang 18.x Y N
clang 18.x N N

ARCH x86_64:
compiler CONFIG_FORTIFY_SOURCE -Werror=stringop-overread
gcc 11.x Y N
gcc 11.x N N
gcc 12.x Y N
gcc 12.x N N
gcc 13.x Y N
gcc 13.x N N
clang 18.x Y N
clang 18.x N N

>From the above matrix, we could see that the sringop-overread error is only encountered
when we use gcc 13 on PowerPC machine and the stringop-overread error is seen only when we
enable CONFIG_FORTIFY_SOURCE. Furthermore, so far I've only encountered this error while
compiling kernel/padata.c file.

> And indeed the subject states that this is a cpumasks-related warninig.
> Cpumask is a global subsystem. If you believe that this warning is
> false-positive, it may show up for any other random victim. Please
> suppress it globally.
>
Yes you were correct, this warning might appear for any other random victims. But as
I mentioned earlier, so far I've only encountered it with kernel/padata.c file.
So, if we want to reduce the blast radius then wouldn't it be sufficient to just silence
it only while compiling kernel/padata.c file? Or do you still suggest disabling it at
global level would be more helpful? I'm OK with either way moving forward. Please let
me know.

> Thanks,
> Yury
>
Thanks,
--Nilay

>>
>> Link: https://lore.kernel.org/all/7cbbd751-8332-4ab2-afa7-8c353834772a@xxxxxxxxxxxxx/
>> Cc: briannorris@xxxxxxxxxxxx
>> Cc: kees@xxxxxxxxxx
>> Cc: nathan@xxxxxxxxxx
>> Cc: yury.norov@xxxxxxxxx
>> Cc: steffen.klassert@xxxxxxxxxxx
>> Cc: daniel.m.jordan@xxxxxxxxxx
>> Cc: linux-crypto@xxxxxxxxxxxxxxx
>> Cc: linux@xxxxxxxxxxxxxx
>> Cc: gjoyce@xxxxxxx
>> Reviewed-by: Brian Norris <briannorris@xxxxxxxxxxxx>
>> Signed-off-by: Nilay Shroff <nilay@xxxxxxxxxxxxx>
>> ---
>> Changes from v1:
>> - Fix spell error in the commit message (Brian Norris)
>> - Add commentary around change to note that changes are needed to
>> avoid false positive on gcc 13+ (Brian Norris)
>> - Add the kerenl/padata.c file maintainers (Brian Norris)
>> ---
>>
>> kernel/Makefile | 4 ++++
>> 1 file changed, 4 insertions(+)
>>
>> diff --git a/kernel/Makefile b/kernel/Makefile
>> index 87866b037fbe..03242d8870c7 100644
>> --- a/kernel/Makefile
>> +++ b/kernel/Makefile
>> @@ -120,6 +120,10 @@ obj-$(CONFIG_CFI_CLANG) += cfi.o
>> obj-$(CONFIG_PERF_EVENTS) += events/
>>
>> obj-$(CONFIG_USER_RETURN_NOTIFIER) += user-return-notifier.o
>> +
>> +# Silence the false positive stringop-overread errors on GCC 13+
>> +CFLAGS_padata.o += $(call cc-disable-warning, stringop-overread)
>> +
>> obj-$(CONFIG_PADATA) += padata.o
>> obj-$(CONFIG_JUMP_LABEL) += jump_label.o
>> obj-$(CONFIG_CONTEXT_TRACKING) += context_tracking.o
>> --
>> 2.45.2