[PATCH 1/2] crypto: drivers

* [PATCH 1/2] crypto: drivers - avoid memcpy size warning
@ 2023-07-24 13:53 Arnd Bergmann
  2023-07-27  5:46 ` claudiu beznea
  2023-08-04  8:16 ` Herbert Xu
  0 siblings, 2 replies; 6+ messages in thread
From: Arnd Bergmann @ 2023-07-24 13:53 UTC (permalink / raw)
  To: Herbert Xu, David S. Miller, Nicolas Ferre, Alexandre Belloni,
	Claudiu Beznea, Ayush Sawal
  Cc: Arnd Bergmann, Yangtao Li, Sergiu Moga, Ryan Wanner,
	Gaosheng Cui, linux-crypto, linux-arm-kernel, linux-kernel

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(+)

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


^ permalink raw reply related	[flat|nested] 6+ messages in thread