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

[Simon Glass]

Hi Ilias,

On Tue, 17 Sept 2024 at 15:02, Ilias Apalodimas
<ilias.apalodimas at linaro.org> wrote:
>
> Hi Simon,
>
> On Mon, 16 Sept 2024 at 18:43, Simon Glass <sjg at chromium.org> wrote:
> >
> > Hi Ilias,
> >
> > On Fri, 13 Sept 2024 at 09:04, Ilias Apalodimas
> > <ilias.apalodimas at linaro.org> wrote:
> > >
> > >
> > > Hi Simon,
> > >
> > > Apologies lost that email
> > >
> > > > On Sun, Sep 01, 2024 at 02:09:44PM -0600, Simon Glass wrote:
> > > > 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).
> > > PSA is Platform Security Architecture for Arm. They define APIs etc and
> > > some crypto ops can move to the Secure World.
> > >
> > > As I responded later down the thread, mbedTLS config.h file allows you to define
> > > alternative implementations. The benefit that I see by using mbedTLS hashing,
> > > is that we can switch on new algorithms by enabling an option in mbedTLS.
> > > OTOH some work will be needed to plug new algorithms in U-Boot and as you
> > > point out HW accel will not work -- Unless we define the accelerator
> > > functions in the config file above. But that doesn't solve your problem of
> > > having one extra ifdef in hash.c
> > >
> > > >
> > > > > > 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.
> > > >
> > >
> > > Look on the response above, we can, but I don't love the solution.
> > >
> > > > The biggest challenge here is that common/hash.c needs some love, as I
> > > > mentioned in an earlier version.
> > >
> > > Fair enough. So the way I see it we got three options.
> > > - We pull in the current one and explicitly state that mbedTLS != HW accel
> > >   for now and plan for a wider refactoring.
> > > - we write a few wrappers to adjust the u-boot functions and define
> > >   those in the mbedTLS config file. We could then go back and try to make
> > >   mbedTLS work with the existing hw accels. This is doable but
> > > - we treat mbedTLS as a 'hardware accelerator', define hw_sha_init etc and
> > >   make wrappers for that. This does solve the extra ifdefery, but OTOH
> > >   mbedTLS will never work with hw accelerators so I'd say no.
> > >
> > > Raymond, can you take a look at (2) and see if it works? You basically have
> > > to rip out all the hashing code and define wrappers on top of
> > > hash_block() that mbedTLS can use
> >
> > That sounds like a good solution to me. Will you be able to complete
> > the pending migrations at some point? It would be great to unify the
> > interfaces, if we can live with a small code-size increase.
> >
>
> Our plan there is to do (2) on the next version. After the patches get
> merged, we can try to find a way to have the hashing configurable. So
> a user could actually select
> - existing U-Boot hashing
> - Mix & match hash functions from mbedTLS and hardware accelerators--
> in case u-boot has no support yet while being able to use any existing
> offloading

OK. The main thing from my side is to tidy up common/hash.c

Regards,
Simon