Re: [PATCH 05/13] efi_loader: signature: make efi_hash_regions more generic
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
> > ---
> > 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
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", , 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(, 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, , )) ||
> > /* for signed image */
> > - efi_hash_regions(regs, , )) {
> > + efi_hash_regions(regs->reg, regs->num, , )) {
> > /* 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, , )) {
> > + if (!efi_hash_regions(regs->reg,
Re: [PATCH 05/13] efi_loader: signature: make efi_hash_regions more generic
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
> ---
> 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?
Otherwise
Reviewed-by: Heinrich Schuchardt
> {
> - *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", , 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(, 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, , )) ||
> /* for signed image */
> - efi_hash_regions(regs, , )) {
> + efi_hash_regions(regs->reg, regs->num, , )) {
> /* 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, , )) {
> + if (!efi_hash_regions(regs->reg, regs->num, , )) {
> debug("Digesting unsigned image failed\n");
> goto out;
> }
>
[PATCH 05/13] efi_loader: signature: make efi_hash_regions more generic
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
---
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)
{
- *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", , 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(, 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, , )) ||
/* for signed image */
- efi_hash_regions(regs, , )) {
+ efi_hash_regions(regs->reg, regs->num, , )) {
/* 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, , )) {
+ if (!efi_hash_regions(regs->reg, regs->num, , )) {
debug("Digesting unsigned image failed\n");
goto out;
}
--
2.25.2
