crypto/caam: Avoid GCC constprop bug warning

PATCH In review Kees Cook 3 participants 9 messages Dec 2, 2022

Subsystems (2)

Crypto API Freescale CAAM (Cryptographic Acceleration and Assurance Module) driver

lore

Share & tools

lore

Kees Cook

author

2022-12-02 01:04 UTC #0 lore raw

PATCH v2 1/1 crypto/caam: Avoid GCC constprop bug warning

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 <[email protected]>

Cc "Horia Geantă" <[email protected]>

Cc Pankaj Gupta <[email protected]>

Cc Gaurav Jain <[email protected]>

Cc "David S. Miller" <[email protected]>

Cc [email protected]

Reported-by kernel test robot <[email protected]>

Link https://lore.kernel.org/lkml/[email protected] open here

Tested-by Anders Roxell <[email protected]>

Signed-off-by Kees Cook <[email protected]>

---
v1: https://lore.kernel.org/lkml/[email protected]/
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(-)

drivers/crypto/caam/desc_constr.h +2 -1

index 62ce6421bb3f..d9da4173af9d 100644
@@ -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) +

Signature · 1 line

2.34.1

Herbert Xu

maintainer

2022-12-02 02:51 UTC #1 lore raw

drivers/crypto/caam/desc_constr.h static inline void append_data(u32 * const desc, const void *data, int len) · line 168

Quoted 13 lines · Kees Cook

drivers/crypto/caam/desc_constr.h +2 -1

index 62ce6421bb3f..d9da4173af9d 100644
@@ -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,

Signature · 3 lines

Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Kees Cook

author

2022-12-02 03:30 UTC #2 lore raw

Quoted 10 lines · Herbert Xu

Quoted 9 lines · Kees Cook

drivers/crypto/caam/desc_constr.h +0 -0

index 62ce6421bb3f..d9da4173af9d 100644
@@ -163,7 +163,8 @@ static inline void append_data(u32 * const desc, const void *data, int len)
 {
 	u32 *offset = desc_end(desc);

Quoted 8 lines

drivers/crypto/caam/desc_constr.h static inline void append_data(u32 * const desc, const void *data, int len) · line 168
Quoted 4 lines
drivers/crypto/caam/desc_constr.h +2 -1
-	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.

Quoted 2 lines

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.

Signature · 1 line

Kees Cook

Herbert Xu

maintainer

2022-12-02 05:05 UTC #3 lore raw

Quoted 3 lines · Kees Cook

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?

drivers/crypto/caam/desc_constr.h +1 -1

index 62ce6421bb3f..b49c995e1cc6 100644
@@ -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,

Signature · 3 lines

Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Kees Cook

author

2022-12-02 18:54 UTC #4 lore raw

Quoted 9 lines · Herbert Xu

Quoted 3 lines · Kees Cook
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

Quoted 14 lines

drivers/crypto/caam/desc_constr.h +1 -1

index 62ce6421bb3f..b49c995e1cc6 100644
@@ -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) +

Signature · 1 line

Kees Cook

Herbert Xu

maintainer

2022-12-02 23:37 UTC #5 lore raw

Quoted 4 lines · Kees Cook

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,

Signature · 3 lines

Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Eric Biggers

2022-12-02 03:18 UTC #6 lore raw

In reply to Kees Cook

Quoted 3 lines · Kees Cook

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

Kees Cook

author

2022-12-02 03:23 UTC #7 lore raw

Quoted 7 lines · Eric Biggers

Quoted 3 lines · Kees Cook
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.

Signature · 1 line

Kees Cook

Eric Biggers

2022-12-02 03:32 UTC #8 lore raw

Quoted 2 lines · Kees Cook

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