diff mbox series

[v2,09/17] efi_loader: signature: make efi_hash_regions more generic

Message ID 20200609050947.17861-10-takahiro.akashi@linaro.org
State Superseded, archived
Delegated to: Heinrich Schuchardt
Headers show
Series efi_loader: rework/improve UEFI secure boot code | expand

Commit Message

AKASHI Takahiro June 9, 2020, 5:09 a.m. UTC
There are a couple of occurrences of hash calculations in which a new
efi_hash_regions will be commonly used.

Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
---
 lib/efi_loader/efi_signature.c | 44 +++++++++++++---------------------
 1 file changed, 16 insertions(+), 28 deletions(-)

Comments

Heinrich Schuchardt July 3, 2020, 11:08 a.m. UTC | #1
On 09.06.20 07:09, AKASHI Takahiro wrote:
> There are a couple of occurrences of hash calculations in which a new
> efi_hash_regions will be commonly used.

Please, describe the difference.

Do you want to calculate the hash over an interval of regions?

>
> Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>

Please, provide a test for efi_hash_regions() in test/lib/.

> ---
>  lib/efi_loader/efi_signature.c | 44 +++++++++++++---------------------
>  1 file changed, 16 insertions(+), 28 deletions(-)
>
> diff --git a/lib/efi_loader/efi_signature.c b/lib/efi_loader/efi_signature.c
> index f22dc151971f..03080bc0b11c 100644
> --- a/lib/efi_loader/efi_signature.c
> +++ b/lib/efi_loader/efi_signature.c
> @@ -30,6 +30,7 @@ const efi_guid_t efi_guid_cert_type_pkcs7 = EFI_CERT_TYPE_PKCS7_GUID;
>  /**
>   * efi_hash_regions - calculate a hash value
>   * @regs:	List of regions

The argument should be renamed and the description corrected:

@reg:	first region

Best regards

Heinrich

> + * @count:	Number of regions
>   * @hash:	Pointer to a pointer to buffer holding a hash value
>   * @size:	Size of buffer to be returned
>   *
> @@ -37,18 +38,20 @@ const efi_guid_t efi_guid_cert_type_pkcs7 = EFI_CERT_TYPE_PKCS7_GUID;
>   *
>   * Return:	true on success, false on error
>   */
> -static bool efi_hash_regions(struct efi_image_regions *regs, void **hash,
> -			     size_t *size)
> +static bool efi_hash_regions(struct image_region *regs, int count,
> +			     void **hash, size_t *size)
>  {
> -	*size = 0;
> -	*hash = calloc(1, SHA256_SUM_LEN);
>  	if (!*hash) {
> -		EFI_PRINT("Out of memory\n");
> -		return false;
> +		*hash = calloc(1, SHA256_SUM_LEN);
> +		if (!*hash) {
> +			EFI_PRINT("Out of memory\n");
> +			return false;
> +		}
>  	}
> -	*size = SHA256_SUM_LEN;
> +	if (size)
> +		*size = SHA256_SUM_LEN;
>
> -	hash_calculate("sha256", regs->reg, regs->num, *hash);
> +	hash_calculate("sha256", regs, count, *hash);
>  #ifdef DEBUG
>  	EFI_PRINT("hash calculated:\n");
>  	print_hex_dump("    ", DUMP_PREFIX_OFFSET, 16, 1,
> @@ -73,26 +76,10 @@ static bool efi_hash_msg_content(struct pkcs7_message *msg, void **hash,
>  {
>  	struct image_region regtmp;
>
> -	*size = 0;
> -	*hash = calloc(1, SHA256_SUM_LEN);
> -	if (!*hash) {
> -		EFI_PRINT("Out of memory\n");
> -		free(msg);
> -		return false;
> -	}
> -	*size = SHA256_SUM_LEN;
> -
>  	regtmp.data = msg->data;
>  	regtmp.size = msg->data_len;
>
> -	hash_calculate("sha256", &regtmp, 1, *hash);
> -#ifdef DEBUG
> -	EFI_PRINT("hash calculated based on contentInfo:\n");
> -	print_hex_dump("    ", DUMP_PREFIX_OFFSET, 16, 1,
> -		       *hash, SHA256_SUM_LEN, false);
> -#endif
> -
> -	return true;
> +	return efi_hash_regions(&regtmp, 1, hash, size);
>  }
>
>  /**
> @@ -170,9 +157,10 @@ static bool efi_signature_verify(struct efi_image_regions *regs,
>  			       false);
>  #endif
>  		/* against contentInfo first */
> +		hash = NULL;
>  		if ((msg->data && efi_hash_msg_content(msg, &hash, &size)) ||
>  				/* for signed image */
> -		    efi_hash_regions(regs, &hash, &size)) {
> +		    efi_hash_regions(regs->reg, regs->num, &hash, &size)) {
>  				/* for authenticated variable */
>  			if (ps_info->msgdigest_len != size ||
>  			    memcmp(hash, ps_info->msgdigest, size)) {
> @@ -240,7 +228,7 @@ bool efi_signature_verify_with_list(struct efi_image_regions *regs,
>  		  regs, signed_info, siglist, valid_cert);
>
>  	if (!signed_info) {
> -		void *hash;
> +		void *hash = NULL;
>  		size_t size;
>
>  		EFI_PRINT("%s: unsigned image\n", __func__);
> @@ -254,7 +242,7 @@ bool efi_signature_verify_with_list(struct efi_image_regions *regs,
>  			goto out;
>  		}
>
> -		if (!efi_hash_regions(regs, &hash, &size)) {
> +		if (!efi_hash_regions(regs->reg, regs->num, &hash, &size)) {
>  			EFI_PRINT("Digesting unsigned image failed\n");
>  			goto out;
>  		}
>
AKASHI Takahiro July 8, 2020, 1:22 a.m. UTC | #2
Heinrich,

On Fri, Jul 03, 2020 at 01:08:55PM +0200, Heinrich Schuchardt wrote:
> On 09.06.20 07:09, AKASHI Takahiro wrote:
> > There are a couple of occurrences of hash calculations in which a new
> > efi_hash_regions will be commonly used.
> 
> Please, describe the difference.

the difference of what?

> Do you want to calculate the hash over an interval of regions?

I don't get your point.
What do you mean by "over an interval"?

> >
> > Signed-off-by: AKASHI Takahiro <takahiro.akashi@linaro.org>
> 
> Please, provide a test for efi_hash_regions() in test/lib/.
> 
> > ---
> >  lib/efi_loader/efi_signature.c | 44 +++++++++++++---------------------
> >  1 file changed, 16 insertions(+), 28 deletions(-)
> >
> > diff --git a/lib/efi_loader/efi_signature.c b/lib/efi_loader/efi_signature.c
> > index f22dc151971f..03080bc0b11c 100644
> > --- a/lib/efi_loader/efi_signature.c
> > +++ b/lib/efi_loader/efi_signature.c
> > @@ -30,6 +30,7 @@ const efi_guid_t efi_guid_cert_type_pkcs7 = EFI_CERT_TYPE_PKCS7_GUID;
> >  /**
> >   * efi_hash_regions - calculate a hash value
> >   * @regs:	List of regions
> 
> The argument should be renamed and the description corrected:
> 
> @reg:	first region

No. You don't understand the code.
Here, I refactored the code and simply extracted a common code as
efi_hash_regions.
The first argument is an array of "struct image_region".
(To avoid any confusion, I will change the description to "Array of".)

-Takahiro Akashi

> Best regards
> 
> Heinrich
> 
> > + * @count:	Number of regions
> >   * @hash:	Pointer to a pointer to buffer holding a hash value
> >   * @size:	Size of buffer to be returned
> >   *
> > @@ -37,18 +38,20 @@ const efi_guid_t efi_guid_cert_type_pkcs7 = EFI_CERT_TYPE_PKCS7_GUID;
> >   *
> >   * Return:	true on success, false on error
> >   */
> > -static bool efi_hash_regions(struct efi_image_regions *regs, void **hash,
> > -			     size_t *size)
> > +static bool efi_hash_regions(struct image_region *regs, int count,
> > +			     void **hash, size_t *size)
> >  {
> > -	*size = 0;
> > -	*hash = calloc(1, SHA256_SUM_LEN);
> >  	if (!*hash) {
> > -		EFI_PRINT("Out of memory\n");
> > -		return false;
> > +		*hash = calloc(1, SHA256_SUM_LEN);
> > +		if (!*hash) {
> > +			EFI_PRINT("Out of memory\n");
> > +			return false;
> > +		}
> >  	}
> > -	*size = SHA256_SUM_LEN;
> > +	if (size)
> > +		*size = SHA256_SUM_LEN;
> >
> > -	hash_calculate("sha256", regs->reg, regs->num, *hash);
> > +	hash_calculate("sha256", regs, count, *hash);
> >  #ifdef DEBUG
> >  	EFI_PRINT("hash calculated:\n");
> >  	print_hex_dump("    ", DUMP_PREFIX_OFFSET, 16, 1,
> > @@ -73,26 +76,10 @@ static bool efi_hash_msg_content(struct pkcs7_message *msg, void **hash,
> >  {
> >  	struct image_region regtmp;
> >
> > -	*size = 0;
> > -	*hash = calloc(1, SHA256_SUM_LEN);
> > -	if (!*hash) {
> > -		EFI_PRINT("Out of memory\n");
> > -		free(msg);
> > -		return false;
> > -	}
> > -	*size = SHA256_SUM_LEN;
> > -
> >  	regtmp.data = msg->data;
> >  	regtmp.size = msg->data_len;
> >
> > -	hash_calculate("sha256", &regtmp, 1, *hash);
> > -#ifdef DEBUG
> > -	EFI_PRINT("hash calculated based on contentInfo:\n");
> > -	print_hex_dump("    ", DUMP_PREFIX_OFFSET, 16, 1,
> > -		       *hash, SHA256_SUM_LEN, false);
> > -#endif
> > -
> > -	return true;
> > +	return efi_hash_regions(&regtmp, 1, hash, size);
> >  }
> >
> >  /**
> > @@ -170,9 +157,10 @@ static bool efi_signature_verify(struct efi_image_regions *regs,
> >  			       false);
> >  #endif
> >  		/* against contentInfo first */
> > +		hash = NULL;
> >  		if ((msg->data && efi_hash_msg_content(msg, &hash, &size)) ||
> >  				/* for signed image */
> > -		    efi_hash_regions(regs, &hash, &size)) {
> > +		    efi_hash_regions(regs->reg, regs->num, &hash, &size)) {
> >  				/* for authenticated variable */
> >  			if (ps_info->msgdigest_len != size ||
> >  			    memcmp(hash, ps_info->msgdigest, size)) {
> > @@ -240,7 +228,7 @@ bool efi_signature_verify_with_list(struct efi_image_regions *regs,
> >  		  regs, signed_info, siglist, valid_cert);
> >
> >  	if (!signed_info) {
> > -		void *hash;
> > +		void *hash = NULL;
> >  		size_t size;
> >
> >  		EFI_PRINT("%s: unsigned image\n", __func__);
> > @@ -254,7 +242,7 @@ bool efi_signature_verify_with_list(struct efi_image_regions *regs,
> >  			goto out;
> >  		}
> >
> > -		if (!efi_hash_regions(regs, &hash, &size)) {
> > +		if (!efi_hash_regions(regs->reg, regs->num, &hash, &size)) {
> >  			EFI_PRINT("Digesting unsigned image failed\n");
> >  			goto out;
> >  		}
> >
>
diff mbox series

Patch

diff --git a/lib/efi_loader/efi_signature.c b/lib/efi_loader/efi_signature.c
index f22dc151971f..03080bc0b11c 100644
--- a/lib/efi_loader/efi_signature.c
+++ b/lib/efi_loader/efi_signature.c
@@ -30,6 +30,7 @@  const efi_guid_t efi_guid_cert_type_pkcs7 = EFI_CERT_TYPE_PKCS7_GUID;
 /**
  * efi_hash_regions - calculate a hash value
  * @regs:	List of regions
+ * @count:	Number of regions
  * @hash:	Pointer to a pointer to buffer holding a hash value
  * @size:	Size of buffer to be returned
  *
@@ -37,18 +38,20 @@  const efi_guid_t efi_guid_cert_type_pkcs7 = EFI_CERT_TYPE_PKCS7_GUID;
  *
  * Return:	true on success, false on error
  */
-static bool efi_hash_regions(struct efi_image_regions *regs, void **hash,
-			     size_t *size)
+static bool efi_hash_regions(struct image_region *regs, int count,
+			     void **hash, size_t *size)
 {
-	*size = 0;
-	*hash = calloc(1, SHA256_SUM_LEN);
 	if (!*hash) {
-		EFI_PRINT("Out of memory\n");
-		return false;
+		*hash = calloc(1, SHA256_SUM_LEN);
+		if (!*hash) {
+			EFI_PRINT("Out of memory\n");
+			return false;
+		}
 	}
-	*size = SHA256_SUM_LEN;
+	if (size)
+		*size = SHA256_SUM_LEN;
 
-	hash_calculate("sha256", regs->reg, regs->num, *hash);
+	hash_calculate("sha256", regs, count, *hash);
 #ifdef DEBUG
 	EFI_PRINT("hash calculated:\n");
 	print_hex_dump("    ", DUMP_PREFIX_OFFSET, 16, 1,
@@ -73,26 +76,10 @@  static bool efi_hash_msg_content(struct pkcs7_message *msg, void **hash,
 {
 	struct image_region regtmp;
 
-	*size = 0;
-	*hash = calloc(1, SHA256_SUM_LEN);
-	if (!*hash) {
-		EFI_PRINT("Out of memory\n");
-		free(msg);
-		return false;
-	}
-	*size = SHA256_SUM_LEN;
-
 	regtmp.data = msg->data;
 	regtmp.size = msg->data_len;
 
-	hash_calculate("sha256", &regtmp, 1, *hash);
-#ifdef DEBUG
-	EFI_PRINT("hash calculated based on contentInfo:\n");
-	print_hex_dump("    ", DUMP_PREFIX_OFFSET, 16, 1,
-		       *hash, SHA256_SUM_LEN, false);
-#endif
-
-	return true;
+	return efi_hash_regions(&regtmp, 1, hash, size);
 }
 
 /**
@@ -170,9 +157,10 @@  static bool efi_signature_verify(struct efi_image_regions *regs,
 			       false);
 #endif
 		/* against contentInfo first */
+		hash = NULL;
 		if ((msg->data && efi_hash_msg_content(msg, &hash, &size)) ||
 				/* for signed image */
-		    efi_hash_regions(regs, &hash, &size)) {
+		    efi_hash_regions(regs->reg, regs->num, &hash, &size)) {
 				/* for authenticated variable */
 			if (ps_info->msgdigest_len != size ||
 			    memcmp(hash, ps_info->msgdigest, size)) {
@@ -240,7 +228,7 @@  bool efi_signature_verify_with_list(struct efi_image_regions *regs,
 		  regs, signed_info, siglist, valid_cert);
 
 	if (!signed_info) {
-		void *hash;
+		void *hash = NULL;
 		size_t size;
 
 		EFI_PRINT("%s: unsigned image\n", __func__);
@@ -254,7 +242,7 @@  bool efi_signature_verify_with_list(struct efi_image_regions *regs,
 			goto out;
 		}
 
-		if (!efi_hash_regions(regs, &hash, &size)) {
+		if (!efi_hash_regions(regs->reg, regs->num, &hash, &size)) {
 			EFI_PRINT("Digesting unsigned image failed\n");
 			goto out;
 		}