| Message ID | 20240816214436.1877263-8-raymond.mao@linaro.org |
|---|---|
| State | Changes Requested |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | Integrate MbedTLS v3.6 LTS with U-Boot | expand |
Hi Raymond On Sat, 17 Aug 2024 at 00: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(+) > > diff --git a/common/hash.c b/common/hash.c > index ac63803fed9..d25fc4854c7 100644 > --- a/common/hash.c > +++ b/common/hash.c > @@ -35,6 +35,144 @@ > #include <u-boot/sha512.h> > #include <u-boot/md5.h> > > +#if CONFIG_IS_ENABLED(MBEDTLS_LIB_CRYPTO) > + > +static int __maybe_unused hash_init_sha1(struct hash_algo *algo, void **ctxp) > +{ > + int ret; > + mbedtls_sha1_context *ctx = calloc(1, sizeof(*ctx)); > + > + if (!ctx) > + return -ENOMEM; > + > + mbedtls_sha1_init(ctx); > + ret = mbedtls_sha1_starts(ctx); > + if (!ret) { > + *ctxp = ctx; > + } else { > + mbedtls_sha1_free(ctx); > + free(ctx); > + } > + > + return ret; > +} > + > +static int __maybe_unused hash_update_sha1(struct hash_algo *algo, void *ctx, > + const void *buf, unsigned int size, > + int is_last) > +{ > + return mbedtls_sha1_update((mbedtls_sha1_context *)ctx, buf, size); > +} > + > +static int __maybe_unused > +hash_finish_sha1(struct hash_algo *algo, void *ctx, void *dest_buf, int size) > +{ > + int ret; > + > + if (size < algo->digest_size) > + return -1; > + > + ret = mbedtls_sha1_finish((mbedtls_sha1_context *)ctx, dest_buf); > + if (!ret) { patch # calls finish & free regardless of the return result of mbedtls_xxxx_finish(). I think this should happen here as well > + mbedtls_sha1_free((mbedtls_sha1_context *)ctx); > + free(ctx); > + } > + > + return ret; > +} > + > +static int __maybe_unused hash_init_sha256(struct hash_algo *algo, void **ctxp) > +{ > + int ret; > + int is224 = algo->digest_size == SHA224_SUM_LEN ? 1 : 0; > + mbedtls_sha256_context *ctx = calloc(1, sizeof(*ctx)); > + > + if (!ctx) > + return -ENOMEM; > + > + mbedtls_sha256_init(ctx); > + ret = mbedtls_sha256_starts(ctx, is224); > + if (!ret) { > + *ctxp = ctx; > + } else { > + mbedtls_sha256_free(ctx); > + free(ctx); > + } > + > + return ret; > +} > + > +static int __maybe_unused hash_update_sha256(struct hash_algo *algo, void *ctx, > + const void *buf, uint size, > + int is_last) > +{ > + return mbedtls_sha256_update((mbedtls_sha256_context *)ctx, buf, size); > +} > + > +static int __maybe_unused > +hash_finish_sha256(struct hash_algo *algo, void *ctx, void *dest_buf, int size) > +{ > + int ret; > + > + if (size < algo->digest_size) > + return -1; > + > + ret = mbedtls_sha256_finish((mbedtls_sha256_context *)ctx, dest_buf); > + if (!ret) { > + mbedtls_sha256_free((mbedtls_sha256_context *)ctx); > + free(ctx); > + } > + > + return ret; > +} > + > +static int __maybe_unused hash_init_sha512(struct hash_algo *algo, void **ctxp) > +{ > + int ret; > + int is384 = algo->digest_size == SHA384_SUM_LEN ? 1 : 0; > + mbedtls_sha512_context *ctx = calloc(1, sizeof(*ctx)); > + > + if (!ctx) > + return -ENOMEM; > + > + mbedtls_sha512_init(ctx); > + ret = mbedtls_sha512_starts(ctx, is384); > + if (!ret) { > + *ctxp = ctx; > + } else { > + mbedtls_sha512_free(ctx); > + free(ctx); > + } > + > + return ret; > +} > + > +static int __maybe_unused hash_update_sha512(struct hash_algo *algo, void *ctx, > + const void *buf, uint size, > + int is_last) > +{ > + return mbedtls_sha512_update((mbedtls_sha512_context *)ctx, buf, size); > +} > + > +static int __maybe_unused > +hash_finish_sha512(struct hash_algo *algo, void *ctx, void *dest_buf, int size) > +{ > + int ret; > + > + if (size < algo->digest_size) > + return -1; > + > + ret = mbedtls_sha512_finish((mbedtls_sha512_context *)ctx, dest_buf); > + if (!ret) { > + mbedtls_sha512_free((mbedtls_sha512_context *)ctx); > + free(ctx); > + } > + > + return ret; > +} > + > +#else /* CONFIG_IS_ENABLED(MBEDTLS_LIB_CRYPTO) */ > + > static int __maybe_unused hash_init_sha1(struct hash_algo *algo, void **ctxp) > { > sha1_context *ctx = malloc(sizeof(sha1_context)); > @@ -143,6 +281,8 @@ static int __maybe_unused hash_finish_sha512(struct hash_algo *algo, void *ctx, > return 0; > } > > +#endif /* CONFIG_IS_ENABLED(MBEDTLS_LIB_CRYPTO) */ > + > static int hash_init_crc16_ccitt(struct hash_algo *algo, void **ctxp) > { > uint16_t *ctx = malloc(sizeof(uint16_t)); > @@ -267,10 +407,16 @@ static struct hash_algo hash_algo[] = { > .hash_init = hw_sha_init, > .hash_update = hw_sha_update, > .hash_finish = hw_sha_finish, > +#else > +#if CONFIG_IS_ENABLED(MBEDTLS_LIB_CRYPTO) > + .hash_init = hash_init_sha512, > + .hash_update = hash_update_sha512, > + .hash_finish = hash_finish_sha512, > #else > .hash_init = hash_init_sha384, > .hash_update = hash_update_sha384, > .hash_finish = hash_finish_sha384, > +#endif > #endif > }, > #endif > -- > 2.25.1 > Thanks /Ilias
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. Can you do the rest of the integration first? Regards, Simon
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? Thanks /Ilias > > Can you do the rest of the integration first? > > Regards, > Simon
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). > > 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. The biggest challenge here is that common/hash.c needs some love, as I mentioned in an earlier version. Regards, Simon
Hi Simon, On Thu, 29 Aug 2024 at 11: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. > > The truth is that other MbedTLS modules e.g. x509, pkcs7 all depend on its own digest library and there is no option for MbedTLS to depend on an external digest library. Unless a refactoring in MbedTLS itself - I believe this is difficult for the MbedTLS project to adopt as it is aimed to be an all-in-one crypto solution. Regards, Raymond
Hi Ilias, On Wed, 28 Aug 2024 at 05:54, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > Hi Raymond > > On Sat, 17 Aug 2024 at 00: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(+) > > > > diff --git a/common/hash.c b/common/hash.c > > index ac63803fed9..d25fc4854c7 100644 > > --- a/common/hash.c > > +++ b/common/hash.c > > @@ -35,6 +35,144 @@ > > #include <u-boot/sha512.h> > > #include <u-boot/md5.h> > > > > +#if CONFIG_IS_ENABLED(MBEDTLS_LIB_CRYPTO) > > + > > +static int __maybe_unused hash_init_sha1(struct hash_algo *algo, void > **ctxp) > > +{ > > + int ret; > > + mbedtls_sha1_context *ctx = calloc(1, sizeof(*ctx)); > > + > > + if (!ctx) > > + return -ENOMEM; > > + > > + mbedtls_sha1_init(ctx); > > + ret = mbedtls_sha1_starts(ctx); > > + if (!ret) { > > + *ctxp = ctx; > > + } else { > > + mbedtls_sha1_free(ctx); > > + free(ctx); > > + } > > + > > + return ret; > > +} > > + > > +static int __maybe_unused hash_update_sha1(struct hash_algo *algo, void > *ctx, > > + const void *buf, unsigned int > size, > > + int is_last) > > +{ > > + return mbedtls_sha1_update((mbedtls_sha1_context *)ctx, buf, > size); > > +} > > + > > +static int __maybe_unused > > +hash_finish_sha1(struct hash_algo *algo, void *ctx, void *dest_buf, int > size) > > +{ > > + int ret; > > + > > + if (size < algo->digest_size) > > + return -1; > > + > > + ret = mbedtls_sha1_finish((mbedtls_sha1_context *)ctx, dest_buf); > > + if (!ret) { > > patch # calls finish & free regardless of the return result of > mbedtls_xxxx_finish(). > I think this should happen here as well > > Unlike the other one who returns void, this API returns int. Why don't we check the result here and return the error code when it exists? [snip] Regards, Raymond
Hi Ilias, On Fri, 30 Aug 2024 at 05: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? > > Digest is the basic library of MbedTLS, I don't believe we can disable it but only use the ones for certificates, unless MbedTLS makes changes to allow hooking external digest libraries - as I mentioned in a previous reply, I don't think this is what MbedTLS wants. Regards, Raymond
Hi Raymond, On Tue, 3 Sept 2024 at 18:54, Raymond Mao <raymond.mao@linaro.org> wrote: > > Hi Ilias, > > On Fri, 30 Aug 2024 at 05: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? >> > Digest is the basic library of MbedTLS, I don't believe we can disable it > but only use the ones for certificates, unless MbedTLS makes changes > to allow hooking external digest libraries - as I mentioned in a previous reply, > I don't think this is what MbedTLS wants. There's a config option on config.h we could use to override shaXXX, but given that mbedTLS can be used to add more hashing alogorithms, I dont think we should do that Cheers /Ilias > > Regards, > Raymond
Hi Ilias, On Fri, 6 Sept 2024 at 03:36, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > Hi Raymond, > > On Tue, 3 Sept 2024 at 18:54, Raymond Mao <raymond.mao@linaro.org> wrote: > > > > Hi Ilias, > > > > On Fri, 30 Aug 2024 at 05: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? > >> > > Digest is the basic library of MbedTLS, I don't believe we can disable it > > but only use the ones for certificates, unless MbedTLS makes changes > > to allow hooking external digest libraries - as I mentioned in a > previous reply, > > I don't think this is what MbedTLS wants. > > There's a config option on config.h we could use to override shaXXX, > but given that mbedTLS can be used to add more hashing alogorithms, I > dont think we should do that > > If you mean the _ALT macros, they are used for porting HW acceleration. Maybe we can point this to the original U-Boot ones, but I didn't try. Raymond
Hi Raymond, On Fri, 6 Sept 2024 at 17:00, Raymond Mao <raymond.mao@linaro.org> wrote: > > Hi Ilias, > > On Fri, 6 Sept 2024 at 03:36, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: >> >> Hi Raymond, >> >> On Tue, 3 Sept 2024 at 18:54, Raymond Mao <raymond.mao@linaro.org> wrote: >> > >> > Hi Ilias, >> > >> > On Fri, 30 Aug 2024 at 05: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? >> >> >> > Digest is the basic library of MbedTLS, I don't believe we can disable it >> > but only use the ones for certificates, unless MbedTLS makes changes >> > to allow hooking external digest libraries - as I mentioned in a previous reply, >> > I don't think this is what MbedTLS wants. >> >> There's a config option on config.h we could use to override shaXXX, >> but given that mbedTLS can be used to add more hashing alogorithms, I >> dont think we should do that >> > If you mean the _ALT macros, they are used for porting HW acceleration. > Maybe we can point this to the original U-Boot ones, but I didn't try. > That will work, it's not for hw accel only, it's for an alternative implementation. But then again you have to change the args of the u-boot ones to match mbedTLS. I really don't think it's worth the effort. Besides the main advantage here, is that we can use more than just the SHAXXX U-Boot has, without adding any crypto code to U-Boot -- just a glue layer. Thanks /Ilias > Raymond
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 Thanks /Ilias > > Regards, > Simon
Hi Ilias, On Fri, 13 Sept 2024 at 09: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 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. Regards, Simon
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 > > 2) 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
Hi Simon, On Mon, 16 Sept 2024 at 18:43, Simon Glass <sjg@chromium.org> wrote: > > Hi Ilias, > > On Fri, 13 Sept 2024 at 09: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 > > 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
Hi Ilias, On Tue, 17 Sept 2024 at 15:02, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > Hi Simon, > > On Mon, 16 Sept 2024 at 18:43, Simon Glass <sjg@chromium.org> wrote: > > > > Hi Ilias, > > > > On Fri, 13 Sept 2024 at 09: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 > > > > 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
diff --git a/common/hash.c b/common/hash.c index ac63803fed9..d25fc4854c7 100644 --- a/common/hash.c +++ b/common/hash.c @@ -35,6 +35,144 @@ #include <u-boot/sha512.h> #include <u-boot/md5.h> +#if CONFIG_IS_ENABLED(MBEDTLS_LIB_CRYPTO) + +static int __maybe_unused hash_init_sha1(struct hash_algo *algo, void **ctxp) +{ + int ret; + mbedtls_sha1_context *ctx = calloc(1, sizeof(*ctx)); + + if (!ctx) + return -ENOMEM; + + mbedtls_sha1_init(ctx); + ret = mbedtls_sha1_starts(ctx); + if (!ret) { + *ctxp = ctx; + } else { + mbedtls_sha1_free(ctx); + free(ctx); + } + + return ret; +} + +static int __maybe_unused hash_update_sha1(struct hash_algo *algo, void *ctx, + const void *buf, unsigned int size, + int is_last) +{ + return mbedtls_sha1_update((mbedtls_sha1_context *)ctx, buf, size); +} + +static int __maybe_unused +hash_finish_sha1(struct hash_algo *algo, void *ctx, void *dest_buf, int size) +{ + int ret; + + if (size < algo->digest_size) + return -1; + + ret = mbedtls_sha1_finish((mbedtls_sha1_context *)ctx, dest_buf); + if (!ret) { + mbedtls_sha1_free((mbedtls_sha1_context *)ctx); + free(ctx); + } + + return ret; +} + +static int __maybe_unused hash_init_sha256(struct hash_algo *algo, void **ctxp) +{ + int ret; + int is224 = algo->digest_size == SHA224_SUM_LEN ? 1 : 0; + mbedtls_sha256_context *ctx = calloc(1, sizeof(*ctx)); + + if (!ctx) + return -ENOMEM; + + mbedtls_sha256_init(ctx); + ret = mbedtls_sha256_starts(ctx, is224); + if (!ret) { + *ctxp = ctx; + } else { + mbedtls_sha256_free(ctx); + free(ctx); + } + + return ret; +} + +static int __maybe_unused hash_update_sha256(struct hash_algo *algo, void *ctx, + const void *buf, uint size, + int is_last) +{ + return mbedtls_sha256_update((mbedtls_sha256_context *)ctx, buf, size); +} + +static int __maybe_unused +hash_finish_sha256(struct hash_algo *algo, void *ctx, void *dest_buf, int size) +{ + int ret; + + if (size < algo->digest_size) + return -1; + + ret = mbedtls_sha256_finish((mbedtls_sha256_context *)ctx, dest_buf); + if (!ret) { + mbedtls_sha256_free((mbedtls_sha256_context *)ctx); + free(ctx); + } + + return ret; +} + +static int __maybe_unused hash_init_sha512(struct hash_algo *algo, void **ctxp) +{ + int ret; + int is384 = algo->digest_size == SHA384_SUM_LEN ? 1 : 0; + mbedtls_sha512_context *ctx = calloc(1, sizeof(*ctx)); + + if (!ctx) + return -ENOMEM; + + mbedtls_sha512_init(ctx); + ret = mbedtls_sha512_starts(ctx, is384); + if (!ret) { + *ctxp = ctx; + } else { + mbedtls_sha512_free(ctx); + free(ctx); + } + + return ret; +} + +static int __maybe_unused hash_update_sha512(struct hash_algo *algo, void *ctx, + const void *buf, uint size, + int is_last) +{ + return mbedtls_sha512_update((mbedtls_sha512_context *)ctx, buf, size); +} + +static int __maybe_unused +hash_finish_sha512(struct hash_algo *algo, void *ctx, void *dest_buf, int size) +{ + int ret; + + if (size < algo->digest_size) + return -1; + + ret = mbedtls_sha512_finish((mbedtls_sha512_context *)ctx, dest_buf); + if (!ret) { + mbedtls_sha512_free((mbedtls_sha512_context *)ctx); + free(ctx); + } + + return ret; +} + +#else /* CONFIG_IS_ENABLED(MBEDTLS_LIB_CRYPTO) */ + static int __maybe_unused hash_init_sha1(struct hash_algo *algo, void **ctxp) { sha1_context *ctx = malloc(sizeof(sha1_context)); @@ -143,6 +281,8 @@ static int __maybe_unused hash_finish_sha512(struct hash_algo *algo, void *ctx, return 0; } +#endif /* CONFIG_IS_ENABLED(MBEDTLS_LIB_CRYPTO) */ + static int hash_init_crc16_ccitt(struct hash_algo *algo, void **ctxp) { uint16_t *ctx = malloc(sizeof(uint16_t)); @@ -267,10 +407,16 @@ static struct hash_algo hash_algo[] = { .hash_init = hw_sha_init, .hash_update = hw_sha_update, .hash_finish = hw_sha_finish, +#else +#if CONFIG_IS_ENABLED(MBEDTLS_LIB_CRYPTO) + .hash_init = hash_init_sha512, + .hash_update = hash_update_sha512, + .hash_finish = hash_finish_sha512, #else .hash_init = hash_init_sha384, .hash_update = hash_update_sha384, .hash_finish = hash_finish_sha384, +#endif #endif }, #endif
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(+)