[PATCH 05/13] efi_loader: signature: make efi_hash_regions more generic

[AKASHI Takahiro]

Heinrich,

On Sat, May 30, 2020 at 08:58:02AM +0200, Heinrich Schuchardt wrote:
> On 5/29/20 8:41 AM, AKASHI Takahiro wrote:
> > 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 at linaro.org>
> > ---
> >  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 35f678de057e..00e442783059 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)
> 
> What is this size parameter good for if we know all signatures are
> SHA256? Should it be eliminiated?

It would make more sense when we support algorithms other than sha256.
The caller, then, could look like:

  efi_hash_regions("sha256", regs, count, hash, size);
  if (!memcmp(data, hash, size))
    ...

That way, callers in different use cases won't have to use an immediate
value for actual hash size.
For example, an certificate entry in the revocation list can be
digested with, according to UEFI specification,
  EFI_CERT_X509_SHA256_GUID
  EFI_CERT_X509_SHA384_GUID
  EFI_CERT_X509_SHA512_GUID

Meanwhile, signatures can be different type of message digests, as well.
  EFI_CERT_SHA1_GUID
  EFI_CERT_SHA224_GUID
  EFI_CERT_SHA256_GUID
  EFI_CERT_SHA384_GUID
  EFI_CERT_SHA512_GUID

> Otherwise
> Reviewed-by: Heinrich Schuchardt <xypron.glpk at gmx.de>

Thanks,
-Takahiro Akashi


> >  {
> > -	*size = 0;
> > -	*hash = calloc(1, SHA256_SUM_LEN);
> >  	if (!*hash) {
> > -		debug("Out of memory\n");
> > -		return false;
> > +		*hash = calloc(1, SHA256_SUM_LEN);
> > +		if (!*hash) {
> > +			debug("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
> >  	debug("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) {
> > -		debug("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
> > -	debug("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);
> >  }
> >
> >  /**
> > @@ -168,9 +155,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)) {
> > @@ -238,7 +226,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;
> >
> >  		debug("%s: unsigned image\n", __func__);
> > @@ -252,7 +240,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)) {
> >  			debug("Digesting unsigned image failed\n");
> >  			goto out;
> >  		}
> >
>