Re: [PATCH] efi: Only print errors about failing to get certs if EFI vars are found

[Javier Martinez Canillas <javierm@redhat.com>]

Hello Hans,

Thanks a lot for your feedback.

On 11/19/19 11:59 AM, Hans de Goede wrote:
> Hi,
> 
> On 19-11-2019 10:18, Javier Martinez Canillas wrote:
>> If CONFIG_LOAD_UEFI_KEYS is enabled, the kernel attempts to load the certs
>> from the db, dbx and MokListRT EFI variables into the appropriate keyrings.
>>
>> But it just assumes that the variables will be present and prints an error
>> if the certs can't be loaded, even when is possible that the variables may
>> not exist. For example the MokListRT variable will only be present if shim
>> is used.
>>
>> So only print an error message about failing to get the certs list from an
>> EFI variable if this is found. Otherwise these printed errors just pollute
>> the kernel ring buffer with confusing messages like the following:
>>
>> [    5.427251] Couldn't get size: 0x800000000000000e
>> [    5.427261] MODSIGN: Couldn't get UEFI db list
>> [    5.428012] Couldn't get size: 0x800000000000000e
>> [    5.428023] Couldn't get UEFI MokListRT
>>
>> Reported-by: Hans de Goede <hdegoede@redhat.com>
>> Signed-off-by: Javier Martinez Canillas <javierm@redhat.com>
> 
> Thanks for this, I just noticed a potential issue which I missed
> when you send this to me for testing:
> 

[snip]

>>   
>>   	if (!efi.get_variable)
>> @@ -153,8 +156,8 @@ static int __init load_uefi_certs(void)
>>   	 * an error if we can't get them.
>>   	 */
>>   	if (!uefi_check_ignore_db()) {
>> -		db = get_cert_list(L"db", &secure_var, &dbsize);
>> -		if (!db) {
>> +		db = get_cert_list(L"db", &secure_var, &dbsize, &status);
>> +		if (!db && status != EFI_NOT_FOUND) {
>>   			pr_err("MODSIGN: Couldn't get UEFI db list\n");
>>   		} else {
>>   			rc = parse_efi_signature_list("UEFI:db",

You are correct, this is another instance of the same issue that you mentioned.

>> @@ -166,8 +169,8 @@ static int __init load_uefi_certs(void)
>>   		}
>>   	}
>>   
>> -	mok = get_cert_list(L"MokListRT", &mok_var, &moksize);
>> -	if (!mok) {
>> +	mok = get_cert_list(L"MokListRT", &mok_var, &moksize, &status);
>> +	if (!mok && status != EFI_NOT_FOUND) {
>>   		pr_info("Couldn't get UEFI MokListRT\n");
>>   	} else {
>>   		rc = parse_efi_signature_list("UEFI:MokListRT",
> 
> This means that if status == EFI_NOT_FOUND we end up still
> trying to parse the signature list, I guess that moksize == 0
> or some such is saving us here, but I believe that
> this should really be:
> 
> 	if (!mok) {
> 		if (status != EFI_NOT_FOUND)
> 			pr_info("Couldn't get UEFI MokListRT\n");
> 	} else {
> 		rc = parse_efi_signature_list("UEFI:MokListRT",
> 

Agreed. I'll fix the issues and post a v2. Since we are adding these statements,
I could also print debug messages for the case that status == EFI_NOT_FOUND.

Best regards,
-- 
Javier Martinez Canillas
Software Engineer - Desktop Hardware Enablement
Red Hat