crypto/caam: Avoid GCC constprop bug warning

Sent: 28 October 2022 22:06

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:

Quoted 5 lines · Kees Cook

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

@@ -163,7 +163,7 @@ static inline void append_data(u32 * const desc, const void *data, int len)
 {
 	u32 *offset = desc_end(desc);
 

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

Quoted 3 lines

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

-	if (len) /* avoid sparse warning: memcpy with byte count of 0 */
+	if (data && len) /* avoid sparse warning: memcpy with byte count of 0 */
 		memcpy(offset, data, len);

How about just killing the if clause altogether? I don't see any sparse warnings without it. What am I missing?

I think that was fixed in sparse release v0.5.1 [1]. The workaround 'if (len)' was introduced back in 2011, and the sparse release v0.5.1 was done in 2017. So it should probably be safe to remove the 'if (len)' or what do you think?

On Thu, Dec 01, 2022 at 12:52:44PM +0100, Anders Roxell wrote:
.

Quoted 4 lines

I think that was fixed in sparse release v0.5.1 [1]. The workaround 'if (len)' was introduced back in 2011, and the sparse release v0.5.1 was done in 2017. So it should probably be safe to remove the 'if (len)' or what do you think?

Could you please send a patch? :)

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

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 "Horia Geantă" <[email protected]>

Cc Pankaj Gupta <[email protected]>

Cc Gaurav Jain <[email protected]>

Cc Herbert Xu <[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

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

---
 drivers/crypto/caam/desc_constr.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

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

index 62ce6421bb3f..ddbba8b00ab7 100644

Sent: 02 December 2022 00:58

Quoted 4 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:

Quoted 4 lines

...
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]

Quoted 5 lines

      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);                        \
         |         ^~~~~~~~~~~~~

From: Anders Roxell

Quoted 7 lines

Sent: 02 December 2022 00:58

Quoted 4 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:

Quoted 10 lines

Quoted 4 lines

...
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]

Quoted 5 lines

      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);                        \
         |         ^~~~~~~~~~~~~

...

Is this really a bug in the fortify-string wrappers?
IIRC the call is memcpy(NULL, ptr, 0) (or maybe memcpy(ptr, NULL, 0). In either case call can be removed at compile time.

I'd bet that the constant propagation of 'len' fails because of all the intermediate variables that get used in order to avoid multiple evaluation.

The some 'tricks' that are used in min() (see minmax.h) to generate a constant output for constant input could be use to detect a compile-time zero length.

Something like:

#define memcpy(dst, src, len) \
	(__is_constzero(len) ? (dst) : memcpy_check(dst, src, len))

With:
#define __is_constzero(x) sizeof(*(1 ? (void *)(x) : (int *)0) != 1) Which could go into const.h and used in the definition of __is_constexpr().

Sent: 02 December 2022 18:58

Quoted 36 lines · David Laight

From: Anders Roxell

Quoted 7 lines

Sent: 02 December 2022 00:58

Quoted 4 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:

Quoted 10 lines

Quoted 4 lines

...
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]

Quoted 5 lines

      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);                        \
         |         ^~~~~~~~~~~~~

...

Is this really a bug in the fortify-string wrappers?
IIRC the call is memcpy(NULL, ptr, 0) (or maybe memcpy(ptr, NULL, 0). In either case call can be removed at compile time.

I'd bet that the constant propagation of 'len' fails because of all the intermediate variables that get used in order to avoid multiple evaluation.

The some 'tricks' that are used in min() (see minmax.h) to generate a constant output for constant input could be use to detect a compile-time zero length.

Something like:

#define memcpy(dst, src, len) \
	(__is_constzero(len) ? (dst) : memcpy_check(dst, src, len))

Quoted 3 lines

With:
#define __is_constzero(x) sizeof(*(1 ? (void *)(x) : (int *)0) != 1) Which could go into const.h and used in the definition of __is_constexpr().

While it could be possible to strip the nonnull attribute, I think it's not an unreasonable check to have. This is literally the only case in the entire kernel that is tripped, for example.