[PATCH] SoC: cros_ec_codec: switch to library API for SHA-256

[PATCH] SoC: cros_ec_codec: switch to library API for SHA-256

[Ard Biesheuvel]

The CrOS EC codec driver uses SHA-256 explicitly, and not in a
performance critical manner, so there is really no point in using
the dynamic SHASH crypto API here. Let's switch to the library API
instead.

Cc: Cheng-Yi Chiang <cychiang@chromium.org> 
Cc: Enric Balletbo i Serra <enric.balletbo@collabora.com>
Cc: Guenter Roeck <groeck@chromium.org>
Cc: Benson Leung <bleung@chromium.org>
Cc: Liam Girdwood <lgirdwood@gmail.com>
Cc: Mark Brown <broonie@kernel.org>
Cc: Arnd Bergmann <arnd@arndb.de>
Cc: Eric Biggers <ebiggers@kernel.org>
Cc: Herbert Xu <herbert@gondor.apana.org.au>
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
Looking at the code, I was wondering if the SHA-256 is really required
here? It looks like it is using it as some kind of fingerprint to decide
whether the provided file is identical to the one that has already been
loaded. If this is the case, we should probably just use CRC32 instead.

Also, do we really need to wipe the context struct? Is there any security
sensitive data in there?

 sound/soc/codecs/Kconfig         |  3 +--
 sound/soc/codecs/cros_ec_codec.c | 21 +++++---------------
 2 files changed, 6 insertions(+), 18 deletions(-)

diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig
index e6a0c5d05fa5..c7ce4cc658cf 100644
--- a/sound/soc/codecs/Kconfig
+++ b/sound/soc/codecs/Kconfig
@@ -537,8 +537,7 @@ config SND_SOC_CQ0093VC
 config SND_SOC_CROS_EC_CODEC
 	tristate "codec driver for ChromeOS EC"
 	depends on CROS_EC
-	select CRYPTO
-	select CRYPTO_SHA256
+	select CRYPTO_LIB_SHA256
 	help
 	  If you say yes here you will get support for the
 	  ChromeOS Embedded Controller's Audio Codec.
diff --git a/sound/soc/codecs/cros_ec_codec.c b/sound/soc/codecs/cros_ec_codec.c
index d3dc42aa6825..6bc02c485ab2 100644
--- a/sound/soc/codecs/cros_ec_codec.c
+++ b/sound/soc/codecs/cros_ec_codec.c
@@ -107,24 +107,13 @@ static int send_ec_host_command(struct cros_ec_device *ec_dev, uint32_t cmd,
 static int calculate_sha256(struct cros_ec_codec_priv *priv,
 			    uint8_t *buf, uint32_t size, uint8_t *digest)
 {
-	struct crypto_shash *tfm;
+	struct sha256_state sctx;
 
-	tfm = crypto_alloc_shash("sha256", CRYPTO_ALG_TYPE_SHASH, 0);
-	if (IS_ERR(tfm)) {
-		dev_err(priv->dev, "can't alloc shash\n");
-		return PTR_ERR(tfm);
-	}
-
-	{
-		SHASH_DESC_ON_STACK(desc, tfm);
-
-		desc->tfm = tfm;
-
-		crypto_shash_digest(desc, buf, size, digest);
-		shash_desc_zero(desc);
-	}
+	sha256_init(&sctx);
+	sha256_update(&sctx, buf, size);
+	sha256_final(&sctx, digest);
 
-	crypto_free_shash(tfm);
+	memzero_explicit(&sctx, sizeof(sctx));
 
 #ifdef DEBUG
 	{
-- 
2.17.1

Re: [PATCH] SoC: cros_ec_codec: switch to library API for SHA-256

[Benson Leung]

[-- Attachment #1: Type: text/plain, Size: 3264 bytes --]

Hi Ard,

On Thu, May 14, 2020 at 06:18:47PM +0200, Ard Biesheuvel wrote:
> The CrOS EC codec driver uses SHA-256 explicitly, and not in a
> performance critical manner, so there is really no point in using
> the dynamic SHASH crypto API here. Let's switch to the library API
> instead.
> 
> Cc: Cheng-Yi Chiang <cychiang@chromium.org> 
> Cc: Enric Balletbo i Serra <enric.balletbo@collabora.com>
> Cc: Guenter Roeck <groeck@chromium.org>
> Cc: Benson Leung <bleung@chromium.org>
> Cc: Liam Girdwood <lgirdwood@gmail.com>
> Cc: Mark Brown <broonie@kernel.org>
> Cc: Arnd Bergmann <arnd@arndb.de>
> Cc: Eric Biggers <ebiggers@kernel.org>
> Cc: Herbert Xu <herbert@gondor.apana.org.au>
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
> Looking at the code, I was wondering if the SHA-256 is really required
> here? It looks like it is using it as some kind of fingerprint to decide
> whether the provided file is identical to the one that has already been
> loaded. If this is the case, we should probably just use CRC32 instead.
> 
> Also, do we really need to wipe the context struct? Is there any security
> sensitive data in there?
> 

Adding Tzung-Bi Shih <tzungbi@google.com> to help answer these, as these
were added as a part of his change here:
 b6bc07d4360d  ASoC: cros_ec_codec: support WoV


Thanks,
Benson

>  sound/soc/codecs/Kconfig         |  3 +--
>  sound/soc/codecs/cros_ec_codec.c | 21 +++++---------------
>  2 files changed, 6 insertions(+), 18 deletions(-)
> 
> diff --git a/sound/soc/codecs/Kconfig b/sound/soc/codecs/Kconfig
> index e6a0c5d05fa5..c7ce4cc658cf 100644
> --- a/sound/soc/codecs/Kconfig
> +++ b/sound/soc/codecs/Kconfig
> @@ -537,8 +537,7 @@ config SND_SOC_CQ0093VC
>  config SND_SOC_CROS_EC_CODEC
>  	tristate "codec driver for ChromeOS EC"
>  	depends on CROS_EC
> -	select CRYPTO
> -	select CRYPTO_SHA256
> +	select CRYPTO_LIB_SHA256
>  	help
>  	  If you say yes here you will get support for the
>  	  ChromeOS Embedded Controller's Audio Codec.
> diff --git a/sound/soc/codecs/cros_ec_codec.c b/sound/soc/codecs/cros_ec_codec.c
> index d3dc42aa6825..6bc02c485ab2 100644
> --- a/sound/soc/codecs/cros_ec_codec.c
> +++ b/sound/soc/codecs/cros_ec_codec.c
> @@ -107,24 +107,13 @@ static int send_ec_host_command(struct cros_ec_device *ec_dev, uint32_t cmd,
>  static int calculate_sha256(struct cros_ec_codec_priv *priv,
>  			    uint8_t *buf, uint32_t size, uint8_t *digest)
>  {
> -	struct crypto_shash *tfm;
> +	struct sha256_state sctx;
>  
> -	tfm = crypto_alloc_shash("sha256", CRYPTO_ALG_TYPE_SHASH, 0);
> -	if (IS_ERR(tfm)) {
> -		dev_err(priv->dev, "can't alloc shash\n");
> -		return PTR_ERR(tfm);
> -	}
> -
> -	{
> -		SHASH_DESC_ON_STACK(desc, tfm);
> -
> -		desc->tfm = tfm;
> -
> -		crypto_shash_digest(desc, buf, size, digest);
> -		shash_desc_zero(desc);
> -	}
> +	sha256_init(&sctx);
> +	sha256_update(&sctx, buf, size);
> +	sha256_final(&sctx, digest);
>  
> -	crypto_free_shash(tfm);
> +	memzero_explicit(&sctx, sizeof(sctx));
>  
>  #ifdef DEBUG
>  	{
> -- 
> 2.17.1
> 

-- 
Benson Leung
Staff Software Engineer
Chrome OS Kernel
Google Inc.
bleung@google.com
Chromium OS Project
bleung@chromium.org

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH] SoC: cros_ec_codec: switch to library API for SHA-256

[Tzung-Bi Shih]

On Fri, May 15, 2020 at 12:26 AM Benson Leung <bleung@google.com> wrote:
> On Thu, May 14, 2020 at 06:18:47PM +0200, Ard Biesheuvel wrote:
> > The CrOS EC codec driver uses SHA-256 explicitly, and not in a
> > performance critical manner, so there is really no point in using
> > the dynamic SHASH crypto API here. Let's switch to the library API
> > instead.

Pardon me if I don't understand it precisely.  What is the difference
between the two APIs?  Suppose it should calculate the same SHA256
hash with the same binary blob after switching to library API?

> > Looking at the code, I was wondering if the SHA-256 is really required
> > here? It looks like it is using it as some kind of fingerprint to decide
> > whether the provided file is identical to the one that has already been
> > loaded. If this is the case, we should probably just use CRC32 instead.

No, the binary blob carries data and possibly code.  We are not only
using the hash as a fingerprint but also an integrity check.

> > Also, do we really need to wipe the context struct? Is there any security
> > sensitive data in there?

No, not necessary as far as I know.

Re: [PATCH] SoC: cros_ec_codec: switch to library API for SHA-256

[Ard Biesheuvel]

On Fri, 15 May 2020 at 04:40, Tzung-Bi Shih <tzungbi@google.com> wrote:
>
> On Fri, May 15, 2020 at 12:26 AM Benson Leung <bleung@google.com> wrote:
> > On Thu, May 14, 2020 at 06:18:47PM +0200, Ard Biesheuvel wrote:
> > > The CrOS EC codec driver uses SHA-256 explicitly, and not in a
> > > performance critical manner, so there is really no point in using
> > > the dynamic SHASH crypto API here. Let's switch to the library API
> > > instead.
>
> Pardon me if I don't understand it precisely.  What is the difference
> between the two APIs?  Suppose it should calculate the same SHA256
> hash with the same binary blob after switching to library API?
>

Yes.

> > > Looking at the code, I was wondering if the SHA-256 is really required
> > > here? It looks like it is using it as some kind of fingerprint to decide
> > > whether the provided file is identical to the one that has already been
> > > loaded. If this is the case, we should probably just use CRC32 instead.
>
> No, the binary blob carries data and possibly code.  We are not only
> using the hash as a fingerprint but also an integrity check.
>

But does it have to be cryptographically strong? Why is CRC32 not sufficient?

> > > Also, do we really need to wipe the context struct? Is there any security
> > > sensitive data in there?
>
> No, not necessary as far as I know.

OK

Re: [PATCH] SoC: cros_ec_codec: switch to library API for SHA-256

[Tzung-Bi Shih]

On Fri, May 15, 2020 at 2:04 PM Ard Biesheuvel <ardb@kernel.org> wrote:
> > > > Looking at the code, I was wondering if the SHA-256 is really required
> > > > here? It looks like it is using it as some kind of fingerprint to decide
> > > > whether the provided file is identical to the one that has already been
> > > > loaded. If this is the case, we should probably just use CRC32 instead.
> >
> > No, the binary blob carries data and possibly code.  We are not only
> > using the hash as a fingerprint but also an integrity check.
> >
>
> But does it have to be cryptographically strong? Why is CRC32 not sufficient?

Please see https://crrev.com/c/1490800/26/include/ec_commands.h#4744
for our original decision.

Also would like to let you know that the data path to call
calculate_sha256( ) is in-frequent (1~2 times) if you think it is too
expensive to use SHA256.

Re: [PATCH] SoC: cros_ec_codec: switch to library API for SHA-256

[Tzung-Bi Shih]

Oh, and just notice your patch title should be "ASoC:" instead of "SoC:".

Re: [PATCH] SoC: cros_ec_codec: switch to library API for SHA-256

[Ard Biesheuvel]

On Fri, 15 May 2020 at 08:40, Tzung-Bi Shih <tzungbi@google.com> wrote:
>
> On Fri, May 15, 2020 at 2:04 PM Ard Biesheuvel <ardb@kernel.org> wrote:
> > > > > Looking at the code, I was wondering if the SHA-256 is really required
> > > > > here? It looks like it is using it as some kind of fingerprint to decide
> > > > > whether the provided file is identical to the one that has already been
> > > > > loaded. If this is the case, we should probably just use CRC32 instead.
> > >
> > > No, the binary blob carries data and possibly code.  We are not only
> > > using the hash as a fingerprint but also an integrity check.
> > >
> >
> > But does it have to be cryptographically strong? Why is CRC32 not sufficient?
>
> Please see https://crrev.com/c/1490800/26/include/ec_commands.h#4744
> for our original decision.
>

In this case, you are using the digest to decide whether the same code
has already been loaded, right?

So it makes sense to think about the threat model here: if you are
able to defeat the strong hash, what does that buy an attacker? If an
attacker is able to create a different piece of code that has the same
digest as the code that was already loaded, the only thing that
happens is that the loader will ignore it. If that is a threat you
want to protect yourself against, then you need sha256. Otherwise, a
crc is sufficient.


> Also would like to let you know that the data path to call
> calculate_sha256( ) is in-frequent (1~2 times) if you think it is too
> expensive to use SHA256

In general, you shouldn't use crypto at all unless you can explain why
it is necessary.

Re: [PATCH] SoC: cros_ec_codec: switch to library API for SHA-256

[Tzung-Bi Shih]

On Fri, May 15, 2020 at 2:50 PM Ard Biesheuvel <ardb@kernel.org> wrote:
> In this case, you are using the digest to decide whether the same code
> has already been loaded, right?
>
> So it makes sense to think about the threat model here: if you are
> able to defeat the strong hash, what does that buy an attacker? If an
> attacker is able to create a different piece of code that has the same
> digest as the code that was already loaded, the only thing that
> happens is that the loader will ignore it. If that is a threat you
> want to protect yourself against, then you need sha256. Otherwise, a
> crc is sufficient.

My original intention is to:
- avoid transmitting duplicate data if remote processor already has
the same binary blob (be reminded that the transmission may be costly)
- check integrity if any transmission error

Not considering preventing an attacker in the original design.  If an
attacker can send arbitrary binary blobs to the remote processor (via
a dedicated SPI or a specific memory region), the attacker should
already "root" the kernel and the device.

I understand your point that CRC should be sufficient in the use case.
However here are my considerations:
- as the payload is possibly executable, I would like to use stronger
hash to pay more attention
- calling calculate_sha256() is in-frequent, I don't really see a
drawback if it is almost one time effort

If we want to switch to use CRC32, we cannot change the kernel code
only.  There is an implementation of a remote processor that also
needs to modify accordingly.  I will keep in mind whether to switch to
use CRC32 next time when we are revisiting the design.

> In general, you shouldn't use crypto at all unless you can explain why
> it is necessary.

When you are mentioning "crypto", do you refer to both crc32 and
sha256 in the case?

Re: [PATCH] SoC: cros_ec_codec: switch to library API for SHA-256

[Ard Biesheuvel]

On Fri, 15 May 2020 at 11:02, Tzung-Bi Shih <tzungbi@google.com> wrote:
>
> On Fri, May 15, 2020 at 2:50 PM Ard Biesheuvel <ardb@kernel.org> wrote:
> > In this case, you are using the digest to decide whether the same code
> > has already been loaded, right?
> >
> > So it makes sense to think about the threat model here: if you are
> > able to defeat the strong hash, what does that buy an attacker? If an
> > attacker is able to create a different piece of code that has the same
> > digest as the code that was already loaded, the only thing that
> > happens is that the loader will ignore it. If that is a threat you
> > want to protect yourself against, then you need sha256. Otherwise, a
> > crc is sufficient.
>
> My original intention is to:
> - avoid transmitting duplicate data if remote processor already has
> the same binary blob (be reminded that the transmission may be costly)
> - check integrity if any transmission error
>
> Not considering preventing an attacker in the original design.  If an
> attacker can send arbitrary binary blobs to the remote processor (via
> a dedicated SPI or a specific memory region), the attacker should
> already "root" the kernel and the device.
>
> I understand your point that CRC should be sufficient in the use case.
> However here are my considerations:
> - as the payload is possibly executable, I would like to use stronger
> hash to pay more attention

As I explained, the fact that the code is executable does not make a
difference here.

Typically, code signing involves SHA-256 to make absolutely sure that
the correct code is loaded only. So *only* if the digest matches, the
code is loaded, and if it doesn't match, the code is rejected.

In your case, the code is only loaded if the digest *does not* match.
I understand that you are using it for integrity as well, but this use
case simply does not require strong crypto, even if it involves code.

> - calling calculate_sha256() is in-frequent, I don't really see a
> drawback if it is almost one time effort
>
> If we want to switch to use CRC32, we cannot change the kernel code
> only.  There is an implementation of a remote processor that also
> needs to modify accordingly.  I will keep in mind whether to switch to
> use CRC32 next time when we are revisiting the design.
>

OK, that is an excellent reason to stick with the current code. If the
receiving side requires changes then switching at this point makes no
sense.

> > In general, you shouldn't use crypto at all unless you can explain why
> > it is necessary.
>
> When you are mentioning "crypto", do you refer to both crc32 and
> sha256 in the case?

No, CRC is not crypto.

Re: [PATCH] SoC: cros_ec_codec: switch to library API for SHA-256

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 402 bytes --]

On Thu, May 14, 2020 at 06:18:47PM +0200, Ard Biesheuvel wrote:
> The CrOS EC codec driver uses SHA-256 explicitly, and not in a
> performance critical manner, so there is really no point in using
> the dynamic SHASH crypto API here. Let's switch to the library API
> instead.

This doesn't apply against current code (it collides with Arnd's patch
to reduce the stack usage), please check and resend.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]