lkml.org 
[lkml]   [2023]   [Jul]   [27]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH 1/2] crypto: drivers - avoid memcpy size warning
From


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

\
 
 \ /
  Last update: 2023-07-27 07:48    [W:0.080 / U:0.948 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site