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.apalodi...@linaro.org> wrote: > > > > Hi Simon, > > > > On Thu, 29 Aug 2024 at 18:01, Simon Glass <s...@chromium.org> wrote: > > > > > > Hi Raymond, > > > > > > On Fri, 16 Aug 2024 at 15:47, Raymond Mao <raymond....@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....@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 Thanks /Ilias > > Regards, > Simon
