於 三,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