linux-kernel - Re: [PATCH 1/2] crypto: drivers

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]
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