On 12/8/24 19:27, Greg KH wrote: > On Sun, Dec 08, 2024 at 07:21:48PM +0530, Nilay Shroff wrote: >> >> >> On 12/8/24 18:58, Greg KH wrote: >>> On Sun, Dec 08, 2024 at 03:51:10PM +0530, Nilay Shroff wrote: >>>> >>>> >>>> On 12/7/24 17:14, Greg KH wrote: >>>>> On Sat, Dec 07, 2024 at 12:43:19PM +0100, Greg KH 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- >>>>>>> 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. So for now, silence these errors using -Wno-stringop- >>>>>>> overread gcc option while building kernel/padata.c file until we find the >>>>>>> root cause. >>>>>> >>>>>> I'm hitting this now on Linus's tree using gcc14 on x86-64 so this isn't >>>>>> just a problem with your arch. >>>> Thanks Greg for confirming that this is not isolated to PowerPC!! >>>>>> >>>>>> Let me try this patch locally and see if it helps... >>>>> >>>>> Yes, fixes the build for me, so either this is a real fix, or something >>>>> else needs to be done for it, so I'll give a: >>>>> >>>>> Acked-by: Greg Kroah-Hartman <gregkh@xxxxxxxxxxxxxxxxxxx> >>>>> >>>>> for now. >>>> Okay so now we've an evidence confirming that this is no longer PowerPC specific >>>> issue. Hence as Yury suggested, in another mail[1], fixing this error by disabling >>>> stringop-overread globally for GCC13+ and CONFIG_FORTIFY_SOURCE=n, I will spin a >>>> new patch and submit it. >>>> >>>> [1] https://lore.kernel.org/all/Z1HTdtvNjm-nZSvJ@yury-ThinkPad/ >>> >>> That feels wrong, unless this is a compiler bug. And if it's a compiler >>> bug, can we fix the compiler please or at least submit a bug to the gcc >>> developers? >>> >> Yes this seems to be a compiler bug. Please see here : >> [1] https://lore.kernel.org/all/202411021337.85E9BB06@keescook/ >> [2] https://gcc.gnu.org/pipermail/gcc-patches/2024-October/666872.html >> >>> I'm slowly moving all my boxes/builds over to using clang to build the >>> kernel due to rust kernel work, so I guess I can do that here as well as >>> I don't think this issue shows up for that compiler, right? >>> >> Yes this error doesn't show up for LLVM/clang. We've two options here: >> 1) To disable this error globally for GCC-13+ until we find the root cause. Maybe when >> GCC folks add more diagnostics and contexts around -Wstringop-* compiler warning as >> discussed in [2] above. >> or >> 2) Silence this error only for file kernel/padata.c compiling which this error manifests >> as of today. >> >> Yury suggested option #1 above so that we may avoid random victims hitting this error. >> What do you suggest? > > I suggest the hardening maintainers should decide, as this is their area > and feature they are supporting, not me :) > Alright, then I would go ahead with option #1, as Yury suggested, for now while I spin next patch and keep in Cc all hardening maintainers to decide the disposition. Thanks, --Nilay
