* [PATCH] efi: Only print errors about failing to get certs if EFI vars are found
@ 2019-11-19 9:18 Javier Martinez Canillas
2019-11-19 10:59 ` Hans de Goede
0 siblings, 1 reply; 3+ messages in thread
From: Javier Martinez Canillas @ 2019-11-19 9:18 UTC (permalink / raw)
To: linux-kernel
Cc: Peter Jones, Hans de Goede, Javier Martinez Canillas,
David Howells, James Morris, Josh Boyer, Mimi Zohar, Nayna Jain,
Serge E. Hallyn, linux-security-module
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>
---
security/integrity/platform_certs/load_uefi.c | 31 ++++++++++---------
1 file changed, 17 insertions(+), 14 deletions(-)
diff --git a/security/integrity/platform_certs/load_uefi.c b/security/integrity/platform_certs/load_uefi.c
index 81b19c52832..336fa528359 100644
--- a/security/integrity/platform_certs/load_uefi.c
+++ b/security/integrity/platform_certs/load_uefi.c
@@ -39,16 +39,18 @@ static __init bool uefi_check_ignore_db(void)
* Get a certificate list blob from the named EFI variable.
*/
static __init void *get_cert_list(efi_char16_t *name, efi_guid_t *guid,
- unsigned long *size)
+ unsigned long *size, efi_status_t *status)
{
- efi_status_t status;
unsigned long lsize = 4;
unsigned long tmpdb[4];
void *db;
- status = efi.get_variable(name, guid, NULL, &lsize, &tmpdb);
- if (status != EFI_BUFFER_TOO_SMALL) {
- pr_err("Couldn't get size: 0x%lx\n", status);
+ *status = efi.get_variable(name, guid, NULL, &lsize, &tmpdb);
+ if (*status == EFI_NOT_FOUND)
+ return NULL;
+
+ if (*status != EFI_BUFFER_TOO_SMALL) {
+ pr_err("Couldn't get size: 0x%lx\n", *status);
return NULL;
}
@@ -56,10 +58,10 @@ static __init void *get_cert_list(efi_char16_t *name, efi_guid_t *guid,
if (!db)
return NULL;
- status = efi.get_variable(name, guid, NULL, &lsize, db);
- if (status != EFI_SUCCESS) {
+ *status = efi.get_variable(name, guid, NULL, &lsize, db);
+ if (*status != EFI_SUCCESS) {
kfree(db);
- pr_err("Error reading db var: 0x%lx\n", status);
+ pr_err("Error reading db var: 0x%lx\n", *status);
return NULL;
}
@@ -144,6 +146,7 @@ static int __init load_uefi_certs(void)
efi_guid_t mok_var = EFI_SHIM_LOCK_GUID;
void *db = NULL, *dbx = NULL, *mok = NULL;
unsigned long dbsize = 0, dbxsize = 0, moksize = 0;
+ efi_status_t status;
int rc = 0;
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",
@@ -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",
@@ -177,8 +180,8 @@ static int __init load_uefi_certs(void)
kfree(mok);
}
- dbx = get_cert_list(L"dbx", &secure_var, &dbxsize);
- if (!dbx) {
+ dbx = get_cert_list(L"dbx", &secure_var, &dbxsize, &status);
+ if (!dbx && status != EFI_NOT_FOUND) {
pr_info("Couldn't get UEFI dbx list\n");
} else {
rc = parse_efi_signature_list("UEFI:dbx",
--
2.23.0
^ permalink raw reply related [flat|nested] 3+ messages in thread
* Re: [PATCH] efi: Only print errors about failing to get certs if EFI vars are found
2019-11-19 9:18 [PATCH] efi: Only print errors about failing to get certs if EFI vars are found Javier Martinez Canillas
@ 2019-11-19 10:59 ` Hans de Goede
2019-11-19 11:38 ` Javier Martinez Canillas
0 siblings, 1 reply; 3+ messages in thread
From: Hans de Goede @ 2019-11-19 10:59 UTC (permalink / raw)
To: Javier Martinez Canillas, linux-kernel
Cc: Peter Jones, David Howells, James Morris, Josh Boyer, Mimi Zohar,
Nayna Jain, Serge E. Hallyn, linux-security-module
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:
>
> ---
>
> security/integrity/platform_certs/load_uefi.c | 31 ++++++++++---------
> 1 file changed, 17 insertions(+), 14 deletions(-)
>
> diff --git a/security/integrity/platform_certs/load_uefi.c b/security/integrity/platform_certs/load_uefi.c
> index 81b19c52832..336fa528359 100644
> --- a/security/integrity/platform_certs/load_uefi.c
> +++ b/security/integrity/platform_certs/load_uefi.c
> @@ -39,16 +39,18 @@ static __init bool uefi_check_ignore_db(void)
> * Get a certificate list blob from the named EFI variable.
> */
> static __init void *get_cert_list(efi_char16_t *name, efi_guid_t *guid,
> - unsigned long *size)
> + unsigned long *size, efi_status_t *status)
> {
> - efi_status_t status;
> unsigned long lsize = 4;
> unsigned long tmpdb[4];
> void *db;
>
> - status = efi.get_variable(name, guid, NULL, &lsize, &tmpdb);
> - if (status != EFI_BUFFER_TOO_SMALL) {
> - pr_err("Couldn't get size: 0x%lx\n", status);
> + *status = efi.get_variable(name, guid, NULL, &lsize, &tmpdb);
> + if (*status == EFI_NOT_FOUND)
> + return NULL;
> +
> + if (*status != EFI_BUFFER_TOO_SMALL) {
> + pr_err("Couldn't get size: 0x%lx\n", *status);
> return NULL;
> }
>
> @@ -56,10 +58,10 @@ static __init void *get_cert_list(efi_char16_t *name, efi_guid_t *guid,
> if (!db)
> return NULL;
>
> - status = efi.get_variable(name, guid, NULL, &lsize, db);
> - if (status != EFI_SUCCESS) {
> + *status = efi.get_variable(name, guid, NULL, &lsize, db);
> + if (*status != EFI_SUCCESS) {
> kfree(db);
> - pr_err("Error reading db var: 0x%lx\n", status);
> + pr_err("Error reading db var: 0x%lx\n", *status);
> return NULL;
> }
>
> @@ -144,6 +146,7 @@ static int __init load_uefi_certs(void)
> efi_guid_t mok_var = EFI_SHIM_LOCK_GUID;
> void *db = NULL, *dbx = NULL, *mok = NULL;
> unsigned long dbsize = 0, dbxsize = 0, moksize = 0;
> + efi_status_t status;
> int rc = 0;
>
> 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",
> @@ -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",
> @@ -177,8 +180,8 @@ static int __init load_uefi_certs(void)
> kfree(mok);
> }
>
> - dbx = get_cert_list(L"dbx", &secure_var, &dbxsize);
> - if (!dbx) {
> + dbx = get_cert_list(L"dbx", &secure_var, &dbxsize, &status);
> + if (!dbx && status != EFI_NOT_FOUND) {
> pr_info("Couldn't get UEFI dbx list\n");
> } else {
> rc = parse_efi_signature_list("UEFI:dbx",
Idem.
Regards,
Hans
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] efi: Only print errors about failing to get certs if EFI vars are found
2019-11-19 10:59 ` Hans de Goede
@ 2019-11-19 11:38 ` Javier Martinez Canillas
0 siblings, 0 replies; 3+ messages in thread
From: Javier Martinez Canillas @ 2019-11-19 11:38 UTC (permalink / raw)
To: Hans de Goede, linux-kernel
Cc: Peter Jones, David Howells, James Morris, Josh Boyer, Mimi Zohar,
Nayna Jain, Serge E. Hallyn, linux-security-module
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
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2019-11-19 11:38 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-11-19 9:18 [PATCH] efi: Only print errors about failing to get certs if EFI vars are found Javier Martinez Canillas
2019-11-19 10:59 ` Hans de Goede
2019-11-19 11:38 ` Javier Martinez Canillas
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).