Re: [PATCH 1/2] crypto: drivers - avoid memcpy size warning

[Arnd Bergmann]

On Fri, Aug 4, 2023, at 10:16, Herbert Xu wrote:
> On Mon, Jul 24, 2023 at 03:53:01PM +0200, Arnd Bergmann wrote:
>> From: Arnd Bergmann <arnd@xxxxxxxx>
>> 
>> Some configurations with gcc-12 or gcc-13 produce a warning for the source
>> and destination of a memcpy() in atmel_sha_hmac_compute_ipad_hash() potentially
>> overlapping:
>> 
>> In file included from include/linux/string.h:254,
>>                  from drivers/crypto/atmel-sha.c:15:
>> drivers/crypto/atmel-sha.c: In function 'atmel_sha_hmac_compute_ipad_hash':
>> include/linux/fortify-string.h:57:33: error: '__builtin_memcpy' accessing 129 or more bytes at offsets 408 and 280 overlaps 1 or more bytes at offset 408 [-Werror=restrict]
>>    57 | #define __underlying_memcpy     __builtin_memcpy
>>       |                                 ^
>> include/linux/fortify-string.h:648:9: note: in expansion of macro '__underlying_memcpy'
>>   648 |         __underlying_##op(p, q, __fortify_size);                        \
>>       |         ^~~~~~~~~~~~~
>> include/linux/fortify-string.h:693:26: note: in expansion of macro '__fortify_memcpy_chk'
>>   693 | #define memcpy(p, q, s)  __fortify_memcpy_chk(p, q, s,                  \
>>       |                          ^~~~~~~~~~~~~~~~~~~~
>> drivers/crypto/atmel-sha.c:1773:9: note: in expansion of macro 'memcpy'
>>  1773 |         memcpy(hmac->opad, hmac->ipad, bs);
>>       |         ^~~~~~
>> 
>> The same thing happens in two more drivers that have the same logic:
>
> Please send me the configurations which triggers these warnings.
> As these are false positives, I'd like to enable them only on the
> configurations where they actually cause a problem.

See https://pastebin.com/raw/ip3tfpJF for a config that triggers this
on x86 with the chelsio and atmel drivers. The bcm driver is only
available on arm64, so you won't hit that one here. I also
see this with allmodconfig, as well as defconfig after enabling
CONFIG_FORTIFY_SOURCE and the three crypto drivers.

I see that commit df8fc4e934c12 ("kbuild: Enable -fstrict-flex-arrays=3")
turned on the strict flex-array behavior that triggers the
warning, so this did not show up until linux-6.5-rc1.
In linux-next, I see no other code hit this warning after all
my other patches for it got merged, regardless strict flex
arrays.

At the moment, -Wrestrict is completely disabled in all builds,
so you have to add a patch to enable it in the build system,
this is what I use locally to enable it at the W=1 level,
though you can probably just replace the cc-disable-warning
line with a -Wrestrict line.

     Arnd

--- a/scripts/Makefile.extrawarn
+++ b/scripts/Makefile.extrawarn
@@ -49,9 +49,6 @@ KBUILD_CFLAGS += -Wno-pointer-sign
 # globally built with -Wcast-function-type.
 KBUILD_CFLAGS += $(call cc-option, -Wcast-function-type)
 
-# Another good warning that we'll want to enable eventually
-KBUILD_CFLAGS += $(call cc-disable-warning, restrict)
-
 # The allocators already balk at large sizes, so silence the compiler
 # warnings for bounds checks involving those possible values. While
 # -Wno-alloc-size-larger-than would normally be used here, earlier versions
@@ -93,6 +90,7 @@ export KBUILD_EXTRA_WARN
 ifneq ($(findstring 1, $(KBUILD_EXTRA_WARN)),)
 
 KBUILD_CFLAGS += -Wextra -Wunused -Wno-unused-parameter
+KBUILD_CFLAGS += $(call cc-option, -Wrestrict)
 KBUILD_CFLAGS += -Wmissing-format-attribute
 KBUILD_CFLAGS += -Wold-style-definition
 KBUILD_CFLAGS += -Wmissing-include-dirs
@@ -105,6 +103,7 @@ else
 
 # Some diagnostics enabled by default are noisy.
 # Suppress them by using -Wno... except for W=1.
+KBUILD_CFLAGS += $(call cc-disable-warning, restrict)
 KBUILD_CFLAGS += $(call cc-disable-warning, packed-not-aligned)
 
 ifdef CONFIG_CC_IS_CLANG