| Message ID | 20241003215112.3103601-6-raymond.mao@linaro.org |
|---|---|
| State | New |
| Delegated to: | Tom Rini |
| Headers | show |
| Series | Integrate MbedTLS v3.6 LTS with U-Boot | expand |
Raymond Mao <raymond.mao@linaro.org> writes: > We don't need an API specially for non-watchdog since sha1_csum_wd > supports it by disabling CONFIG_HW_WATCHDOG and CONFIG_WATCHDOG. > Set 0x10000 as default chunk size for SHA1. > I have to say I believe this is a step in the wrong direction. Having everybody call a function with that _wd suffix is ugly, as is having them all pass some pre-defined constant. Moreover, nowadays what happens every chunksize bytes isn't restricted to watchdog handling. So yes, we don't need an API specially for non-watchdog, but why not just make sha1_csum() be the interface to call, and let the "maybe we need to call schedule() once in a while" be an implementation detail of sha1_csum(). The same as with our zlib implementaion; we don't have a separate _wd set of routines, we've just hooked schedule() into the main loop of that inflate algorithm. Also, I think the IS_ENABLED(CONFIG_HW_WATCHDOG) || IS_ENABLED(CONFIG_WATCHDOG) guards are wrong and shouldn't be copy-pasted to new code. Preferably, the code shouldn't even have any ifdefs, but just always be built as a loop with the (possibly no-op) schedule() every xxx bytes. I just sent a series which is the beginning of cleaning up the CONFIG_HW_WATCHDOG/CONFIG_WATCHDOG/CONFIG_CYCLIC/when is schedule defined and when should it be called. Rasmus
Hi Rasmus, On Fri, 4 Oct 2024 at 03:50, Rasmus Villemoes <rasmus.villemoes@prevas.dk> wrote: > Raymond Mao <raymond.mao@linaro.org> writes: > > > We don't need an API specially for non-watchdog since sha1_csum_wd > > supports it by disabling CONFIG_HW_WATCHDOG and CONFIG_WATCHDOG. > > Set 0x10000 as default chunk size for SHA1. > > > > I have to say I believe this is a step in the wrong direction. Having > everybody call a function with that _wd suffix is ugly, as is having > them all pass some pre-defined constant. Moreover, nowadays what happens > every chunksize bytes isn't restricted to watchdog handling. > > So yes, we don't need an API specially for non-watchdog, but why not > just make sha1_csum() be the interface to call, and let the "maybe we > need to call schedule() once in a while" be an implementation detail of > sha1_csum(). > > The same as with our zlib implementaion; we don't have a separate _wd > set of routines, we've just hooked schedule() into the main loop of that > inflate algorithm. > > The reason I keep the ` _csum_wd()` one is to align to other hash APIs currently we have. For MD5, SHA256, SHA512, we only have `_csum_wd()` but no `_csum()` - SHA1 is a special one due to the historical problem I guess. The minimum refactoring I did here is to unify the interface and make it adaptable to the MbedTLS library I introduced with this series. Yes, I agree some optimizations or refactoring are needed for the hash APIs, but I will prefer to do this in a new series other than increasing the scope of this patch set. [snip] Regards, Raymond
diff --git a/board/gdsys/a38x/hre.c b/board/gdsys/a38x/hre.c index f303793b63b..06856ea36d3 100644 --- a/board/gdsys/a38x/hre.c +++ b/board/gdsys/a38x/hre.c @@ -166,7 +166,7 @@ static int find_key(struct udevice *tpm, const uint8_t auth[20], return -1; if (err) continue; - sha1_csum(buf, buf_len, digest); + sha1_csum_wd(buf, buf_len, digest, SHA1_DEF_CHUNK_SZ); if (!memcmp(digest, pubkey_digest, 20)) { *handle = key_handles[i]; return 0; diff --git a/include/u-boot/sha1.h b/include/u-boot/sha1.h index ab88134fb98..36c3db15e22 100644 --- a/include/u-boot/sha1.h +++ b/include/u-boot/sha1.h @@ -39,6 +39,8 @@ extern "C" { #define SHA1_SUM_LEN 20 #define SHA1_DER_LEN 15 +#define SHA1_DEF_CHUNK_SZ 0x10000 + extern const uint8_t sha1_der_prefix[]; #if defined(CONFIG_MBEDTLS_LIB_CRYPTO) @@ -81,16 +83,6 @@ void sha1_update(sha1_context *ctx, const unsigned char *input, */ void sha1_finish( sha1_context *ctx, unsigned char output[20] ); -/** - * \brief Output = SHA-1( input buffer ) - * - * \param input buffer holding the data - * \param ilen length of the input data - * \param output SHA-1 checksum result - */ -void sha1_csum(const unsigned char *input, unsigned int ilen, - unsigned char *output); - /** * \brief Output = SHA-1( input buffer ), with watchdog triggering * diff --git a/lib/sha1.c b/lib/sha1.c index 7ef536f4b5d..81412283b49 100644 --- a/lib/sha1.c +++ b/lib/sha1.c @@ -304,19 +304,6 @@ void sha1_finish (sha1_context * ctx, unsigned char output[20]) PUT_UINT32_BE (ctx->state[4], output, 16); } -/* - * Output = SHA-1( input buffer ) - */ -void sha1_csum(const unsigned char *input, unsigned int ilen, - unsigned char *output) -{ - sha1_context ctx; - - sha1_starts (&ctx); - sha1_update (&ctx, input, ilen); - sha1_finish (&ctx, output); -} - /* * Output = SHA-1( input buffer ). Trigger the watchdog every 'chunk_sz' * bytes of input processed. diff --git a/lib/tpm-v1.c b/lib/tpm-v1.c index e66023da5e6..a6727c575fd 100644 --- a/lib/tpm-v1.c +++ b/lib/tpm-v1.c @@ -871,7 +871,7 @@ u32 tpm1_find_key_sha1(struct udevice *dev, const u8 auth[20], return -1; if (err) continue; - sha1_csum(buf, buf_len, digest); + sha1_csum_wd(buf, buf_len, digest, SHA1_DEF_CHUNK_SZ); if (!memcmp(digest, pubkey_digest, 20)) { *handle = key_handles[i]; return 0;