From: Nathan Chancellor
> Sent: 12 November 2020 21:49
>
> 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)
The result is the same for both | and || as they are both higher
priority than ?: (which is only higher priority than ,).
The () around the == aren't needed (except to stop the compiler
bleating). The bitwise | is lower priority than == because it
existed before || and K&R didn't change the priority when they
added || (I think they've said later they wished they had.)
The () around the entire ?: clause are needed.
So the code is the same as:
rval = PD_CTL_HOST_READY;
if (alg_type == CRYPTO_ALG_TYPE_AHASH | alg_type == CRYPTO_ALG_TYPE_AEAD)
rval |= PD_CTL_HASH_FINAL;
return rval;
Using | may well generate faster code (no branches).
David
-
Registered Address Lakeside, Bramley Road, Mount Farm, Milton Keynes, MK1 1PT, UK
Registration No: 1397386 (Wales)
