Re: [PATCH 1/2] efi_loader: add sha384/512 on certificate revocation

[Ilias Apalodimas <ilias.apalodimas@linaro.org>]

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