Hi Simon, On Mon, 16 Sept 2024 at 18:43, Simon Glass <s...@chromium.org> wrote: > > Hi Ilias, > > On Fri, 13 Sept 2024 at 09:04, Ilias Apalodimas > <ilias.apalodi...@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.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 > > 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 Regards /Ilias > Regards, > Simon
