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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <Z1HTdtvNjm-nZSvJ@yury-ThinkPad>
Date: Thu, 5 Dec 2024 08:23:18 -0800
From: Yury Norov <yury.norov@...il.com>
To: Nilay Shroff <nilay@...ux.ibm.com>
Cc: linux-kernel@...r.kernel.org, briannorris@...omium.org, kees@...nel.org,
	nathan@...nel.org, steffen.klassert@...unet.com,
	daniel.m.jordan@...cle.com, linux-crypto@...r.kernel.org,
	linux@...ssschuh.net, gjoyce@....com
Subject: Re: [PATCHv2] cpumask: work around false-postive stringop-overread
 errors

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.

> 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.

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.

Thanks,
Yury

> 
> Link: https://lore.kernel.org/all/7cbbd751-8332-4ab2-afa7-8c353834772a@linux.ibm.com/
> Cc: briannorris@...omium.org
> Cc: kees@...nel.org
> Cc: nathan@...nel.org
> Cc: yury.norov@...il.com
> Cc: steffen.klassert@...unet.com
> Cc: daniel.m.jordan@...cle.com
> Cc: linux-crypto@...r.kernel.org
> Cc: linux@...ssschuh.net
> Cc: gjoyce@....com
> Reviewed-by: Brian Norris <briannorris@...omium.org>
> Signed-off-by: Nilay Shroff <nilay@...ux.ibm.com>
> ---
> 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