[PATCH v6 07/28] hash: integrate hash on mbedtls

[Simon Glass]

Hi Ilias,

On Fri, 30 Aug 2024 at 03:37, Ilias Apalodimas
<ilias.apalodimas at linaro.org> wrote:
>
> Hi Simon,
>
> On Thu, 29 Aug 2024 at 18:01, Simon Glass <sjg at chromium.org> wrote:
> >
> > Hi Raymond,
> >
> > On Fri, 16 Aug 2024 at 15:47, Raymond Mao <raymond.mao at linaro.org> wrote:
> > >
> > > Integrate common/hash.c on the hash shim layer so that hash APIs
> > > from mbedtls can be leveraged by boot/image and efi_loader.
> > >
> > > Signed-off-by: Raymond Mao <raymond.mao at linaro.org>
> > > ---
> > > Changes in v2
> > > - Use the original head files instead of creating new ones.
> > > Changes in v3
> > > - Add handle checkers for malloc.
> > > Changes in v4
> > > - None.
> > > Changes in v5
> > > - Add __maybe_unused to solve linker errors in some platforms.
> > > - replace malloc with calloc.
> > > Changes in v6
> > > - None.
> > >
> > >  common/hash.c | 146 ++++++++++++++++++++++++++++++++++++++++++++++++++
> > >  1 file changed, 146 insertions(+)
> >
> > I am not seeing the benefit of replacing U-Boot's hashing algorithms.
> > They work well and don't change. Also it seems to be making the code a
> > lot uglier, with an uncertain timeline for clean-up.
>
> A lot uglier where? It adds a few wrappers that fit into the current
> design and callbacks.
> I don't think what you are asking is possible. To do assymetric
> crypto, signatures  etc -- and in the future add TLS support in wget
> mbedTLS relies on its internal hashing functions for the cipher suites
> it supports. So what you are asking would just make the code even
> larger. Raymond can you please double check?

It's really just a case of dropping the hash calls. It should not
cause any other problems, so far as I can see, but I have not dug in
in detail.

Re TLS is relying on its internal hashing functions, is this what you
are talking about?

$ git grep mbedtls_sha1_free
common/hash.c:          mbedtls_sha1_free(ctx);
common/hash.c:          mbedtls_sha1_free((mbedtls_sha1_context *)ctx);
lib/mbedtls/external/mbedtls/include/mbedtls/sha1.h:void
mbedtls_sha1_free(mbedtls_sha1_context *ctx);
lib/mbedtls/external/mbedtls/library/md.c:
mbedtls_sha1_free(ctx->md_ctx);
lib/mbedtls/external/mbedtls/library/psa_crypto_hash.c:
mbedtls_sha1_free(&operation->ctx.sha1);
lib/mbedtls/external/mbedtls/library/sha1.c:void
mbedtls_sha1_free(mbedtls_sha1_context *ctx)
lib/mbedtls/external/mbedtls/library/sha1.c:    mbedtls_sha1_free(ctx);
lib/mbedtls/external/mbedtls/library/sha1.c:    mbedtls_sha1_free(&ctx);
lib/mbedtls/external/mbedtls/library/sha1.c:    mbedtls_sha1_free(&ctx);
lib/mbedtls/sha1.c:     mbedtls_sha1_free(ctx);

I see this in psa_crypto_hash.c (not sure what that is though).

> > Can you do the rest of the integration first?

I believe this is the best approach. We need to permit using crypto
acceleration too (via driver model), which is obviously impossible if
mbed algorithms are using built-in hashing.

The biggest challenge here is that common/hash.c needs some love, as I
mentioned in an earlier version.

Regards,
Simon