Messages in this thread | | | Date | Thu, 27 Jul 2023 08:46:00 +0300 | Subject | Re: [PATCH 1/2] crypto: drivers - avoid memcpy size warning | From | claudiu beznea <> |
| |
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
| |