[v2] 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 <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(-)
  

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,