[PATCH] crypto: caam - Avoid GCC memset bug warning

[PATCH] crypto: caam - Avoid GCC memset bug warning

[Herbert Xu]

Certain versions of gcc don't like the memcpy with a NULL dst
(which only happens with a zero length).  This only happens
when debugging is enabled so add an if clause to work around
these warnings.

A similar warning used to be generated by sparse but that was
fixed years ago.

Link: https://lore.kernel.org/lkml/202210290446.qBayTfzl-lkp@intel.com
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Kees Cook <keescook@chromium.org>
Reported-by: Uwe Kleine-K�nig <u.kleine-koenig@pengutronix.de>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/drivers/crypto/caam/desc_constr.h b/drivers/crypto/caam/desc_constr.h
index 62ce6421bb3f..824c94d44f94 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 data == NULL */
+	if (!IS_ENABLED(CONFIG_CRYPTO_DEV_FSL_CAAM_DEBUG) || data)
 		memcpy(offset, data, len);
 
 	(*desc) = cpu_to_caam32(caam32_to_cpu(*desc) +
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] crypto: caam - Avoid GCC memset bug warning

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 960 bytes --]

On Wed, Dec 28, 2022 at 04:46:39PM +0800, Herbert Xu wrote:
> Certain versions of gcc don't like the memcpy with a NULL dst
> (which only happens with a zero length).  This only happens
> when debugging is enabled so add an if clause to work around
> these warnings.
> 
> A similar warning used to be generated by sparse but that was
> fixed years ago.
> 
> Link: https://lore.kernel.org/lkml/202210290446.qBayTfzl-lkp@intel.com
> Reported-by: kernel test robot <lkp@intel.com>
> Reported-by: Kees Cook <keescook@chromium.org>
> Reported-by: Uwe Kleine-K�nig <u.kleine-koenig@pengutronix.de>

Huh, broken encoding in the mail. I'd appreciate someone to doublecheck
it's fine in the final commit.

Tested-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

[v2 PATCH] crypto: caam - Avoid GCC memset bug warning

[Herbert Xu]

On Wed, Dec 28, 2022 at 10:39:17AM +0100, Uwe Kleine-König wrote:
>
> Huh, broken encoding in the mail. I'd appreciate someone to doublecheck
> it's fine in the final commit.
> 
> Tested-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>

Sorry.  Let me try again:

---8<---
Certain versions of gcc don't like the memcpy with a NULL dst
(which only happens with a zero length).  This only happens
when debugging is enabled so add an if clause to work around
these warnings.

A similar warning used to be generated by sparse but that was
fixed years ago.

Link: https://lore.kernel.org/lkml/202210290446.qBayTfzl-lkp@intel.com
Reported-by: kernel test robot <lkp@intel.com>
Reported-by: Kees Cook <keescook@chromium.org>
Reported-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Tested-by: Uwe Kleine-König <u.kleine-koenig@pengutronix.de>
Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>

diff --git a/drivers/crypto/caam/desc_constr.h b/drivers/crypto/caam/desc_constr.h
index 62ce6421bb3f..824c94d44f94 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 data == NULL */
+	if (!IS_ENABLED(CONFIG_CRYPTO_DEV_FSL_CAAM_DEBUG) || data)
 		memcpy(offset, data, len);
 
 	(*desc) = cpu_to_caam32(caam32_to_cpu(*desc) +
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

Re: [PATCH] crypto: caam - Avoid GCC memset bug warning

[Uwe Kleine-König]

[-- Attachment #1: Type: text/plain, Size: 2235 bytes --]

On Wed, Dec 28, 2022 at 04:46:39PM +0800, Herbert Xu wrote:
> Certain versions of gcc don't like the memcpy with a NULL dst
> (which only happens with a zero length).  This only happens
> when debugging is enabled so add an if clause to work around
> these warnings.
> 
> A similar warning used to be generated by sparse but that was
> fixed years ago.
> 
> Link: https://lore.kernel.org/lkml/202210290446.qBayTfzl-lkp@intel.com
> Reported-by: kernel test robot <lkp@intel.com>
> Reported-by: Kees Cook <keescook@chromium.org>
> Reported-by: Uwe Kleine-K�nig <u.kleine-koenig@pengutronix.de>
> Signed-off-by: Herbert Xu <herbert@gondor.apana.org.au>
> 
> diff --git a/drivers/crypto/caam/desc_constr.h b/drivers/crypto/caam/desc_constr.h
> index 62ce6421bb3f..824c94d44f94 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 data == NULL */
> +	if (!IS_ENABLED(CONFIG_CRYPTO_DEV_FSL_CAAM_DEBUG) || data)

I just tried: For me a plain

	if (data)

is also enough to make both gcc and sparse happy.

(On a related note, sparse reports:

  CHECK   drivers/crypto/caam/jr.c
drivers/crypto/caam/jr.c: note: in included file (through arch/arm64/include/asm/io.h, include/linux/io.h, include/linux/irq.h, ...):
include/asm-generic/io.h:290:22: warning: incorrect type in argument 1 (different base types)
include/asm-generic/io.h:290:22:    expected unsigned long long [usertype] val
include/asm-generic/io.h:290:22:    got restricted __le64 [usertype]
include/asm-generic/io.h:290:22: warning: incorrect type in argument 1 (different base types)
include/asm-generic/io.h:290:22:    expected unsigned long long [usertype] val
include/asm-generic/io.h:290:22:    got restricted __le64 [usertype]

Didn't look into that though.)

Best regards
Uwe

-- 
Pengutronix e.K.                           | Uwe Kleine-König            |
Industrial Linux Solutions                 | https://www.pengutronix.de/ |

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH] crypto: caam - Avoid GCC memset bug warning

[Herbert Xu]

On Wed, Dec 28, 2022 at 12:30:35PM +0100, Uwe Kleine-König wrote:
>
> > -	if (len) /* avoid sparse warning: memcpy with byte count of 0 */
> > +	/* Avoid gcc warning: memcpy with data == NULL */
> > +	if (!IS_ENABLED(CONFIG_CRYPTO_DEV_FSL_CAAM_DEBUG) || data)
> 
> I just tried: For me a plain
> 
> 	if (data)
> 
> is also enough to make both gcc and sparse happy.

Of course it is.  The point of the extra condition is to remove
the unnecessary check on data unless we are in debugging mode
(as it is only needed in debugging mode to work around the buggy
compiler).

> (On a related note, sparse reports:
> 
>   CHECK   drivers/crypto/caam/jr.c
> drivers/crypto/caam/jr.c: note: in included file (through arch/arm64/include/asm/io.h, include/linux/io.h, include/linux/irq.h, ...):
> include/asm-generic/io.h:290:22: warning: incorrect type in argument 1 (different base types)
> include/asm-generic/io.h:290:22:    expected unsigned long long [usertype] val
> include/asm-generic/io.h:290:22:    got restricted __le64 [usertype]
> include/asm-generic/io.h:290:22: warning: incorrect type in argument 1 (different base types)
> include/asm-generic/io.h:290:22:    expected unsigned long long [usertype] val
> include/asm-generic/io.h:290:22:    got restricted __le64 [usertype]

That's a bug in include/asm-generic/io.h.  It feeds an __le64 to
__raw_writeq which wants a u64.

Cheers,
-- 
Email: Herbert Xu <herbert@gondor.apana.org.au>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt

RE: [PATCH] crypto: caam - Avoid GCC memset bug warning

[David Laight]

From: Herbert Xu
> Sent: 29 December 2022 01:49
> 
> On Wed, Dec 28, 2022 at 12:30:35PM +0100, Uwe Kleine-König wrote:
> >
> > > -	if (len) /* avoid sparse warning: memcpy with byte count of 0 */
> > > +	/* Avoid gcc warning: memcpy with data == NULL */
> > > +	if (!IS_ENABLED(CONFIG_CRYPTO_DEV_FSL_CAAM_DEBUG) || data)
> >
> > I just tried: For me a plain
> >
> > 	if (data)
> >
> > is also enough to make both gcc and sparse happy.
> 
> Of course it is.  The point of the extra condition is to remove
> the unnecessary check on data unless we are in debugging mode
> (as it is only needed in debugging mode to work around the buggy
> compiler).

IIRC the 'problematic' case is one where 'len' and 'data'
are actually compile-time zeros - in which case you don't
want to call memcpy() at all.
In all other cases I think there is something to copy so you
don't really want the check (or the one in memcpy() will do).

Whether (builtin_constant_p(data) && !data) is good enough is
another matter.
It might need the (sizeof *(1 ? (void *)(data) : (int *)0) == 1)
test.

	David

-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)