Hi Ilias,
On Fri, 13 Sept 2024 at 11:04, Ilias Apalodimas ilias.apalodimas@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@linaro.org wrote:
Hi Simon,
On Thu, 29 Aug 2024 at 18:01, Simon Glass sjg@chromium.org wrote:
Hi Raymond,
On Fri, 16 Aug 2024 at 15:47, 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.
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
- is a good idea, actually U-Boot original algorithms and hardware
acceleration can be regarded as MbedTLS alternative algorithms. I can start work on a separated patch set on top of my v7 series to avoid introducing too many changes in one patch set. This separated patch set will include the wrapper of U-Boot hashing with kconfigs to control the MbedTLS alternative hashing.
Hi Simon, Is this plan good for you?
Regards, Raymond