於 三，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
_______________________________________________
kernel mailing list
kernel@xxxxxxxxxxxxxxxxxxxxxxx
https://admin.fedoraproject.org/mailman/listinfo/kernel
