| Message ID | 20240702182325.2904421-9-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 Tue, 2 Jul 2024 at 21:27, 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. > > common/hash.c | 143 ++++++++++++++++++++++++++++++++++++++++++++++++++ > 1 file changed, 143 insertions(+) > > diff --git a/common/hash.c b/common/hash.c > index ac63803fed9..96caf074374 100644 > --- a/common/hash.c > +++ b/common/hash.c > @@ -35,6 +35,141 @@ > #include <u-boot/sha512.h> > #include <u-boot/md5.h> > > +#if CONFIG_IS_ENABLED(MBEDTLS_LIB_CRYPTO) > + > +static int hash_init_sha1(struct hash_algo *algo, void **ctxp) > +{ > + int ret; > + mbedtls_sha1_context *ctx = malloc(sizeof(mbedtls_sha1_context)); > + > + 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 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 > +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 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 = malloc(sizeof(mbedtls_sha256_context)); I prefer sizeof(*ctx) on all the allocations. Also since it's a crypto stack I prefer a calloc (sha384 for example will allocate more bytes) > + > + 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 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 > +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 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 = malloc(sizeof(mbedtls_sha512_context)); > + > + 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 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 > +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 +278,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 +404,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, I don't have strong opinion here, but you could alternatively define sha384 wrappers for mbedTLS and avoid this ifdef. In any case I personally don't mind. > #else > .hash_init = hash_init_sha384, > .hash_update = hash_update_sha384, > .hash_finish = hash_finish_sha384, > +#endif > #endif > }, > #endif > -- > 2.25.1 >
Hi, On Wed, Jul 3, 2024, 09:56 Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > Hi Raymond > > On Tue, 2 Jul 2024 at 21:27, 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. > > > > common/hash.c | 143 ++++++++++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 143 insertions(+) > > > > diff --git a/common/hash.c b/common/hash.c > > index ac63803fed9..96caf074374 100644 > > --- a/common/hash.c > > +++ b/common/hash.c > > @@ -35,6 +35,141 @@ > > #include <u-boot/sha512.h> > > #include <u-boot/md5.h> > > > > +#if CONFIG_IS_ENABLED(MBEDTLS_LIB_CRYPTO) > > + > > +static int hash_init_sha1(struct hash_algo *algo, void **ctxp) > > +{ > > + int ret; > > + mbedtls_sha1_context *ctx = malloc(sizeof(mbedtls_sha1_context)); Why do we need allocation here? We should avoid it where possible. > > + > > + 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; > > +} > > + [..] Regards, Simon
Hi Simon, On Fri, 5 Jul 2024 at 04:36, Simon Glass <sjg@chromium.org> wrote: > Hi, > > On Wed, Jul 3, 2024, 09:56 Ilias Apalodimas <ilias.apalodimas@linaro.org> > wrote: > > > > Hi Raymond > > > > On Tue, 2 Jul 2024 at 21:27, 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. > > > > > > common/hash.c | 143 ++++++++++++++++++++++++++++++++++++++++++++++++++ > > > 1 file changed, 143 insertions(+) > > > > > > diff --git a/common/hash.c b/common/hash.c > > > index ac63803fed9..96caf074374 100644 > > > --- a/common/hash.c > > > +++ b/common/hash.c > > > @@ -35,6 +35,141 @@ > > > #include <u-boot/sha512.h> > > > #include <u-boot/md5.h> > > > > > > +#if CONFIG_IS_ENABLED(MBEDTLS_LIB_CRYPTO) > > > + > > > +static int hash_init_sha1(struct hash_algo *algo, void **ctxp) > > > +{ > > > + int ret; > > > + mbedtls_sha1_context *ctx = > malloc(sizeof(mbedtls_sha1_context)); > > > Why do we need allocation here? We should avoid it where possible. > > The API "hash_init_sha1(struct hash_algo *algo, void **ctxp)" is passing a pointer address and expecting to get the context from the pointer, it is reasonable to do the allocation. On top of that, this patch doesn't make changes on this API itself, but just adapted it to MbedTLS stacks, thus you can see the allocation is needed by the original API as well. Regards, Raymond
Hi Ilias, On Wed, 3 Jul 2024 at 04:56, Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > Hi Raymond > > On Tue, 2 Jul 2024 at 21:27, 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. > > > > common/hash.c | 143 ++++++++++++++++++++++++++++++++++++++++++++++++++ > > 1 file changed, 143 insertions(+) > > > > diff --git a/common/hash.c b/common/hash.c > > index ac63803fed9..96caf074374 100644 > > --- a/common/hash.c > > +++ b/common/hash.c > [snip] > > @@ -267,10 +404,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, > > I don't have strong opinion here, but you could alternatively define > sha384 wrappers for mbedTLS and avoid this ifdef. > In any case I personally don't mind. > > I will prefer not to create a new wrapper function in order to save space. Regards, Raymond
Hi Raymond, On Thu, 18 Jul 2024 at 17:46, Raymond Mao <raymond.mao@linaro.org> wrote: > > Hi Simon, > > On Fri, 5 Jul 2024 at 04:36, Simon Glass <sjg@chromium.org> wrote: >> >> Hi, >> >> On Wed, Jul 3, 2024, 09:56 Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: >> > >> > Hi Raymond >> > >> > On Tue, 2 Jul 2024 at 21:27, 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. >> > > >> > > common/hash.c | 143 ++++++++++++++++++++++++++++++++++++++++++++++++++ >> > > 1 file changed, 143 insertions(+) >> > > >> > > diff --git a/common/hash.c b/common/hash.c >> > > index ac63803fed9..96caf074374 100644 >> > > --- a/common/hash.c >> > > +++ b/common/hash.c >> > > @@ -35,6 +35,141 @@ >> > > #include <u-boot/sha512.h> >> > > #include <u-boot/md5.h> >> > > >> > > +#if CONFIG_IS_ENABLED(MBEDTLS_LIB_CRYPTO) >> > > + >> > > +static int hash_init_sha1(struct hash_algo *algo, void **ctxp) >> > > +{ >> > > + int ret; >> > > + mbedtls_sha1_context *ctx = malloc(sizeof(mbedtls_sha1_context)); >> >> >> Why do we need allocation here? We should avoid it where possible. >> > The API "hash_init_sha1(struct hash_algo *algo, void **ctxp)" is passing a pointer > address and expecting to get the context from the pointer, it is reasonable to do the > allocation. > On top of that, this patch doesn't make changes on this API itself, but just adapted > it to MbedTLS stacks, thus you can see the allocation is needed by the original API > as well. Oh dear., I see Now I am looking at the code. It is full of #ifdefs for different cases. The whole thing needs a bit of a rationalisation before adding another case. Regards, Simon
On Fri, Jul 19, 2024 at 04:05:09PM +0100, Simon Glass wrote: > Hi Raymond, > > On Thu, 18 Jul 2024 at 17:46, Raymond Mao <raymond.mao@linaro.org> wrote: > > > > Hi Simon, > > > > On Fri, 5 Jul 2024 at 04:36, Simon Glass <sjg@chromium.org> wrote: > >> > >> Hi, > >> > >> On Wed, Jul 3, 2024, 09:56 Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > >> > > >> > Hi Raymond > >> > > >> > On Tue, 2 Jul 2024 at 21:27, 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. > >> > > > >> > > common/hash.c | 143 ++++++++++++++++++++++++++++++++++++++++++++++++++ > >> > > 1 file changed, 143 insertions(+) > >> > > > >> > > diff --git a/common/hash.c b/common/hash.c > >> > > index ac63803fed9..96caf074374 100644 > >> > > --- a/common/hash.c > >> > > +++ b/common/hash.c > >> > > @@ -35,6 +35,141 @@ > >> > > #include <u-boot/sha512.h> > >> > > #include <u-boot/md5.h> > >> > > > >> > > +#if CONFIG_IS_ENABLED(MBEDTLS_LIB_CRYPTO) > >> > > + > >> > > +static int hash_init_sha1(struct hash_algo *algo, void **ctxp) > >> > > +{ > >> > > + int ret; > >> > > + mbedtls_sha1_context *ctx = malloc(sizeof(mbedtls_sha1_context)); > >> > >> > >> Why do we need allocation here? We should avoid it where possible. > >> > > The API "hash_init_sha1(struct hash_algo *algo, void **ctxp)" is passing a pointer > > address and expecting to get the context from the pointer, it is reasonable to do the > > allocation. > > On top of that, this patch doesn't make changes on this API itself, but just adapted > > it to MbedTLS stacks, thus you can see the allocation is needed by the original API > > as well. > > Oh dear., I see Now I am looking at the code. It is full of #ifdefs > for different cases. > > The whole thing needs a bit of a rationalisation before adding another case. If you're referring too the hash_algo struct, I'm not sure we can do something different that doesn't in turn increase size globally. And long term some of this may be able to go away if we can remove non-mbedTLS options.
Hi Tom, On Fri, 19 Jul 2024 at 16:25, Tom Rini <trini@konsulko.com> wrote: > > On Fri, Jul 19, 2024 at 04:05:09PM +0100, Simon Glass wrote: > > Hi Raymond, > > > > On Thu, 18 Jul 2024 at 17:46, Raymond Mao <raymond.mao@linaro.org> wrote: > > > > > > Hi Simon, > > > > > > On Fri, 5 Jul 2024 at 04:36, Simon Glass <sjg@chromium.org> wrote: > > >> > > >> Hi, > > >> > > >> On Wed, Jul 3, 2024, 09:56 Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > >> > > > >> > Hi Raymond > > >> > > > >> > On Tue, 2 Jul 2024 at 21:27, 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. > > >> > > > > >> > > common/hash.c | 143 ++++++++++++++++++++++++++++++++++++++++++++++++++ > > >> > > 1 file changed, 143 insertions(+) > > >> > > > > >> > > diff --git a/common/hash.c b/common/hash.c > > >> > > index ac63803fed9..96caf074374 100644 > > >> > > --- a/common/hash.c > > >> > > +++ b/common/hash.c > > >> > > @@ -35,6 +35,141 @@ > > >> > > #include <u-boot/sha512.h> > > >> > > #include <u-boot/md5.h> > > >> > > > > >> > > +#if CONFIG_IS_ENABLED(MBEDTLS_LIB_CRYPTO) > > >> > > + > > >> > > +static int hash_init_sha1(struct hash_algo *algo, void **ctxp) > > >> > > +{ > > >> > > + int ret; > > >> > > + mbedtls_sha1_context *ctx = malloc(sizeof(mbedtls_sha1_context)); > > >> > > >> > > >> Why do we need allocation here? We should avoid it where possible. > > >> > > > The API "hash_init_sha1(struct hash_algo *algo, void **ctxp)" is passing a pointer > > > address and expecting to get the context from the pointer, it is reasonable to do the > > > allocation. > > > On top of that, this patch doesn't make changes on this API itself, but just adapted > > > it to MbedTLS stacks, thus you can see the allocation is needed by the original API > > > as well. > > > > Oh dear., I see Now I am looking at the code. It is full of #ifdefs > > for different cases. > > > > The whole thing needs a bit of a rationalisation before adding another case. > > If you're referring too the hash_algo struct, I'm not sure we can do > something different that doesn't in turn increase size globally. And > long term some of this may be able to go away if we can remove > non-mbedTLS options. Well, at least using the existing functions rather than writing entirely new ones would help. The real culprit here is the hardware-acceleration stuff, but making the software side messy too is not nice. Hashing should move to a linker-list approach for the implementation, and probably driver model for the hardware acceleration. Regards, Simon
On Sat, Jul 20, 2024 at 01:36:02PM +0100, Simon Glass wrote: > Hi Tom, > > On Fri, 19 Jul 2024 at 16:25, Tom Rini <trini@konsulko.com> wrote: > > > > On Fri, Jul 19, 2024 at 04:05:09PM +0100, Simon Glass wrote: > > > Hi Raymond, > > > > > > On Thu, 18 Jul 2024 at 17:46, Raymond Mao <raymond.mao@linaro.org> wrote: > > > > > > > > Hi Simon, > > > > > > > > On Fri, 5 Jul 2024 at 04:36, Simon Glass <sjg@chromium.org> wrote: > > > >> > > > >> Hi, > > > >> > > > >> On Wed, Jul 3, 2024, 09:56 Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > > >> > > > > >> > Hi Raymond > > > >> > > > > >> > On Tue, 2 Jul 2024 at 21:27, 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. > > > >> > > > > > >> > > common/hash.c | 143 ++++++++++++++++++++++++++++++++++++++++++++++++++ > > > >> > > 1 file changed, 143 insertions(+) > > > >> > > > > > >> > > diff --git a/common/hash.c b/common/hash.c > > > >> > > index ac63803fed9..96caf074374 100644 > > > >> > > --- a/common/hash.c > > > >> > > +++ b/common/hash.c > > > >> > > @@ -35,6 +35,141 @@ > > > >> > > #include <u-boot/sha512.h> > > > >> > > #include <u-boot/md5.h> > > > >> > > > > > >> > > +#if CONFIG_IS_ENABLED(MBEDTLS_LIB_CRYPTO) > > > >> > > + > > > >> > > +static int hash_init_sha1(struct hash_algo *algo, void **ctxp) > > > >> > > +{ > > > >> > > + int ret; > > > >> > > + mbedtls_sha1_context *ctx = malloc(sizeof(mbedtls_sha1_context)); > > > >> > > > >> > > > >> Why do we need allocation here? We should avoid it where possible. > > > >> > > > > The API "hash_init_sha1(struct hash_algo *algo, void **ctxp)" is passing a pointer > > > > address and expecting to get the context from the pointer, it is reasonable to do the > > > > allocation. > > > > On top of that, this patch doesn't make changes on this API itself, but just adapted > > > > it to MbedTLS stacks, thus you can see the allocation is needed by the original API > > > > as well. > > > > > > Oh dear., I see Now I am looking at the code. It is full of #ifdefs > > > for different cases. > > > > > > The whole thing needs a bit of a rationalisation before adding another case. > > > > If you're referring too the hash_algo struct, I'm not sure we can do > > something different that doesn't in turn increase size globally. And > > long term some of this may be able to go away if we can remove > > non-mbedTLS options. > > Well, at least using the existing functions rather than writing > entirely new ones would help. > > The real culprit here is the hardware-acceleration stuff, but making > the software side messy too is not nice. Hashing should move to a > linker-list approach for the implementation, and probably driver model > for the hardware acceleration. I'm fine with looking at that, after mbedtls is merged and we're looking at what can be removed / cleaned up. Perhaps we'll be able to shift most/all of the hardware assisted algorithms to being handled within mbedtls, I don't know.
Hi Tom, On Sat, 20 Jul 2024 at 18:13, Tom Rini <trini@konsulko.com> wrote: > > On Sat, Jul 20, 2024 at 01:36:02PM +0100, Simon Glass wrote: > > Hi Tom, > > > > On Fri, 19 Jul 2024 at 16:25, Tom Rini <trini@konsulko.com> wrote: > > > > > > On Fri, Jul 19, 2024 at 04:05:09PM +0100, Simon Glass wrote: > > > > Hi Raymond, > > > > > > > > On Thu, 18 Jul 2024 at 17:46, Raymond Mao <raymond.mao@linaro.org> wrote: > > > > > > > > > > Hi Simon, > > > > > > > > > > On Fri, 5 Jul 2024 at 04:36, Simon Glass <sjg@chromium.org> wrote: > > > > >> > > > > >> Hi, > > > > >> > > > > >> On Wed, Jul 3, 2024, 09:56 Ilias Apalodimas <ilias.apalodimas@linaro.org> wrote: > > > > >> > > > > > >> > Hi Raymond > > > > >> > > > > > >> > On Tue, 2 Jul 2024 at 21:27, 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. > > > > >> > > > > > > >> > > common/hash.c | 143 ++++++++++++++++++++++++++++++++++++++++++++++++++ > > > > >> > > 1 file changed, 143 insertions(+) > > > > >> > > > > > > >> > > diff --git a/common/hash.c b/common/hash.c > > > > >> > > index ac63803fed9..96caf074374 100644 > > > > >> > > --- a/common/hash.c > > > > >> > > +++ b/common/hash.c > > > > >> > > @@ -35,6 +35,141 @@ > > > > >> > > #include <u-boot/sha512.h> > > > > >> > > #include <u-boot/md5.h> > > > > >> > > > > > > >> > > +#if CONFIG_IS_ENABLED(MBEDTLS_LIB_CRYPTO) > > > > >> > > + > > > > >> > > +static int hash_init_sha1(struct hash_algo *algo, void **ctxp) > > > > >> > > +{ > > > > >> > > + int ret; > > > > >> > > + mbedtls_sha1_context *ctx = malloc(sizeof(mbedtls_sha1_context)); > > > > >> > > > > >> > > > > >> Why do we need allocation here? We should avoid it where possible. > > > > >> > > > > > The API "hash_init_sha1(struct hash_algo *algo, void **ctxp)" is passing a pointer > > > > > address and expecting to get the context from the pointer, it is reasonable to do the > > > > > allocation. > > > > > On top of that, this patch doesn't make changes on this API itself, but just adapted > > > > > it to MbedTLS stacks, thus you can see the allocation is needed by the original API > > > > > as well. > > > > > > > > Oh dear., I see Now I am looking at the code. It is full of #ifdefs > > > > for different cases. > > > > > > > > The whole thing needs a bit of a rationalisation before adding another case. > > > > > > If you're referring too the hash_algo struct, I'm not sure we can do > > > something different that doesn't in turn increase size globally. And > > > long term some of this may be able to go away if we can remove > > > non-mbedTLS options. > > > > Well, at least using the existing functions rather than writing > > entirely new ones would help. > > > > The real culprit here is the hardware-acceleration stuff, but making > > the software side messy too is not nice. Hashing should move to a > > linker-list approach for the implementation, and probably driver model > > for the hardware acceleration. > > I'm fine with looking at that, after mbedtls is merged and we're looking > at what can be removed / cleaned up. Perhaps we'll be able to shift > most/all of the hardware assisted algorithms to being handled within > mbedtls, I don't know. So long as the same person does the clean-up straight afterwards, that sounds fine. Otherwise, no one else will take it on as this series makes it harder. For hardware-assist, I think we need a driver model solution with a proper API. Regards, Simon
Hi Tom, On Sat, 20 Jul 2024 at 13:13, Tom Rini <trini@konsulko.com> wrote: > On Sat, Jul 20, 2024 at 01:36:02PM +0100, Simon Glass wrote: > > Hi Tom, > > > > On Fri, 19 Jul 2024 at 16:25, Tom Rini <trini@konsulko.com> wrote: > > > > > > On Fri, Jul 19, 2024 at 04:05:09PM +0100, Simon Glass wrote: > > > > Hi Raymond, > > > > > > > > On Thu, 18 Jul 2024 at 17:46, Raymond Mao <raymond.mao@linaro.org> > wrote: > > > > > > > > > > Hi Simon, > > > > > > > > > > On Fri, 5 Jul 2024 at 04:36, Simon Glass <sjg@chromium.org> wrote: > > > > >> > > > > >> Hi, > > > > >> > > > > >> On Wed, Jul 3, 2024, 09:56 Ilias Apalodimas < > ilias.apalodimas@linaro.org> wrote: > > > > >> > > > > > >> > Hi Raymond > > > > >> > > > > > >> > On Tue, 2 Jul 2024 at 21:27, 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. > > > > >> > > > > > > >> > > common/hash.c | 143 > ++++++++++++++++++++++++++++++++++++++++++++++++++ > > > > >> > > 1 file changed, 143 insertions(+) > > > > >> > > > > > > >> > > diff --git a/common/hash.c b/common/hash.c > > > > >> > > index ac63803fed9..96caf074374 100644 > > > > >> > > --- a/common/hash.c > > > > >> > > +++ b/common/hash.c > > > > >> > > @@ -35,6 +35,141 @@ > > > > >> > > #include <u-boot/sha512.h> > > > > >> > > #include <u-boot/md5.h> > > > > >> > > > > > > >> > > +#if CONFIG_IS_ENABLED(MBEDTLS_LIB_CRYPTO) > > > > >> > > + > > > > >> > > +static int hash_init_sha1(struct hash_algo *algo, void > **ctxp) > > > > >> > > +{ > > > > >> > > + int ret; > > > > >> > > + mbedtls_sha1_context *ctx = > malloc(sizeof(mbedtls_sha1_context)); > > > > >> > > > > >> > > > > >> Why do we need allocation here? We should avoid it where possible. > > > > >> > > > > > The API "hash_init_sha1(struct hash_algo *algo, void **ctxp)" is > passing a pointer > > > > > address and expecting to get the context from the pointer, it is > reasonable to do the > > > > > allocation. > > > > > On top of that, this patch doesn't make changes on this API > itself, but just adapted > > > > > it to MbedTLS stacks, thus you can see the allocation is needed by > the original API > > > > > as well. > > > > > > > > Oh dear., I see Now I am looking at the code. It is full of #ifdefs > > > > for different cases. > > > > > > > > The whole thing needs a bit of a rationalisation before adding > another case. > > > > > > If you're referring too the hash_algo struct, I'm not sure we can do > > > something different that doesn't in turn increase size globally. And > > > long term some of this may be able to go away if we can remove > > > non-mbedTLS options. > > > > Well, at least using the existing functions rather than writing > > entirely new ones would help. > > > > The real culprit here is the hardware-acceleration stuff, but making > > the software side messy too is not nice. Hashing should move to a > > linker-list approach for the implementation, and probably driver model > > for the hardware acceleration. > > I'm fine with looking at that, after mbedtls is merged and we're looking > at what can be removed / cleaned up. Perhaps we'll be able to shift > most/all of the hardware assisted algorithms to being handled within > mbedtls, I don't know. > > Yes, I think to handle this within mbedtls is straight forward. Regards, Raymond
Hi Simon, On Sat, 20 Jul 2024 at 08:36, Simon Glass <sjg@chromium.org> wrote: > Hi Tom, > > On Fri, 19 Jul 2024 at 16:25, Tom Rini <trini@konsulko.com> wrote: > > > > On Fri, Jul 19, 2024 at 04:05:09PM +0100, Simon Glass wrote: > > > Hi Raymond, > > > > > > On Thu, 18 Jul 2024 at 17:46, Raymond Mao <raymond.mao@linaro.org> > wrote: > > > > > > > > Hi Simon, > > > > > > > > On Fri, 5 Jul 2024 at 04:36, Simon Glass <sjg@chromium.org> wrote: > > > >> > > > >> Hi, > > > >> > > > >> On Wed, Jul 3, 2024, 09:56 Ilias Apalodimas < > ilias.apalodimas@linaro.org> wrote: > > > >> > > > > >> > Hi Raymond > > > >> > > > > >> > On Tue, 2 Jul 2024 at 21:27, 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. > > > >> > > > > > >> > > common/hash.c | 143 > ++++++++++++++++++++++++++++++++++++++++++++++++++ > > > >> > > 1 file changed, 143 insertions(+) > > > >> > > > > > >> > > diff --git a/common/hash.c b/common/hash.c > > > >> > > index ac63803fed9..96caf074374 100644 > > > >> > > --- a/common/hash.c > > > >> > > +++ b/common/hash.c > > > >> > > @@ -35,6 +35,141 @@ > > > >> > > #include <u-boot/sha512.h> > > > >> > > #include <u-boot/md5.h> > > > >> > > > > > >> > > +#if CONFIG_IS_ENABLED(MBEDTLS_LIB_CRYPTO) > > > >> > > + > > > >> > > +static int hash_init_sha1(struct hash_algo *algo, void **ctxp) > > > >> > > +{ > > > >> > > + int ret; > > > >> > > + mbedtls_sha1_context *ctx = > malloc(sizeof(mbedtls_sha1_context)); > > > >> > > > >> > > > >> Why do we need allocation here? We should avoid it where possible. > > > >> > > > > The API "hash_init_sha1(struct hash_algo *algo, void **ctxp)" is > passing a pointer > > > > address and expecting to get the context from the pointer, it is > reasonable to do the > > > > allocation. > > > > On top of that, this patch doesn't make changes on this API itself, > but just adapted > > > > it to MbedTLS stacks, thus you can see the allocation is needed by > the original API > > > > as well. > > > > > > Oh dear., I see Now I am looking at the code. It is full of #ifdefs > > > for different cases. > > > > > > The whole thing needs a bit of a rationalisation before adding another > case. > > > > If you're referring too the hash_algo struct, I'm not sure we can do > > something different that doesn't in turn increase size globally. And > > long term some of this may be able to go away if we can remove > > non-mbedTLS options. > > Well, at least using the existing functions rather than writing > entirely new ones would help. > > I understand that now the changes are not perfect but it is a trade-off at the moment. I prefer to new functions, in this case we just need to add "CONFIG_IS_ENABLED(MBEDTLS_LIB_CRYPTO)" only once. Otherwise, we have to add this build option under every function and make it messy and bad for reviewing. Regards, Raymond
diff --git a/common/hash.c b/common/hash.c index ac63803fed9..96caf074374 100644 --- a/common/hash.c +++ b/common/hash.c @@ -35,6 +35,141 @@ #include <u-boot/sha512.h> #include <u-boot/md5.h> +#if CONFIG_IS_ENABLED(MBEDTLS_LIB_CRYPTO) + +static int hash_init_sha1(struct hash_algo *algo, void **ctxp) +{ + int ret; + mbedtls_sha1_context *ctx = malloc(sizeof(mbedtls_sha1_context)); + + 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 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 +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 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 = malloc(sizeof(mbedtls_sha256_context)); + + 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 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 +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 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 = malloc(sizeof(mbedtls_sha512_context)); + + 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 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 +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 +278,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 +404,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. common/hash.c | 143 ++++++++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 143 insertions(+)