| Message ID | 20221202010410.gonna.444-kees@kernel.org (mailing list archive) |
|---|---|
| State | Changes Requested |
| Delegated to: | Herbert Xu |
| Headers | show |
| Series | [v2] crypto/caam: Avoid GCC constprop bug warning | expand |
On Thu, Dec 01, 2022 at 05:04:14PM -0800, Kees Cook wrote: > > diff --git a/drivers/crypto/caam/desc_constr.h b/drivers/crypto/caam/desc_constr.h > index 62ce6421bb3f..d9da4173af9d 100644 > --- a/drivers/crypto/caam/desc_constr.h > +++ b/drivers/crypto/caam/desc_constr.h > @@ -163,7 +163,8 @@ static inline void append_data(u32 * const desc, const void *data, int len) > { > u32 *offset = desc_end(desc); > > - if (len) /* avoid sparse warning: memcpy with byte count of 0 */ > + /* Avoid GCC warning: memcpy with NULL dest (but byte count of 0). */ > + if (data && len) > memcpy(offset, data, len); This makes no sense. The if clause was added to silence sparse. That then in turn caused gcc to barf. However, sparse has since been fixed so that it doesn't warn without the if clause. The solution is not to keep adding crap to the if clause, but to get rid of it once and for all. Thanks,
On Thu, Dec 01, 2022 at 05:04:14PM -0800, Kees Cook wrote: > GCC 12 appears to perform constant propagation incompletely(?) and can > no longer notice that "len" is always 0 when "data" is NULL. Expand the > check to avoid warnings about memcpy() having a NULL argument: Is there a gcc option to turn off the "memcpy with NULL and len=0 is undefined behavior" thing? It's basically a bug in the C standard. Note that the kernel already uses options that make other types of undefined behavior defined: -fno-strict-overflow, -fno-strict-aliasing, and -fno-delete-null-pointer-checks. - Eric
On Thu, Dec 01, 2022 at 07:18:15PM -0800, Eric Biggers wrote: > On Thu, Dec 01, 2022 at 05:04:14PM -0800, Kees Cook wrote: > > GCC 12 appears to perform constant propagation incompletely(?) and can > > no longer notice that "len" is always 0 when "data" is NULL. Expand the > > check to avoid warnings about memcpy() having a NULL argument: > > Is there a gcc option to turn off the "memcpy with NULL and len=0 is undefined > behavior" thing? It's basically a bug in the C standard. It's not undefined -- it's just pedantic. __builtin_memcpy is defined internally to GCC with __attribute__((nonnull (1, 2))), and since it can find a path from an always-NULL argument, it warns. I think it's a dumb limitation, given that "zero size to/from NULL" is perfectly valid.
On Fri, Dec 02, 2022 at 10:50:48AM +0800, Herbert Xu wrote: > On Thu, Dec 01, 2022 at 05:04:14PM -0800, Kees Cook wrote: > > > > diff --git a/drivers/crypto/caam/desc_constr.h b/drivers/crypto/caam/desc_constr.h > > index 62ce6421bb3f..d9da4173af9d 100644 > > --- a/drivers/crypto/caam/desc_constr.h > > +++ b/drivers/crypto/caam/desc_constr.h > > @@ -163,7 +163,8 @@ static inline void append_data(u32 * const desc, const void *data, int len) > > { > > u32 *offset = desc_end(desc); > > > > - if (len) /* avoid sparse warning: memcpy with byte count of 0 */ > > + /* Avoid GCC warning: memcpy with NULL dest (but byte count of 0). */ > > + if (data && len) > > memcpy(offset, data, len); > > This makes no sense. The if clause was added to silence sparse. > That then in turn caused gcc to barf. However, sparse has since > been fixed so that it doesn't warn without the if clause. It's _GCC_, not sparse, that is enforcing the nonnull argument attribute. > The solution is not to keep adding crap to the if clause, but to > get rid of it once and for all. Getting rid of the if doesn't solve the warning. I can switch it to just "if (data)", though. That keeps GCC happy.
On Thu, Dec 01, 2022 at 07:23:49PM -0800, Kees Cook wrote: > I think it's a dumb > limitation, given that "zero size to/from NULL" is perfectly valid. No, that is undefined behavior. Which is presumably the reason for the nonnull annotation. Anyway, it is silly, which is why I'd hope that someone would have added an option to disable this C standard bug by now... - Eric
On Thu, Dec 01, 2022 at 07:30:22PM -0800, Kees Cook wrote: > > Getting rid of the if doesn't solve the warning. I can switch it to just > "if (data)", though. That keeps GCC happy. OK I misread the thread. Anyhow, it appears that this warning only occurs due to a debug printk in caam. So how about something like this? diff --git a/drivers/crypto/caam/desc_constr.h b/drivers/crypto/caam/desc_constr.h index 62ce6421bb3f..b49c995e1cc6 100644 --- a/drivers/crypto/caam/desc_constr.h +++ b/drivers/crypto/caam/desc_constr.h @@ -163,7 +163,7 @@ static inline void append_data(u32 * const desc, const void *data, int len) { u32 *offset = desc_end(desc); - if (len) /* avoid sparse warning: memcpy with byte count of 0 */ + if (!IS_ENABLED(CONFIG_CRYPTO_DEV_FSL_CAAM_DEBUG) || data) memcpy(offset, data, len); (*desc) = cpu_to_caam32(caam32_to_cpu(*desc) + Thanks,
On Fri, Dec 02, 2022 at 01:05:16PM +0800, Herbert Xu wrote: > On Thu, Dec 01, 2022 at 07:30:22PM -0800, Kees Cook wrote: > > > > Getting rid of the if doesn't solve the warning. I can switch it to just > > "if (data)", though. That keeps GCC happy. > > OK I misread the thread. > > Anyhow, it appears that this warning only occurs due to a debug > printk in caam. So how about something like this? What? I don't think that's true? I think CONFIG_CRYPTO_DEV_FSL_CAAM_DEBUG only controls "PRINT_POS", which is unrelated? The call path is: drivers/crypto/caam/key_gen.c: gen_split_key() append_fifo_load_as_imm(..., NULL, ...) <- literal NULL append_cmd_data(..., data, ...) memcpy(..., data, ...) and doesn't seem affected at all by CONFIG_CRYPTO_DEV_FSL_CAAM_DEBUG. -Kees > > diff --git a/drivers/crypto/caam/desc_constr.h b/drivers/crypto/caam/desc_constr.h > index 62ce6421bb3f..b49c995e1cc6 100644 > --- a/drivers/crypto/caam/desc_constr.h > +++ b/drivers/crypto/caam/desc_constr.h > @@ -163,7 +163,7 @@ static inline void append_data(u32 * const desc, const void *data, int len) > { > u32 *offset = desc_end(desc); > > - if (len) /* avoid sparse warning: memcpy with byte count of 0 */ > + if (!IS_ENABLED(CONFIG_CRYPTO_DEV_FSL_CAAM_DEBUG) || data) > memcpy(offset, data, len); > > (*desc) = cpu_to_caam32(caam32_to_cpu(*desc) +
On Fri, Dec 02, 2022 at 10:54:05AM -0800, Kees Cook wrote: > > What? I don't think that's true? I think > CONFIG_CRYPTO_DEV_FSL_CAAM_DEBUG only controls "PRINT_POS", which is > unrelated? Without the PRINT_POS or DEBUG enabled I can't reproduce this on arm. Can you reproduce this without it? If so please send me your Kconfig file and compiler version. Thanks,
diff --git a/drivers/crypto/caam/desc_constr.h b/drivers/crypto/caam/desc_constr.h index 62ce6421bb3f..d9da4173af9d 100644 --- a/drivers/crypto/caam/desc_constr.h +++ b/drivers/crypto/caam/desc_constr.h @@ -163,7 +163,8 @@ static inline void append_data(u32 * const desc, const void *data, int len) { u32 *offset = desc_end(desc); - if (len) /* avoid sparse warning: memcpy with byte count of 0 */ + /* Avoid GCC warning: memcpy with NULL dest (but byte count of 0). */ + if (data && len) memcpy(offset, data, len); (*desc) = cpu_to_caam32(caam32_to_cpu(*desc) +
GCC 12 appears to perform constant propagation incompletely(?) and can no longer notice that "len" is always 0 when "data" is NULL. Expand the check to avoid warnings about memcpy() having a NULL argument: ... from drivers/crypto/caam/key_gen.c:8: drivers/crypto/caam/desc_constr.h: In function 'append_data.constprop': include/linux/fortify-string.h:48:33: warning: argument 2 null where non-null expected [-Wnonnull] 48 | #define __underlying_memcpy __builtin_memcpy | ^ include/linux/fortify-string.h:438:9: note: in expansion of macro '__underlying_memcpy' 438 | __underlying_##op(p, q, __fortify_size); \ | ^~~~~~~~~~~~~ The NULL was being propagated from: append_fifo_load_as_imm(desc, NULL, 0, LDST_CLASS_2_CCB | FIFOLD_TYPE_MSG | FIFOLD_TYPE_LAST2); ... static inline void append_##cmd##_as_imm(u32 * const desc, const void *data, \ unsigned int len, u32 options) \ { \ PRINT_POS; \ append_cmd_data(desc, data, len, CMD_##op | options); \ } ... APPEND_CMD_PTR_TO_IMM(fifo_load, FIFO_LOAD); ... static inline void append_cmd_data(u32 * const desc, const void *data, int len, u32 command) { append_cmd(desc, command | IMMEDIATE | len); append_data(desc, data, len); } Cc: Herbert Xu <herbert@gondor.apana.org.au> Cc: "Horia Geantă" <horia.geanta@nxp.com> Cc: Pankaj Gupta <pankaj.gupta@nxp.com> Cc: Gaurav Jain <gaurav.jain@nxp.com> Cc: "David S. Miller" <davem@davemloft.net> Cc: linux-crypto@vger.kernel.org Reported-by: kernel test robot <lkp@intel.com> Link: https://lore.kernel.org/lkml/202210290446.qBayTfzl-lkp@intel.com Tested-by: Anders Roxell <anders.roxell@linaro.org> Signed-off-by: Kees Cook <keescook@chromium.org> --- v1: https://lore.kernel.org/lkml/20221028210527.never.934-kees@kernel.org/ v2: update comment (anders) Note that this is about GCC, not sparse (any more). --- drivers/crypto/caam/desc_constr.h | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-)