| Message ID | 20200514161847.6240-1-ardb@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | SoC: cros_ec_codec: switch to library API for SHA-256 | expand |
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
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.
Oh, and just notice your patch title should be "ASoC:" instead of "SoC:".
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.
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?
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.
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.
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 {
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(-)