| Message ID | 20221028210527.never.934-kees@kernel.org (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | crypto/caam: Avoid GCC constprop bug warning | expand |
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)
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,
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
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,
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
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
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)
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.
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)
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) +
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(-)