When compiling arm64 allmodconfig with gcc 10.2.1 I get
drivers/crypto/caam/desc_constr.h: In function ‘append_data.constprop’:
include/linux/fortify-string.h:57:29: error: argument 2 null where non-null expected [-Werror=nonnull]
Fix this by skipping the memcpy if data is NULL and add a BUG_ON instead
that triggers on a problematic call that is now prevented to trigger.
After data == NULL && len != 0 is known to be false, logically
if (len)
memcpy(...)
could be enough to know that memcpy is not called with dest=NULL, but
gcc doesn't seem smart enough for that conclusion. gcc 12 doesn't have a
problem with the original code.
Signed-off-by: Uwe Kleine-König <[email protected]>
---
drivers/crypto/caam/desc_constr.h | 8 +++++++-
1 file changed, 7 insertions(+), 1 deletion(-)
diff --git a/drivers/crypto/caam/desc_constr.h b/drivers/crypto/caam/desc_constr.h
index 62ce6421bb3f..163e0e740b11 100644
--- a/drivers/crypto/caam/desc_constr.h
+++ b/drivers/crypto/caam/desc_constr.h
@@ -163,7 +163,13 @@ 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 sparse warning: "memcpy with byte count of 0" and
+ * and "error: argument 2 null where non-null expected
+ * [-Werror=nonnull]" with fortify enabled.
+ */
+ BUG_ON(data == NULL && len != 0);
+ if (len && data)
memcpy(offset, data, len);
(*desc) = cpu_to_caam32(caam32_to_cpu(*desc) +
base-commit: 9d2f6060fe4c3b49d0cdc1dce1c99296f33379c8
--
2.30.2
On Thu, Dec 22, 2022 at 05:25:13PM +0100, Uwe Kleine-König wrote:
> When compiling arm64 allmodconfig with gcc 10.2.1 I get
>
> drivers/crypto/caam/desc_constr.h: In function ‘append_data.constprop’:
> include/linux/fortify-string.h:57:29: error: argument 2 null where non-null expected [-Werror=nonnull]
>
> Fix this by skipping the memcpy if data is NULL and add a BUG_ON instead
> that triggers on a problematic call that is now prevented to trigger.
> After data == NULL && len != 0 is known to be false, logically
>
> if (len)
> memcpy(...)
>
> could be enough to know that memcpy is not called with dest=NULL, but
> gcc doesn't seem smart enough for that conclusion. gcc 12 doesn't have a
> problem with the original code.
>
> Signed-off-by: Uwe Kleine-König <[email protected]>
> ---
> drivers/crypto/caam/desc_constr.h | 8 +++++++-
> 1 file changed, 7 insertions(+), 1 deletion(-)
Does this patch fix your problem?
https://lore.kernel.org/all/Y4mHjKXnF%[email protected]/
If not please send me your kconfig file.
Thanks,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
On Fri, Dec 23, 2022 at 02:29:52PM +0800, Herbert Xu wrote:
> On Thu, Dec 22, 2022 at 05:25:13PM +0100, Uwe Kleine-König wrote:
> > When compiling arm64 allmodconfig with gcc 10.2.1 I get
> >
> > drivers/crypto/caam/desc_constr.h: In function ‘append_data.constprop’:
> > include/linux/fortify-string.h:57:29: error: argument 2 null where non-null expected [-Werror=nonnull]
> >
> > Fix this by skipping the memcpy if data is NULL and add a BUG_ON instead
> > that triggers on a problematic call that is now prevented to trigger.
> > After data == NULL && len != 0 is known to be false, logically
> >
> > if (len)
> > memcpy(...)
> >
> > could be enough to know that memcpy is not called with dest=NULL, but
> > gcc doesn't seem smart enough for that conclusion. gcc 12 doesn't have a
> > problem with the original code.
> >
> > Signed-off-by: Uwe Kleine-König <[email protected]>
> > ---
> > drivers/crypto/caam/desc_constr.h | 8 +++++++-
> > 1 file changed, 7 insertions(+), 1 deletion(-)
>
> Does this patch fix your problem?
>
> https://lore.kernel.org/all/Y4mHjKXnF%[email protected]/
Using
if (data && len)
fixes it (that's the patch that b4 picks for the above message id :-\),
if (!IS_ENABLED(CONFIG_CRYPTO_DEV_FSL_CAAM_DEBUG) && len)
makes the problem go away, too. But I wonder if the latter is correct.
Shouldn't the memcpy happen even with that debugging symbol enabled?
> If not please send me your kconfig file.
(It's a plain arm64 allmodconfig)
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
On Fri, Dec 23, 2022 at 06:47:19PM +0100, Uwe Kleine-K?nig wrote:
>
> Using
>
> if (data && len)
>
> fixes it (that's the patch that b4 picks for the above message id :-\),
>
> if (!IS_ENABLED(CONFIG_CRYPTO_DEV_FSL_CAAM_DEBUG) && len)
>
> makes the problem go away, too. But I wonder if the latter is correct.
Thanks for confirming!
> Shouldn't the memcpy happen even with that debugging symbol enabled?
It's just a pecularity with gcc. It only produces the warning
because of the extra complexities introduced by the debugging
code.
Cheers,
--
Email: Herbert Xu <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
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/[email protected]
Reported-by: kernel test robot <[email protected]>
Reported-by: Kees Cook <[email protected]>
Reported-by: Uwe Kleine-K�nig <[email protected]>
Signed-off-by: Herbert Xu <[email protected]>
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 <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
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/[email protected]
> Reported-by: kernel test robot <[email protected]>
> Reported-by: Kees Cook <[email protected]>
> Reported-by: Uwe Kleine-K�nig <[email protected]>
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 <[email protected]>
Best regards
Uwe
--
Pengutronix e.K. | Uwe Kleine-König |
Industrial Linux Solutions | https://www.pengutronix.de/ |
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 <[email protected]>
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/[email protected]
Reported-by: kernel test robot <[email protected]>
Reported-by: Kees Cook <[email protected]>
Reported-by: Uwe Kleine-K?nig <[email protected]>
Tested-by: Uwe Kleine-K?nig <[email protected]>
Signed-off-by: Herbert Xu <[email protected]>
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 <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
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/[email protected]
> Reported-by: kernel test robot <[email protected]>
> Reported-by: Kees Cook <[email protected]>
> Reported-by: Uwe Kleine-K�nig <[email protected]>
> Signed-off-by: Herbert Xu <[email protected]>
>
> 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/ |
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 <[email protected]>
Home Page: http://gondor.apana.org.au/~herbert/
PGP Key: http://gondor.apana.org.au/~herbert/pubkey.txt
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)