Akashi-san,
> > + {
> > + EFI_CERT_X509_SHA256_GUID,
> > + "sha256",
> > + 256,
>
> => SHA256_SUM_LEN * 8
>
Sure
> > + },
> > + {
> > + EFI_CERT_SHA256_GUID,
> > + "sha256",
> > + 256,
> > + },
> > + {
> > + EFI_CERT_X509_SHA384_GUID,
> > + "sha384",
> > + 384,
> > + },
> > + {
> > + EFI_CERT_X509_SHA512_GUID,
> > + "sha512",
> > + 512,
> > + },
> > +};
> > +
> > +#define MAX_GUID_TO_HASH_COUNT ARRAY_SIZE(guid_to_hash)
> > +
> > +/** guid_to_sha_str - return the sha string e.g "sha256" for a given guid
> > + * used on EFI security databases
> > + *
> > + * @guid: guid to check
> > + *
> > + * Return: len or 0 if no match is found
> > + */
> > +const char *guid_to_sha_str(const efi_guid_t *guid)
> > +{
> > + size_t i;
> > +
> > + for (i = 0; i < MAX_GUID_TO_HASH_COUNT; i++) {
> > + if (!guidcmp(guid, &guid_to_hash[i].guid))
> > + return guid_to_hash[i].algo;
> > + }
> > +
> > + return NULL;
> > +}
> > +
> > +/** guid_to_sha_len - return the sha size in bytes for a given guid
> > + * used on EFI security databases
> > + *
> > + * @guid: guid to check
> > + *
> > + * Return: len or 0 if no match is found
> > + */
> > +int guid_to_sha_len(const efi_guid_t *guid)
> > +{
> > + size_t i;
> > +
> > + for (i = 0; i < MAX_GUID_TO_HASH_COUNT; i++) {
> > + if (!guidcmp(guid, &guid_to_hash[i].guid))
> > + return guid_to_hash[i].bits / 8;
> > + }
> > +
> > + return 0;
> > +}
>
> If we have algo_to_len(), we don't need guid_to_sha_len().
True, I'll get rid of this
>
> > +
> > +/** algo_to_len - return the sha size in bytes for a given string
> > + *
> > + * @guid: string to check
> > + *
> > + * Return: len or 0 if no match is found
> > + */
> > +int algo_to_len(const char *algo)
>
> This function seems to be quite generic and useful for others.
> Why not implement it along with hash_calculate() in hash-checksum.c
> (without using guid_to_hash[]).
>
Tbh I have similar stuff in efi_tcg2.c, but they are all bound to EFI atm.
We could refactor all of the structures a bit and have a more generic
version. Can we leave it for now and I can send a refactoring series
after this gets merged?
> > +{
> > + size_t i;
> > +
> > + for (i = 0; i < MAX_GUID_TO_HASH_COUNT; i++) {
> > + if (!strcmp(algo, guid_to_hash[i].algo))
> > + return guid_to_hash[i].bits / 8;
> > + }
> > +
> > + return 0;
> > +}
> > diff --git a/lib/efi_loader/efi_signature.c b/lib/efi_loader/efi_signature.c
> > index 79ed077ae7dd..cf01f21b4d04 100644
> > --- a/lib/efi_loader/efi_signature.c
> > +++ b/lib/efi_loader/efi_signature.c
> > @@ -24,6 +24,8 @@ const efi_guid_t efi_guid_sha256 = EFI_CERT_SHA256_GUID;
> > const efi_guid_t efi_guid_cert_rsa2048 = EFI_CERT_RSA2048_GUID;
> > const efi_guid_t efi_guid_cert_x509 = EFI_CERT_X509_GUID;
> > const efi_guid_t efi_guid_cert_x509_sha256 = EFI_CERT_X509_SHA256_GUID;
> > +const efi_guid_t efi_guid_cert_x509_sha384 = EFI_CERT_X509_SHA384_GUID;
> > +const efi_guid_t efi_guid_cert_x509_sha512 = EFI_CERT_X509_SHA512_GUID;
> > const efi_guid_t efi_guid_cert_type_pkcs7 = EFI_CERT_TYPE_PKCS7_GUID;
> >
> > static u8 pkcs7_hdr[] = {
> > @@ -124,23 +126,32 @@ struct pkcs7_message *efi_parse_pkcs7_header(const
> > void *buf,
> > * Return: true on success, false on error
> > */
> > static bool efi_hash_regions(struct image_region *regs, int count,
> > - void **hash, size_t *size)
> > + void **hash, const char *hash_algo, int *len)
> > {
> > + int ret;
> > +
> > + if (!hash_algo || !len)
> > + return false;
> > +
> > + *len = algo_to_len(hash_algo);
>
> Please modify it this way;
> int hash_len;
>
> hash_len = algo_to_len(hash_algo);
> if (len)
> *len = hash_len;
>
> Then use hash_len instead of *len hereafterr.
>
> This way, we don't have to define a unused local variable below code.
Sure
[...]
> > +
> > + /* We just need any sha256 algo to start the matching */
> > + hash_algo = guid_to_sha_str(&efi_guid_sha256);
>
> Why not check if hash_algo is NULL?
>
> > + len = guid_to_sha_len(&efi_guid_sha256);
>
> => len = algo_to_len(hash_algo);
>
> Or even we don't need this line as efi_has_regions() will set it for us.
>
Yea good catch. I was playing with a version where len wasn't a ptr in
efi_hash_regions() and that's a leftover (as well as guid_to_sha_len()).
[...]
Thanks for having a look
/Ilias
