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

From: Ard Biesheuvel <ardb@kernel.org>
To: Tzung-Bi Shih <tzungbi@google.com>
Cc: ALSA development <alsa-devel@alsa-project.org>,
	Nicolas Boichat <drinkcat@chromium.org>,
	Herbert Xu <herbert@gondor.apana.org.au>,
	Arnd Bergmann <arnd@arndb.de>,
	Liam Girdwood <lgirdwood@gmail.com>,
	Eric Biggers <ebiggers@kernel.org>,
	Guenter Roeck <groeck@chromium.org>,
	Mark Brown <broonie@kernel.org>,
	Enric Balletbo i Serra <enric.balletbo@collabora.com>,
	Benson Leung <bleung@google.com>,
	Benson Leung <bleung@chromium.org>,
	Cheng-Yi Chiang <cychiang@chromium.org>
Subject: Re: [PATCH] SoC: cros_ec_codec: switch to library API for SHA-256
Date: Fri, 15 May 2020 11:08:39 +0200	[thread overview]
Message-ID: <CAMj1kXH7p+nXZnTn-rzo_3u_wCi1VHPMf6jvtZNw8h3TRj6uUw@mail.gmail.com> (raw)
In-Reply-To: <CA+Px+wXFpGZJx6yRJWQPsT0=8uWPduWW1_EierRxEJhjLFHjTw@mail.gmail.com>

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.