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

[Tom Rini <trini@konsulko.com>]

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

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@linaro.org> wrote:
> >
> > Hi Simon,
> >
> > On Fri, 5 Jul 2024 at 04:36, Simon Glass <sjg@chromium.org> wrote:
> >>
> >> Hi,
> >>
> >> On Wed, Jul 3, 2024, 09:56 Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote:
> >> >
> >> > Hi Raymond
> >> >
> >> > On Tue, 2 Jul 2024 at 21:27, Raymond Mao <raymond.mao@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@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.

-- 
Tom

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