| Message ID | 20230724135327.1173309-1-arnd@kernel.org |
|---|---|
| State | New |
| Headers | show |
| Series | [1/2] crypto: drivers - avoid memcpy size warning | expand |
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 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