On Thu, Nov 12, 2020 at 10:21:35PM +0100, Christian Lamparter wrote:
> Hello,
>
> On 12/11/2020 21:07, Nathan Chancellor wrote:
> > Clang warns:
> >
> > drivers/crypto/amcc/crypto4xx_core.c:921:60: warning: operator '?:' has
> > lower precedence than '|'; '|' will be evaluated first
> > [-Wbitwise-conditional-parentheses]
> > (crypto_tfm_alg_type(req->tfm) == CRYPTO_ALG_TYPE_AEAD) ?
> > ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~ ^
> > drivers/crypto/amcc/crypto4xx_core.c:921:60: note: place parentheses
> > around the '|' expression to silence this warning
> > (crypto_tfm_alg_type(req->tfm) == CRYPTO_ALG_TYPE_AEAD) ?
> > ^
> > )
> > drivers/crypto/amcc/crypto4xx_core.c:921:60: note: place parentheses
> > around the '?:' expression to evaluate it first
> > (crypto_tfm_alg_type(req->tfm) == CRYPTO_ALG_TYPE_AEAD) ?
> > ^
> > (
> > 1 warning generated.
> >
> > It looks like this should have been a logical OR so that
> > PD_CTL_HASH_FINAL gets added to the w bitmask if crypto_tfm_alg_type
> > is either CRYPTO_ALG_TYPE_AHASH or CRYPTO_ALG_TYPE_AEAD.
> Yes. This probably wasn't spotted earlier since the driver doesn't make
> use of CRYPTO_ALG_TYPE_AHASH (yet). This is because the hash accelerator
> setup cost was never worth it.
>
> > Change the operator so that everything works properly.
> I'm curious if this is true. Is there a way to break this somehow on purpose?
I do not really have a way to validate that statement, I just figured
that the operator being wrong meant that something could go wrong that
was not intended.
> I've extracted the code from line 921 and added the defines
> (the CRYPTO_ALG_... from the current 5.10-rc3 crypto.h and the PD_CTL_
> from crypto4xx_reg_def.h) and replaced the u32 with uint32_t
> so it runs in userspace too:
>
> --- crypto4xx_test.c ---
> /* test study - is it possible to break the | vs || in crypto4xx's code */
>
> #include <stdio.h>
> #include <stdint.h>
>
> #define CRYPTO_ALG_TYPE_AEAD 0x00000003
> #define CRYPTO_ALG_TYPE_AHASH 0x0000000f
> #define PD_CTL_HASH_FINAL (1<<4) /* Stand-in for BIT(4) */
> #define PD_CTL_HOST_READY (1<<0) /* BIT(0) */
>
> uint32_t func_with_bitwise_or(uint32_t alg_type)
> {
> return PD_CTL_HOST_READY |
> ((alg_type == CRYPTO_ALG_TYPE_AHASH) |
> (alg_type == CRYPTO_ALG_TYPE_AEAD) ?
> PD_CTL_HASH_FINAL : 0);
> }
Looking at this more, I think the only reason that the code works as is
is because PD_CTL_HOST_READY is 1 AND CRYPTO_ALG_TYPE_AHASH is not used.
(alg_type == CRYPTO_ALG_TYPE_AEAD) ? PD_CTL_HASH_FINAL : 0 is evaluated
first, which results in either PD_CTL_HASH_FINAL or 0.
Then (alg_type == CRYPTO_ALG_TYPE_AHASH) is evaluated, which is
evaluated to either 0 or 1.
Then we mask everything together:
PD_CTL_HOST_READY | (0 || 1) | (PD_CTL_HOST_READY || 0)
If PD_CTL_HOST_READY was anything other than BIT(0), we would have an
extra 0x1 in the mask. That realistically might not matter, I did not
have a full look over the code to see what this might mean. If
CRYPTO_ALG_TYPE_AHASH was used, it could be used over
CRYPTO_ALG_TYPE_AEAD and PD_CTL_HASH_FINAL would never get added to the
mask, which certainly sounds like a bug.
> uint32_t func_with_logical_or(uint32_t alg_type)
> {
> return PD_CTL_HOST_READY |
> ((alg_type == CRYPTO_ALG_TYPE_AHASH) ||
> (alg_type == CRYPTO_ALG_TYPE_AEAD) ?
> PD_CTL_HASH_FINAL : 0);
> }
>
> int main(int arg, char **args)
> {
> uint32_t alg;
>
> for (alg = 0; alg < 0x10; alg++) { /* this is because CRYPTO_ALG_TYPE_MASK is 0xf */
> if (func_with_bitwise_or(alg) != func_with_logical_or(alg)) {
> printf("for alg_type:%d, the bitwise result=%d doesn't match the logical result=%d\n",
> alg, func_with_bitwise_or(alg), func_with_logical_or(alg));
> return 1;
> }
> }
> printf("logical and bitwise always agreed.\n");
>
> return 0;
> }
> --- EOF ---
>
> Both gcc (gcc version 10.2.0 (Debian 10.2.0-17)) or clang (clang version 9.0.1-15)
> version always gave the "logical and bitwise always agreed.". which means there wasn't
> anything wrong and this patch just makes clang happy? Or can you get it to break?
>
> Also, can you please give this patch a try:
> --- extra-bracket.patch
>
> --- a/drivers/crypto/amcc/crypto4xx_core.c
> +++ b/drivers/crypto/amcc/crypto4xx_core.c
> @@ -932,8 +932,8 @@ int crypto4xx_build_pd(struct crypto_async_request *req,
> }
>
> pd->pd_ctl.w = PD_CTL_HOST_READY |
> - ((crypto_tfm_alg_type(req->tfm) == CRYPTO_ALG_TYPE_AHASH) |
> - (crypto_tfm_alg_type(req->tfm) == CRYPTO_ALG_TYPE_AEAD) ?
> + (((crypto_tfm_alg_type(req->tfm) == CRYPTO_ALG_TYPE_AHASH) |
> + (crypto_tfm_alg_type(req->tfm) == CRYPTO_ALG_TYPE_AEAD)) ?
> PD_CTL_HASH_FINAL : 0);
> pd->pd_ctl_len.w = 0x00400000 | (assoclen + datalen);
> pd_uinfo->state = PD_ENTRY_INUSE | (is_busy ? PD_ENTRY_BUSY : 0);
>
> ---
> I'm mostly curious if clang will warn about it too.
It does not with that diff. I guess it is entirely up to you which one
we go with.
> That said:
> Reviewed-by: Christian Lamparter <chunkeey@xxxxxxxxx>
Thank you for all the analysis and taking a look over the patch, I
appreciate it!
Cheers,
Nathan
