| Message ID | 20230724135327.1173309-1-arnd@kernel.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | [1/2] crypto: drivers - avoid memcpy size warning | expand |
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
On Mon, Jul 24, 2023 at 03:53:01PM +0200, 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: Please send me the configurations which triggers these warnings. As these are false positives, I'd like to enable them only on the configurations where they actually cause a problem. Thanks,
On Fri, Aug 4, 2023, at 10:16, Herbert Xu wrote: > On Mon, Jul 24, 2023 at 03:53:01PM +0200, 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: > > Please send me the configurations which triggers these warnings. > As these are false positives, I'd like to enable them only on the > configurations where they actually cause a problem. See https://pastebin.com/raw/ip3tfpJF for a config that triggers this on x86 with the chelsio and atmel drivers. The bcm driver is only available on arm64, so you won't hit that one here. I also see this with allmodconfig, as well as defconfig after enabling CONFIG_FORTIFY_SOURCE and the three crypto drivers. I see that commit df8fc4e934c12 ("kbuild: Enable -fstrict-flex-arrays=3") turned on the strict flex-array behavior that triggers the warning, so this did not show up until linux-6.5-rc1. In linux-next, I see no other code hit this warning after all my other patches for it got merged, regardless strict flex arrays. At the moment, -Wrestrict is completely disabled in all builds, so you have to add a patch to enable it in the build system, this is what I use locally to enable it at the W=1 level, though you can probably just replace the cc-disable-warning line with a -Wrestrict line. Arnd --- a/scripts/Makefile.extrawarn +++ b/scripts/Makefile.extrawarn @@ -49,9 +49,6 @@ KBUILD_CFLAGS += -Wno-pointer-sign # globally built with -Wcast-function-type. KBUILD_CFLAGS += $(call cc-option, -Wcast-function-type) -# Another good warning that we'll want to enable eventually -KBUILD_CFLAGS += $(call cc-disable-warning, restrict) - # The allocators already balk at large sizes, so silence the compiler # warnings for bounds checks involving those possible values. While # -Wno-alloc-size-larger-than would normally be used here, earlier versions @@ -93,6 +90,7 @@ export KBUILD_EXTRA_WARN ifneq ($(findstring 1, $(KBUILD_EXTRA_WARN)),) KBUILD_CFLAGS += -Wextra -Wunused -Wno-unused-parameter +KBUILD_CFLAGS += $(call cc-option, -Wrestrict) KBUILD_CFLAGS += -Wmissing-format-attribute KBUILD_CFLAGS += -Wold-style-definition KBUILD_CFLAGS += -Wmissing-include-dirs @@ -105,6 +103,7 @@ else # Some diagnostics enabled by default are noisy. # Suppress them by using -Wno... except for W=1. +KBUILD_CFLAGS += $(call cc-disable-warning, restrict) KBUILD_CFLAGS += $(call cc-disable-warning, packed-not-aligned) ifdef CONFIG_CC_IS_CLANG
On Mon, Aug 07, 2023 at 02:04:05PM +0200, Arnd Bergmann wrote: > > See https://pastebin.com/raw/ip3tfpJF for a config that triggers this > on x86 with the chelsio and atmel drivers. The bcm driver is only > available on arm64, so you won't hit that one here. I also > see this with allmodconfig, as well as defconfig after enabling > CONFIG_FORTIFY_SOURCE and the three crypto drivers. OK I can reproduce this now: In file included from ../include/linux/string.h:254, from ../arch/x86/include/asm/page_32.h:18, from ../arch/x86/include/asm/page.h:14, from ../arch/x86/include/asm/processor.h:20, from ../arch/x86/include/asm/timex.h:5, from ../include/linux/timex.h:67, from ../include/linux/time32.h:13, from ../include/linux/time.h:60, from ../include/linux/stat.h:19, from ../include/linux/module.h:13, 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 304 and 176 overlaps 1 or more bytes at offset 304 [-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); | ^~~~~~ ../include/linux/fortify-string.h:57:33: error: ‘__builtin_memcpy’ accessing 129 or more bytes at offsets 304 and 176 overlaps 1 or more bytes at offset 304 [-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); | ^~~~~~ cc1: all warnings being treated as errors But why are we turning these warnings on if they're giving completely bogus false positives like this? struct atmel_sha_hmac_ctx { struct atmel_sha_ctx base; struct atmel_sha_hmac_key hkey; u32 ipad[SHA512_BLOCK_SIZE / sizeof(u32)]; u32 opad[SHA512_BLOCK_SIZE / sizeof(u32)]; atmel_sha_fn_t resume; }; struct atmel_sha_hmac_ctx *hmac = crypto_ahash_ctx(tfm); size_t bs = ctx->block_size; memcpy(hmac->opad, hmac->ipad, bs); The block_size is set by the algorithm, you can easily grep for it in atmel-sha.c and the biggest one there is SHA512_BLOCK_SIZE, which is how big hmac->ipad/hmac->opad are. So logically this code is perfectly fine. There is no way for the compiler to know how big ctx->block_size is. So why do we expect it to make deductions on how big bs can be? This warning looks broken to me. It looks like there is already a solution to this though. Just use unsafe_memcpy and be done with it. Cheers,
On Fri, Aug 11, 2023, at 12:48, Herbert Xu wrote: > On Mon, Aug 07, 2023 at 02:04:05PM +0200, Arnd Bergmann wrote: > > struct atmel_sha_hmac_ctx *hmac = crypto_ahash_ctx(tfm); > size_t bs = ctx->block_size; > > memcpy(hmac->opad, hmac->ipad, bs); > > The block_size is set by the algorithm, you can easily grep for > it in atmel-sha.c and the biggest one there is SHA512_BLOCK_SIZE, > which is how big hmac->ipad/hmac->opad are. > > So logically this code is perfectly fine. Right, that was my conclusion as well. > There is no way for the compiler to know how big ctx->block_size is. > So why do we expect it to make deductions on how big bs can be? > > This warning looks broken to me. I'm still unsure how exactly it goes wrong here, but I suspect it works as designed and just happens to run into a case in these drivers that is just not that common. I also see that the kernel's memcpy() declaration is missing the "restrict" annotation, but the fortified version calls the __builtin_memcpy() variant that has an implicit prototype with those annotations. > It looks like there is already a solution to this though. Just use > unsafe_memcpy and be done with it. Fine with me, I'll send a new version doing that. Arnd
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