[PATCH v4 08/29] hash: integrate hash on mbedtls

[Simon Glass]

Hi Tom,

On Sat, 20 Jul 2024 at 18:13, Tom Rini <trini at konsulko.com> wrote:
>
> On Sat, Jul 20, 2024 at 01:36:02PM +0100, Simon Glass wrote:
> > Hi Tom,
> >
> > On Fri, 19 Jul 2024 at 16:25, Tom Rini <trini at konsulko.com> wrote:
> > >
> > > On Fri, Jul 19, 2024 at 04:05:09PM +0100, Simon Glass wrote:
> > > > Hi Raymond,
> > > >
> > > > On Thu, 18 Jul 2024 at 17:46, Raymond Mao <raymond.mao at linaro.org> wrote:
> > > > >
> > > > > Hi Simon,
> > > > >
> > > > > On Fri, 5 Jul 2024 at 04:36, Simon Glass <sjg at chromium.org> wrote:
> > > > >>
> > > > >> Hi,
> > > > >>
> > > > >> On Wed, Jul 3, 2024, 09:56 Ilias Apalodimas <ilias.apalodimas at linaro.org> wrote:
> > > > >> >
> > > > >> > Hi Raymond
> > > > >> >
> > > > >> > On Tue, 2 Jul 2024 at 21:27, 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.
> > > > >> > >
> > > > >> > >  common/hash.c | 143 ++++++++++++++++++++++++++++++++++++++++++++++++++
> > > > >> > >  1 file changed, 143 insertions(+)
> > > > >> > >
> > > > >> > > diff --git a/common/hash.c b/common/hash.c
> > > > >> > > index ac63803fed9..96caf074374 100644
> > > > >> > > --- a/common/hash.c
> > > > >> > > +++ b/common/hash.c
> > > > >> > > @@ -35,6 +35,141 @@
> > > > >> > >  #include <u-boot/sha512.h>
> > > > >> > >  #include <u-boot/md5.h>
> > > > >> > >
> > > > >> > > +#if CONFIG_IS_ENABLED(MBEDTLS_LIB_CRYPTO)
> > > > >> > > +
> > > > >> > > +static int hash_init_sha1(struct hash_algo *algo, void **ctxp)
> > > > >> > > +{
> > > > >> > > +       int ret;
> > > > >> > > +       mbedtls_sha1_context *ctx = malloc(sizeof(mbedtls_sha1_context));
> > > > >>
> > > > >>
> > > > >> Why do we need allocation here? We should avoid it where possible.
> > > > >>
> > > > > The API "hash_init_sha1(struct hash_algo *algo, void **ctxp)" is passing a pointer
> > > > > address and expecting to get the context from the pointer, it is reasonable to do the
> > > > > allocation.
> > > > > On top of that, this patch doesn't make changes on this API itself, but just adapted
> > > > > it to MbedTLS stacks, thus you can see the allocation is needed by the original API
> > > > > as well.
> > > >
> > > > Oh dear., I see Now I am looking at the code. It is full of #ifdefs
> > > > for different cases.
> > > >
> > > > The whole thing needs a bit of a rationalisation before adding another case.
> > >
> > > If you're referring too the hash_algo struct, I'm not sure we can do
> > > something different that doesn't in turn increase size globally. And
> > > long term some of this may be able to go away if we can remove
> > > non-mbedTLS options.
> >
> > Well, at least using the existing functions rather than writing
> > entirely new ones would help.
> >
> > The real culprit here is the hardware-acceleration stuff, but making
> > the software side messy too is not nice. Hashing should move to a
> > linker-list approach for the implementation, and probably driver model
> > for the hardware acceleration.
>
> I'm fine with looking at that, after mbedtls is merged and we're looking
> at what can be removed / cleaned up. Perhaps we'll be able to shift
> most/all of the hardware assisted algorithms to being handled within
> mbedtls, I don't know.

So long as the same person does the clean-up straight afterwards, that
sounds fine. Otherwise, no one else will take it on as this series
makes it harder.

For hardware-assist, I think we need a driver model solution with a proper API.

Regards,
Simon