linux-kernel - RE: [PATCH] crypto: crypto4xx - Replace bitwise OR with logical OR in crypto4xx_build

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Fri, 13 Nov 2020 09:13:24 +0000
From:   David Laight <David.Laight@...LAB.COM>
To:     'Nathan Chancellor' <natechancellor@...il.com>,
        Christian Lamparter <chunkeey@...il.com>
CC:     Herbert Xu <herbert@...dor.apana.org.au>,
        "David S. Miller" <davem@...emloft.net>,
        Nick Desaulniers <ndesaulniers@...gle.com>,
        "linux-crypto@...r.kernel.org" <linux-crypto@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "clang-built-linux@...glegroups.com" 
        <clang-built-linux@...glegroups.com>
Subject: RE: [PATCH] crypto: crypto4xx - Replace bitwise OR with logical OR in
 crypto4xx_build_pd

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)