[2/3] fit: Use hash.c to call SHA code

Message ID 20210217032042.425512-3-joel@jms.id.au
State New
Headers show
Series
  • Improvements to FIT hashing
Related show

Commit Message

Joel Stanley Feb. 17, 2021, 3:20 a.m.
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

Comments

AKASHI Takahiro Feb. 17, 2021, 5:03 a.m. | #1
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

>
Joel Stanley April 12, 2021, 11:52 p.m. | #2
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

> >
Tom Rini April 13, 2021, 2:30 a.m. | #3
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

Patch

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;
 }