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

  From: Arnd Bergmann <arnd@arndb.de>

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:

drivers/crypto/chelsio/chcr_algo.c: In function 'chcr_ahash_setkey':
include/linux/fortify-string.h:57:33: error: '__builtin_memcpy' accessing 129 or more bytes at offsets 260 and 132 overlaps 1 or more bytes at offset 260 [-Werror=restrict]
drivers/crypto/bcm/cipher.c: In function 'ahash_hmac_setkey':
include/linux/fortify-string.h:57:33: error: '__builtin_memcpy' accessing between 129 and 4294967295 bytes at offsets 840 and 712 overlaps between 1 and 4294967167 bytes at offset 840 [-Werror=restrict]

I don't think it can actually happen because the size is strictly bounded
to the available block sizes, at most 128 bytes, though inlining decisions
could lead gcc to not see that.

Add an explicit size check to make sure gcc also sees this function is safe
regardless of inlining.

Note that the -Wrestrict warning is currently disabled by default, but it
would be nice to finally enable it, and these are the only false
postives that I see at the moment. There are 9 other crypto drivers that
also use an identical memcpy() but don't show up in randconfig build
warnings for me, presumably because of different inlining decisions.

Signed-off-by: Arnd Bergmann <arnd@arndb.de>
---
 drivers/crypto/atmel-sha.c         | 3 +++
 drivers/crypto/bcm/cipher.c        | 3 +++
 drivers/crypto/chelsio/chcr_algo.c | 3 +++
 3 files changed, 9 insertions(+)
  

On 24.07.2023 16:53, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> 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:
> 
> drivers/crypto/chelsio/chcr_algo.c: In function 'chcr_ahash_setkey':
> include/linux/fortify-string.h:57:33: error: '__builtin_memcpy' accessing 129 or more bytes at offsets 260 and 132 overlaps 1 or more bytes at offset 260 [-Werror=restrict]
> drivers/crypto/bcm/cipher.c: In function 'ahash_hmac_setkey':
> include/linux/fortify-string.h:57:33: error: '__builtin_memcpy' accessing between 129 and 4294967295 bytes at offsets 840 and 712 overlaps between 1 and 4294967167 bytes at offset 840 [-Werror=restrict]
> 
> I don't think it can actually happen because the size is strictly bounded
> to the available block sizes, at most 128 bytes, though inlining decisions
> could lead gcc to not see that.
> 
> Add an explicit size check to make sure gcc also sees this function is safe
> regardless of inlining.
> 
> Note that the -Wrestrict warning is currently disabled by default, but it
> would be nice to finally enable it, and these are the only false
> postives that I see at the moment. There are 9 other crypto drivers that
> also use an identical memcpy() but don't show up in randconfig build
> warnings for me, presumably because of different inlining decisions.
> 
> Signed-off-by: Arnd Bergmann <arnd@arndb.de>

Reviewed-by: Claudiu Beznea <claudiu.beznea@tuxon.dev> # atmel-sha

> ---
>   drivers/crypto/atmel-sha.c         | 3 +++
>   drivers/crypto/bcm/cipher.c        | 3 +++
>   drivers/crypto/chelsio/chcr_algo.c | 3 +++
>   3 files changed, 9 insertions(+)
> 
> diff --git a/drivers/crypto/atmel-sha.c b/drivers/crypto/atmel-sha.c
> index f2031f934be95..52a3c81b3a05a 100644
> --- a/drivers/crypto/atmel-sha.c
> +++ b/drivers/crypto/atmel-sha.c
> @@ -1770,6 +1770,9 @@ static int atmel_sha_hmac_compute_ipad_hash(struct atmel_sha_dev *dd)
>   	size_t bs = ctx->block_size;
>   	size_t i, num_words = bs / sizeof(u32);
>   
> +	if (bs > sizeof(hmac->opad))
> +		return -EINVAL;
> +
>   	memcpy(hmac->opad, hmac->ipad, bs);
>   	for (i = 0; i < num_words; ++i) {
>   		hmac->ipad[i] ^= 0x36363636;
> diff --git a/drivers/crypto/bcm/cipher.c b/drivers/crypto/bcm/cipher.c
> index 70b911baab26d..8633ca0286a10 100644
> --- a/drivers/crypto/bcm/cipher.c
> +++ b/drivers/crypto/bcm/cipher.c
> @@ -2327,6 +2327,9 @@ static int ahash_hmac_setkey(struct crypto_ahash *ahash, const u8 *key,
>   		 __func__, ahash, key, keylen, blocksize, digestsize);
>   	flow_dump("  key: ", key, keylen);
>   
> +	if (blocksize > sizeof(ctx->opad))
> +		return -EINVAL;
> +
>   	if (keylen > blocksize) {
>   		switch (ctx->auth.alg) {
>   		case HASH_ALG_MD5:
> diff --git a/drivers/crypto/chelsio/chcr_algo.c b/drivers/crypto/chelsio/chcr_algo.c
> index 0eade4fa6695b..5c8e10ee010ff 100644
> --- a/drivers/crypto/chelsio/chcr_algo.c
> +++ b/drivers/crypto/chelsio/chcr_algo.c
> @@ -2201,6 +2201,9 @@ static int chcr_ahash_setkey(struct crypto_ahash *tfm, const u8 *key,
>   
>   	SHASH_DESC_ON_STACK(shash, hmacctx->base_hash);
>   
> +	if (bs > sizeof(hmacctx->opad))
> +		return -EINVAL;
> +
>   	/* use the key to calculate the ipad and opad. ipad will sent with the
>   	 * first request's data. opad will be sent with the final hash result
>   	 * ipad in hmacctx->ipad and opad in hmacctx->opad location
  

On Mon, Jul 24, 2023 at 03:53:01PM +0200, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@arndb.de>
> 
> 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.

Thanks,
  

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@arndb.de>
>> 
>> 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
  

On Mon, Aug 07, 2023 at 02:04:05PM +0200, Arnd Bergmann wrote:
>
> 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.

OK I can reproduce this now:

In file included from ../include/linux/string.h:254,
                 from ../arch/x86/include/asm/page_32.h:18,
                 from ../arch/x86/include/asm/page.h:14,
                 from ../arch/x86/include/asm/processor.h:20,
                 from ../arch/x86/include/asm/timex.h:5,
                 from ../include/linux/timex.h:67,
                 from ../include/linux/time32.h:13,
                 from ../include/linux/time.h:60,
                 from ../include/linux/stat.h:19,
                 from ../include/linux/module.h:13,
                 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 304 and 176 overlaps 1 or more bytes at offset 304 [-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);
      |         ^~~~~~
../include/linux/fortify-string.h:57:33: error: ‘__builtin_memcpy’ accessing 129 or more bytes at offsets 304 and 176 overlaps 1 or more bytes at offset 304 [-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);
      |         ^~~~~~
cc1: all warnings being treated as errors

But why are we turning these warnings on if they're giving completely
bogus false positives like this?

	struct atmel_sha_hmac_ctx {
		struct atmel_sha_ctx	base;

		struct atmel_sha_hmac_key	hkey;
		u32			ipad[SHA512_BLOCK_SIZE / sizeof(u32)];
		u32			opad[SHA512_BLOCK_SIZE / sizeof(u32)];
		atmel_sha_fn_t		resume;
	};

	struct atmel_sha_hmac_ctx *hmac = crypto_ahash_ctx(tfm);
	size_t bs = ctx->block_size;

	memcpy(hmac->opad, hmac->ipad, bs);

The block_size is set by the algorithm, you can easily grep for
it in atmel-sha.c and the biggest one there is SHA512_BLOCK_SIZE,
which is how big hmac->ipad/hmac->opad are.

So logically this code is perfectly fine.

There is no way for the compiler to know how big ctx->block_size is.
So why do we expect it to make deductions on how big bs can be?

This warning looks broken to me.

It looks like there is already a solution to this though.  Just use
unsafe_memcpy and be done with it.

Cheers,
  

On Fri, Aug 11, 2023, at 12:48, Herbert Xu wrote:
> On Mon, Aug 07, 2023 at 02:04:05PM +0200, Arnd Bergmann wrote:

>
> 	struct atmel_sha_hmac_ctx *hmac = crypto_ahash_ctx(tfm);
> 	size_t bs = ctx->block_size;
>
> 	memcpy(hmac->opad, hmac->ipad, bs);
>
> The block_size is set by the algorithm, you can easily grep for
> it in atmel-sha.c and the biggest one there is SHA512_BLOCK_SIZE,
> which is how big hmac->ipad/hmac->opad are.
>
> So logically this code is perfectly fine.

Right, that was my conclusion as well.

> There is no way for the compiler to know how big ctx->block_size is.
> So why do we expect it to make deductions on how big bs can be?
>
> This warning looks broken to me.

I'm still unsure how exactly it goes wrong here, but I suspect
it works as designed and just happens to run into a case in these
drivers that is just not that common. I also see that the kernel's
memcpy() declaration is missing the "restrict" annotation,
but the fortified version calls the __builtin_memcpy() variant
that has an implicit prototype with those annotations.

> It looks like there is already a solution to this though.  Just use
> unsafe_memcpy and be done with it.

Fine with me, I'll send a new version doing that.

     Arnd