[PATCH] crypto/caam: Avoid GCC constprop bug warning

[PATCH] crypto/caam: Avoid GCC constprop bug warning

[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:

   ...
                    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ă" <horia.geanta@nxp.com>
Cc: Pankaj Gupta <pankaj.gupta@nxp.com>
Cc: Gaurav Jain <gaurav.jain@nxp.com>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
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
Signed-off-by: Kees Cook <keescook@chromium.org>
---
 drivers/crypto/caam/desc_constr.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/crypto/caam/desc_constr.h b/drivers/crypto/caam/desc_constr.h
index 62ce6421bb3f..ddbba8b00ab7 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 (data && len) /* avoid sparse warning: memcpy with byte count of 0 */
 		memcpy(offset, data, len);
 
 	(*desc) = cpu_to_caam32(caam32_to_cpu(*desc) +
-- 
2.34.1

RE: [PATCH] crypto/caam: Avoid GCC constprop bug warning

[David Laight]

From: Kees Cook
> 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:
...
> 
> diff --git a/drivers/crypto/caam/desc_constr.h b/drivers/crypto/caam/desc_constr.h
> index 62ce6421bb3f..ddbba8b00ab7 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 (data && len) /* avoid sparse warning: memcpy with byte count of 0 */
>  		memcpy(offset, data, len);

I'd guess non-constant zero lengths are unlikely?
So how about:
	/* Avoid calling memcpy() when there is never a buffer */
	if (!__builtin_constant(len) || len)
		memcpy(offset, data, len);

Then the test should never actually end up in the object code.

	David

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

Re: [PATCH] crypto/caam: Avoid GCC constprop bug warning

[Herbert Xu]

On Fri, Oct 28, 2022 at 02:05:31PM -0700, Kees Cook wrote:
>
> @@ -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 (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?

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 constprop bug warning

[Anders Roxell]

On 2022-11-04 17:03, Herbert Xu wrote:
> On Fri, Oct 28, 2022 at 02:05:31PM -0700, Kees Cook wrote:
> >
> > @@ -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 (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?


Cheers,
Anders
[1] https://sparse.docs.kernel.org/en/latest/release-notes/v0.5.1.html

Re: [PATCH] crypto/caam: Avoid GCC constprop bug warning

[Herbert Xu]

On Thu, Dec 01, 2022 at 12:52:44PM +0100, Anders Roxell wrote:
.
> 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 <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 constprop bug warning

[Anders Roxell]

On Thu, 1 Dec 2022 at 13:10, Herbert Xu <herbert@gondor.apana.org.au> wrote:
>
> On Thu, Dec 01, 2022 at 12:52:44PM +0100, Anders Roxell wrote:
> .
> > 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? :)

Please ignore my previous email... since its not the 'if (len)' statement that
GCC comlains about.

Cheers,
Anders


>
> Thanks,
> --
> 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 constprop bug warning

[Anders Roxell]

On 2022-10-28 14:05, 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:
> 
>    ...
>                     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ă" <horia.geanta@nxp.com>
> Cc: Pankaj Gupta <pankaj.gupta@nxp.com>
> Cc: Gaurav Jain <gaurav.jain@nxp.com>
> Cc: Herbert Xu <herbert@gondor.apana.org.au>
> 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
> Signed-off-by: Kees Cook <keescook@chromium.org>


Tested-by: Anders Roxell <anders.roxell@linaro.org>

> ---
>  drivers/crypto/caam/desc_constr.h | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/crypto/caam/desc_constr.h b/drivers/crypto/caam/desc_constr.h
> index 62ce6421bb3f..ddbba8b00ab7 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 (data && len) /* avoid sparse warning: memcpy with byte count of 0 */

Maybe we should update the comment, since newer releases of sparse
doesn't warn about this.


Cheers,
Anders

RE: [PATCH] crypto/caam: Avoid GCC constprop bug warning

[David Laight]

From: Anders Roxell
> Sent: 02 December 2022 00:58
> 
> On 2022-10-28 14:05, 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:
> >
> >    ...
> >                     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);                        \
> >          |         ^~~~~~~~~~~~~
...

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().

	David

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

Re: [PATCH] crypto/caam: Avoid GCC constprop bug warning

[Kees Cook]

On Fri, Dec 02, 2022 at 10:01:50AM +0000, David Laight wrote:
> From: Anders Roxell
> > Sent: 02 December 2022 00:58
> > 
> > On 2022-10-28 14:05, 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:
> > >
> > >    ...
> > >                     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);                        \
> > >          |         ^~~~~~~~~~~~~
> ...
> 
> 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().

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.

-- 
Kees Cook

RE: [PATCH] crypto/caam: Avoid GCC constprop bug warning

[David Laight]

From: Kees Cook
> Sent: 02 December 2022 18:58
> 
> On Fri, Dec 02, 2022 at 10:01:50AM +0000, David Laight wrote:
> > From: Anders Roxell
> > > Sent: 02 December 2022 00:58
> > >
> > > On 2022-10-28 14:05, 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:
> > > >
> > > >    ...
> > > >                     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);                        \
> > > >          |         ^~~~~~~~~~~~~
> > ...
> >
> > 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().
> 
> 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.

It is probably the only place that calls memcpy() with compile-time
NULL and zero length.
IIRC the memcpy() call comes from a #define expansion where some
expansions don't need anything copied.
A simple 'builtin_constant' check and then one for zero in the
#define itself would probably suffice - and avoid the call
being compiled in at all.

	David

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