[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
Message-ID: <3202ac5b-a697-64b0-375c-d2ae2aa9ffdd@tuxon.dev>
Date: Thu, 27 Jul 2023 08:46:00 +0300
From: claudiu beznea <claudiu.beznea@...on.dev>
To: Arnd Bergmann <arnd@...nel.org>,
Herbert Xu <herbert@...dor.apana.org.au>,
"David S. Miller" <davem@...emloft.net>,
Nicolas Ferre <nicolas.ferre@...rochip.com>,
Alexandre Belloni <alexandre.belloni@...tlin.com>,
Claudiu Beznea <claudiu.beznea@...rochip.com>,
Ayush Sawal <ayush.sawal@...lsio.com>
Cc: Arnd Bergmann <arnd@...db.de>, Yangtao Li <frank.li@...o.com>,
Sergiu Moga <sergiu.moga@...rochip.com>,
Ryan Wanner <Ryan.Wanner@...rochip.com>,
Gaosheng Cui <cuigaosheng1@...wei.com>,
linux-crypto@...r.kernel.org, linux-arm-kernel@...ts.infradead.org,
linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] crypto: drivers - avoid memcpy size warning
On 24.07.2023 16:53, Arnd Bergmann wrote:
> From: Arnd Bergmann <arnd@...db.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@...db.de>
Reviewed-by: Claudiu Beznea <claudiu.beznea@...on.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
Powered by blists - more mailing lists