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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <c021654a-8d34-b34b-098b-b7622e7b0b39@redhat.com>
Date:   Tue, 19 Nov 2019 12:38:36 +0100
From:   Javier Martinez Canillas <javierm@...hat.com>
To:     Hans de Goede <hdegoede@...hat.com>, linux-kernel@...r.kernel.org
Cc:     Peter Jones <pjones@...hat.com>,
        David Howells <dhowells@...hat.com>,
        James Morris <jmorris@...ei.org>,
        Josh Boyer <jwboyer@...oraproject.org>,
        Mimi Zohar <zohar@...ux.ibm.com>,
        Nayna Jain <nayna@...ux.ibm.com>,
        "Serge E. Hallyn" <serge@...lyn.com>,
        linux-security-module@...r.kernel.org
Subject: Re: [PATCH] efi: Only print errors about failing to get certs if EFI
 vars are found

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@...hat.com>
>> Signed-off-by: Javier Martinez Canillas <javierm@...hat.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