| Message ID | 20210217032042.425512-3-joel@jms.id.au |
|---|---|
| State | New |
| Headers | show |
| Series | Improvements to FIT hashing | expand |
Simon, # This is not a direct comment on this patch. On Wed, Feb 17, 2021 at 01:50:41PM +1030, Joel Stanley wrote: > Currently the FIT hashing will call directly into the SHA algorithms to > get a hash. > > This moves the fit code to use hash_lookup_algo, giving a common > entrypoint into the hashing code and removing the duplicated algorithm > look up. It also allows the use of hardware acceleration if configured. > > Signed-off-by: Joel Stanley <joel@jms.id.au> > --- > common/image-fit.c | 34 ++++++++-------------------------- > 1 file changed, 8 insertions(+), 26 deletions(-) > > diff --git a/common/image-fit.c b/common/image-fit.c > index 28b3d2b19111..3451cdecc95b 100644 > --- a/common/image-fit.c > +++ b/common/image-fit.c > @@ -1210,37 +1210,19 @@ int fit_set_timestamp(void *fit, int noffset, time_t timestamp) > * 0, on success > * -1, when algo is unsupported > */ > -int calculate_hash(const void *data, int data_len, const char *algo, > +int calculate_hash(const void *data, int data_len, const char *algo_name, > uint8_t *value, int *value_len) > { > - if (IMAGE_ENABLE_CRC32 && strcmp(algo, "crc32") == 0) { > - *((uint32_t *)value) = crc32_wd(0, data, data_len, > - CHUNKSZ_CRC32); > - *((uint32_t *)value) = cpu_to_uimage(*((uint32_t *)value)); > - *value_len = 4; > - } else if (IMAGE_ENABLE_SHA1 && strcmp(algo, "sha1") == 0) { > - sha1_csum_wd((unsigned char *)data, data_len, > - (unsigned char *)value, CHUNKSZ_SHA1); > - *value_len = 20; > - } else if (IMAGE_ENABLE_SHA256 && strcmp(algo, "sha256") == 0) { > - sha256_csum_wd((unsigned char *)data, data_len, > - (unsigned char *)value, CHUNKSZ_SHA256); > - *value_len = SHA256_SUM_LEN; > - } else if (IMAGE_ENABLE_SHA384 && strcmp(algo, "sha384") == 0) { > - sha384_csum_wd((unsigned char *)data, data_len, > - (unsigned char *)value, CHUNKSZ_SHA384); > - *value_len = SHA384_SUM_LEN; > - } else if (IMAGE_ENABLE_SHA512 && strcmp(algo, "sha512") == 0) { > - sha512_csum_wd((unsigned char *)data, data_len, > - (unsigned char *)value, CHUNKSZ_SHA512); > - *value_len = SHA512_SUM_LEN; > - } else if (IMAGE_ENABLE_MD5 && strcmp(algo, "md5") == 0) { > - md5_wd((unsigned char *)data, data_len, value, CHUNKSZ_MD5); > - *value_len = 16; > - } else { > + struct hash_algo *algo; > + > + if (hash_lookup_algo(algo_name, &algo)) { > debug("Unsupported hash alogrithm\n"); > return -1; > } > + > + algo->hash_func_ws(data, data_len, value, algo->chunk_size); > + *value_len = algo->digest_size; With this patch applied, there co-exists a very similar, hence confusing function, hash_calculate(), in rsa-checksum.c (now checksum.c?). If there is no particular reason for those two functions, we'd better unify them? -Takahiro Akashi > return 0; > } > > -- > 2.30.0 >
On Wed, 17 Feb 2021 at 05:04, AKASHI Takahiro <takahiro.akashi@linaro.org> wrote: > > Simon, > > # This is not a direct comment on this patch. > > On Wed, Feb 17, 2021 at 01:50:41PM +1030, Joel Stanley wrote: > > Currently the FIT hashing will call directly into the SHA algorithms to > > get a hash. > > > > This moves the fit code to use hash_lookup_algo, giving a common > > entrypoint into the hashing code and removing the duplicated algorithm > > look up. It also allows the use of hardware acceleration if configured. > > > > Signed-off-by: Joel Stanley <joel@jms.id.au> > > --- > > common/image-fit.c | 34 ++++++++-------------------------- > > 1 file changed, 8 insertions(+), 26 deletions(-) > > > > diff --git a/common/image-fit.c b/common/image-fit.c > > index 28b3d2b19111..3451cdecc95b 100644 > > --- a/common/image-fit.c > > +++ b/common/image-fit.c > > @@ -1210,37 +1210,19 @@ int fit_set_timestamp(void *fit, int noffset, time_t timestamp) > > * 0, on success > > * -1, when algo is unsupported > > */ > > -int calculate_hash(const void *data, int data_len, const char *algo, > > +int calculate_hash(const void *data, int data_len, const char *algo_name, > > uint8_t *value, int *value_len) > > { > > - if (IMAGE_ENABLE_CRC32 && strcmp(algo, "crc32") == 0) { > > - *((uint32_t *)value) = crc32_wd(0, data, data_len, > > - CHUNKSZ_CRC32); > > - *((uint32_t *)value) = cpu_to_uimage(*((uint32_t *)value)); > > - *value_len = 4; > > - } else if (IMAGE_ENABLE_SHA1 && strcmp(algo, "sha1") == 0) { > > - sha1_csum_wd((unsigned char *)data, data_len, > > - (unsigned char *)value, CHUNKSZ_SHA1); > > - *value_len = 20; > > - } else if (IMAGE_ENABLE_SHA256 && strcmp(algo, "sha256") == 0) { > > - sha256_csum_wd((unsigned char *)data, data_len, > > - (unsigned char *)value, CHUNKSZ_SHA256); > > - *value_len = SHA256_SUM_LEN; > > - } else if (IMAGE_ENABLE_SHA384 && strcmp(algo, "sha384") == 0) { > > - sha384_csum_wd((unsigned char *)data, data_len, > > - (unsigned char *)value, CHUNKSZ_SHA384); > > - *value_len = SHA384_SUM_LEN; > > - } else if (IMAGE_ENABLE_SHA512 && strcmp(algo, "sha512") == 0) { > > - sha512_csum_wd((unsigned char *)data, data_len, > > - (unsigned char *)value, CHUNKSZ_SHA512); > > - *value_len = SHA512_SUM_LEN; > > - } else if (IMAGE_ENABLE_MD5 && strcmp(algo, "md5") == 0) { > > - md5_wd((unsigned char *)data, data_len, value, CHUNKSZ_MD5); > > - *value_len = 16; > > - } else { > > + struct hash_algo *algo; > > + > > + if (hash_lookup_algo(algo_name, &algo)) { > > debug("Unsupported hash alogrithm\n"); > > return -1; > > } > > + > > + algo->hash_func_ws(data, data_len, value, algo->chunk_size); > > + *value_len = algo->digest_size; > > With this patch applied, there co-exists a very similar, hence > confusing function, hash_calculate(), in rsa-checksum.c (now checksum.c?). > If there is no particular reason for those two functions, > we'd better unify them? hash_calculate is doing a progressive hash over a count of regions. This code is hashing a single chunk of data. I agree the naming could be improved to make this clearer. Cheers, Joel > > -Takahiro Akashi > > > > return 0; > > } > > > > -- > > 2.30.0 > >
On Wed, Feb 17, 2021 at 01:50:41PM +1030, Joel Stanley wrote: > Currently the FIT hashing will call directly into the SHA algorithms to > get a hash. > > This moves the fit code to use hash_lookup_algo, giving a common > entrypoint into the hashing code and removing the duplicated algorithm > look up. It also allows the use of hardware acceleration if configured. > > Signed-off-by: Joel Stanley <joel@jms.id.au> This breaks a few boards: ls1046ardb_qspi_spl imx8mm_beacon imx8mn_beacon imx8mn_beacon_2g imx8mm_evk imx8mn_ddr4_evk imx8mn_evk imx8mp_evk imx8mq_evk imx8mm_venice imx8mq_phanbell phycore-imx8mm phycore-imx8mp pico-imx8mq verdin-imx8mm mt8183_pumpkin mt8516_pumpkin that use FIT images, in SPL, but don't actually check hashes apparently. Just including hash.o for the hash-lookup function fails because we don't have crc16, etc, enabled and also I think need: https://patchwork.ozlabs.org/project/uboot/patch/20210322133331.1646575-1-mr.nuke.me@gmail.com/ for consistent symbol naming. So, I like this patch in concept, but I think it'll need to be reworked a bit, after I pull in some other changes soon. Thanks! -- Tom
diff --git a/common/image-fit.c b/common/image-fit.c index 28b3d2b19111..3451cdecc95b 100644 --- a/common/image-fit.c +++ b/common/image-fit.c @@ -1210,37 +1210,19 @@ int fit_set_timestamp(void *fit, int noffset, time_t timestamp) * 0, on success * -1, when algo is unsupported */ -int calculate_hash(const void *data, int data_len, const char *algo, +int calculate_hash(const void *data, int data_len, const char *algo_name, uint8_t *value, int *value_len) { - if (IMAGE_ENABLE_CRC32 && strcmp(algo, "crc32") == 0) { - *((uint32_t *)value) = crc32_wd(0, data, data_len, - CHUNKSZ_CRC32); - *((uint32_t *)value) = cpu_to_uimage(*((uint32_t *)value)); - *value_len = 4; - } else if (IMAGE_ENABLE_SHA1 && strcmp(algo, "sha1") == 0) { - sha1_csum_wd((unsigned char *)data, data_len, - (unsigned char *)value, CHUNKSZ_SHA1); - *value_len = 20; - } else if (IMAGE_ENABLE_SHA256 && strcmp(algo, "sha256") == 0) { - sha256_csum_wd((unsigned char *)data, data_len, - (unsigned char *)value, CHUNKSZ_SHA256); - *value_len = SHA256_SUM_LEN; - } else if (IMAGE_ENABLE_SHA384 && strcmp(algo, "sha384") == 0) { - sha384_csum_wd((unsigned char *)data, data_len, - (unsigned char *)value, CHUNKSZ_SHA384); - *value_len = SHA384_SUM_LEN; - } else if (IMAGE_ENABLE_SHA512 && strcmp(algo, "sha512") == 0) { - sha512_csum_wd((unsigned char *)data, data_len, - (unsigned char *)value, CHUNKSZ_SHA512); - *value_len = SHA512_SUM_LEN; - } else if (IMAGE_ENABLE_MD5 && strcmp(algo, "md5") == 0) { - md5_wd((unsigned char *)data, data_len, value, CHUNKSZ_MD5); - *value_len = 16; - } else { + struct hash_algo *algo; + + if (hash_lookup_algo(algo_name, &algo)) { debug("Unsupported hash alogrithm\n"); return -1; } + + algo->hash_func_ws(data, data_len, value, algo->chunk_size); + *value_len = algo->digest_size; + return 0; }
Currently the FIT hashing will call directly into the SHA algorithms to get a hash. This moves the fit code to use hash_lookup_algo, giving a common entrypoint into the hashing code and removing the duplicated algorithm look up. It also allows the use of hardware acceleration if configured. Signed-off-by: Joel Stanley <joel@jms.id.au> --- common/image-fit.c | 34 ++++++++-------------------------- 1 file changed, 8 insertions(+), 26 deletions(-) -- 2.30.0