[PATCH 2/2] MODSIGN: load hash blacklist of modules from MOKx

[joeyli]

於 三，2013-12-11 於 11:15 -0500，Josh Boyer 提到：
> On Wed, Dec 11, 2013 at 03:26:17PM +0800, Lee, Chun-Yi wrote:
> > This patch add the support to load blacklisted hash or public key from
...
> > +static int __init signature_blacklist_func(efi_guid_t efi_cert_guid,
> > +		const efi_signature_data_t *elem)
> > +{
> > +	struct module_hash *module_hash = NULL;
> > +	int i;
> > +
> > +	for (i = 0; efi_hash_types[i].hash < PKEY_HASH__LAST; i++) {
> > +		struct efi_hash_type type = efi_hash_types[i];
> > +
> > +		if (efi_guidcmp(efi_cert_guid, type.efi_cert_guid) != 0)
> > +			continue;
> > +
> > +		module_hash = kzalloc(sizeof(*module_hash) + type.size, GFP_KERNEL);
> > +		module_hash->hash = type.hash;
> > +		module_hash->hash_name = type.hash_name;
> > +		module_hash->size = type.size;
> > +		memcpy(module_hash->hash_data, elem->signature_data, type.size);
> > +	}
> > +
> > +	if (!module_hash) {
> > +		pr_err("Problem loading hash of blacklisted module: %pUb\n",
> > +			&efi_cert_guid);
> > +		return -ENOTSUPP;
> 
> Shouldn't this check be moved directly under the kzalloc call?  Or I
> suppose a similar check should be added there.
> 

Next version patch will hook the hash algorithm of blacklist the same
with kernel module signature. I will simplify this checking code.

> > +		data += sizeof(list) + hsize;
> > +		size -= sizeof(list) + hsize;
> > +		offs += sizeof(list) + hsize;
> > +
> > +		for (; elsize > 0; elsize -= esize) {
> > +			elem = data;
> > +
> > +			pr_debug("ELEM[%04x]\n", offs);
> > +			func(efi_cert_guid, elem);
> 
> The func call here can fail but there is no return code checking.
> 

Thank's for your point out, I will add the return code checking here.

> > +			data += esize;
> > +			size -= esize;
> > +			offs += esize;
> > +		}
> > +	}
> > +
> > +	return 0;
> > +}
> > +
> > +static int __init parse_efi_signature_list_hashs(const void *data, size_t size,
> > +		int (*func)(efi_guid_t, const efi_signature_data_t *))
> 
> Why do we pass around a function pointer here when there is only one
> place we call this and it always passes the same function?
> 

Because the code of parse_efi_signature_list_hash() is almost the same
with parse_efi_signature_list(), so I want pass different functions to
handle the different logic of them.

Sorry for I forgot merge this change to parse_efi_signature_list().
Thanks for your remind, I will do it in next version.

> >  /*
> >   *  * Load the certs contained in the UEFI databases
> >   *   */
> > @@ -62,8 +194,8 @@ static int __init load_uefi_certs(void)
> >  {
> >  	efi_guid_t secure_var = EFI_IMAGE_SECURITY_DATABASE_GUID;
> >  	efi_guid_t mok_var = EFI_SHIM_LOCK_GUID;
> > -	void *db = NULL, *dbx = NULL, *mok = NULL;
> > -	unsigned long dbsize = 0, dbxsize = 0, moksize = 0;
> > +	void *db = NULL, *dbx = NULL, *mok = NULL, *mokx = NULL;
> > +	unsigned long dbsize = 0, dbxsize = 0, moksize = 0, mokxsize = 0;
> >  	int ignore_db, rc = 0;
> >  
> >  	/* Check if SB is enabled and just return if not */
> > @@ -109,6 +241,20 @@ static int __init load_uefi_certs(void)
> >  		kfree(dbx);
> >  	}
> >  
> > +	mokx = get_cert_list(L"MokListXRT", &mok_var, &mokxsize);
> > +	if (!mokx) {
> > +		pr_info("MODSIGN: Couldn't get UEFI MokListXRT\n");
> 
> Is there a reason we don't call the hash parsing on dbx itself?
> 

Yes, we can, I will add the hash parsing on dbx.

> > +	} else {
> > +		rc = parse_efi_signature_list(mokx, mokxsize,
> > +			system_blacklist_keyring);
> > +		if (rc)
> > +			pr_err("Couldn't parse MokListXRT signatures: %d\n", rc);
> > +		else
> > +			parse_efi_signature_list_hashs(mokx, mokxsize,
> > +					signature_blacklist_func);
> > +		kfree(mokx);
> 
> Do we want to only check for hashes if signature parsing fails?
> MokListXRT could contain both hashes and certs, couldn't it?
> 

Yes, the signature parsing fail should not effect to the hash parsing. I
will fix it in next version.


Thanks a lot!
Joey Lee