| Message ID | 20191119115043.21585-1-javierm@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] efi: Only print errors about failing to get certs if EFI vars are found | expand |
Hi, On 19-11-2019 12:50, 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> > > --- > Hans, > > I'll really appreciate if you can test this patch. I just built tested it > because I don't have access to a machine to reproduce the issue right now. Ok, I've given this a test-run just now, works as advertised for me: Tested-by: Hans de Goede <hdegoede@redhat.com> Regards, Hans > Changes in v2: > - Fix flaws in the logic, that caused the signature list was parsed if > the return code was EFI_NOT_FOUND that pointed out Hans de Goede. > - Print debug messages if the variables are not found. > > security/integrity/platform_certs/load_uefi.c | 40 ++++++++++++------- > 1 file changed, 26 insertions(+), 14 deletions(-) > > diff --git a/security/integrity/platform_certs/load_uefi.c b/security/integrity/platform_certs/load_uefi.c > index 81b19c52832..020fc7a11ef 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,9 +156,12 @@ 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); > + db = get_cert_list(L"db", &secure_var, &dbsize, &status); > if (!db) { > - pr_err("MODSIGN: Couldn't get UEFI db list\n"); > + if (status == EFI_NOT_FOUND) > + pr_debug("MODSIGN: db variable wasn't found\n"); > + else > + pr_err("MODSIGN: Couldn't get UEFI db list\n"); > } else { > rc = parse_efi_signature_list("UEFI:db", > db, dbsize, get_handler_for_db); > @@ -166,9 +172,12 @@ static int __init load_uefi_certs(void) > } > } > > - mok = get_cert_list(L"MokListRT", &mok_var, &moksize); > + mok = get_cert_list(L"MokListRT", &mok_var, &moksize, &status); > if (!mok) { > - pr_info("Couldn't get UEFI MokListRT\n"); > + if (status == EFI_NOT_FOUND) > + pr_debug("MokListRT variable wasn't found\n"); > + else > + pr_info("Couldn't get UEFI MokListRT\n"); > } else { > rc = parse_efi_signature_list("UEFI:MokListRT", > mok, moksize, get_handler_for_db); > @@ -177,9 +186,12 @@ static int __init load_uefi_certs(void) > kfree(mok); > } > > - dbx = get_cert_list(L"dbx", &secure_var, &dbxsize); > + dbx = get_cert_list(L"dbx", &secure_var, &dbxsize, &status); > if (!dbx) { > - pr_info("Couldn't get UEFI dbx list\n"); > + if (status == EFI_NOT_FOUND) > + pr_debug("dbx variable wasn't found\n"); > + else > + pr_info("Couldn't get UEFI dbx list\n"); > } else { > rc = parse_efi_signature_list("UEFI:dbx", > dbx, dbxsize, >
On 11/19/19 4:40 PM, Hans de Goede wrote: > Hi, > > On 19-11-2019 12:50, 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> >> >> --- >> Hans, >> >> I'll really appreciate if you can test this patch. I just built tested it >> because I don't have access to a machine to reproduce the issue right now. > > Ok, I've given this a test-run just now, works as advertised for me: > > Tested-by: Hans de Goede <hdegoede@redhat.com> > Thanks a lot for testing Hans. James and Mimi, Anything else that's needed for this patch to be picked? > Regards, > > Hans > Best regards,
diff --git a/security/integrity/platform_certs/load_uefi.c b/security/integrity/platform_certs/load_uefi.c index 81b19c52832..020fc7a11ef 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,9 +156,12 @@ 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); + db = get_cert_list(L"db", &secure_var, &dbsize, &status); if (!db) { - pr_err("MODSIGN: Couldn't get UEFI db list\n"); + if (status == EFI_NOT_FOUND) + pr_debug("MODSIGN: db variable wasn't found\n"); + else + pr_err("MODSIGN: Couldn't get UEFI db list\n"); } else { rc = parse_efi_signature_list("UEFI:db", db, dbsize, get_handler_for_db); @@ -166,9 +172,12 @@ static int __init load_uefi_certs(void) } } - mok = get_cert_list(L"MokListRT", &mok_var, &moksize); + mok = get_cert_list(L"MokListRT", &mok_var, &moksize, &status); if (!mok) { - pr_info("Couldn't get UEFI MokListRT\n"); + if (status == EFI_NOT_FOUND) + pr_debug("MokListRT variable wasn't found\n"); + else + pr_info("Couldn't get UEFI MokListRT\n"); } else { rc = parse_efi_signature_list("UEFI:MokListRT", mok, moksize, get_handler_for_db); @@ -177,9 +186,12 @@ static int __init load_uefi_certs(void) kfree(mok); } - dbx = get_cert_list(L"dbx", &secure_var, &dbxsize); + dbx = get_cert_list(L"dbx", &secure_var, &dbxsize, &status); if (!dbx) { - pr_info("Couldn't get UEFI dbx list\n"); + if (status == EFI_NOT_FOUND) + pr_debug("dbx variable wasn't found\n"); + else + pr_info("Couldn't get UEFI dbx list\n"); } else { rc = parse_efi_signature_list("UEFI:dbx", dbx, dbxsize,
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> --- Hans, I'll really appreciate if you can test this patch. I just built tested it because I don't have access to a machine to reproduce the issue right now. Best regards, Javier Changes in v2: - Fix flaws in the logic, that caused the signature list was parsed if the return code was EFI_NOT_FOUND that pointed out Hans de Goede. - Print debug messages if the variables are not found. security/integrity/platform_certs/load_uefi.c | 40 ++++++++++++------- 1 file changed, 26 insertions(+), 14 deletions(-)