Re: [PATCH 2/2 v2] efi: print appropriate status message when loading certificates
On Wed, Mar 27, 2019 at 03:23:55PM -0400, Mimi Zohar wrote: > On Sun, 2019-03-24 at 08:26 +0800, Lee, Chun-Yi wrote: > > When loading certificates list from UEFI variable, the original error > > message direct shows the efi status code from UEFI firmware. It looks > > ugly: > > > > [2.335031] Couldn't get size: 0x800e > > [2.335032] Couldn't get UEFI MokListRT > > [2.339985] Couldn't get size: 0x800e > > [2.339987] Couldn't get UEFI dbx list > > > > So, this patch shows the status string instead of status code. > > > > On the other hand, the "Couldn't get UEFI" message doesn't need > > to be exposed when db/dbx/mok variable do not exist. So, this > > patch set the message level to debug. > > > > v2. > > Setting the MODSIGN messagse level to debug. > > > > Link: > > https://forums.opensuse.org/showthread.php/535324-MODSIGN-Couldn-t-get-UEFI-db-list?p=2897516#post2897516 > > Cc: James Morris > > Cc: Serge E. Hallyn" > > Cc: David Howells > > Cc: Nayna Jain > > Cc: Josh Boyer > > Cc: Mimi Zohar > > Signed-off-by: "Lee, Chun-Yi" > > --- > > security/integrity/platform_certs/load_uefi.c | 13 - > > 1 file changed, 8 insertions(+), 5 deletions(-) > > > > diff --git a/security/integrity/platform_certs/load_uefi.c > > b/security/integrity/platform_certs/load_uefi.c > > index 81b19c52832b..e65244b31f04 100644 > > --- a/security/integrity/platform_certs/load_uefi.c > > +++ b/security/integrity/platform_certs/load_uefi.c > > @@ -48,7 +48,9 @@ static __init void *get_cert_list(efi_char16_t *name, > > efi_guid_t *guid, > > > > 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); > > + if (status != EFI_NOT_FOUND) > > + pr_err("Couldn't get size: %s\n", > > + efi_status_to_str(status)); > > return NULL; > > } > > > > @@ -59,7 +61,8 @@ static __init void *get_cert_list(efi_char16_t *name, > > efi_guid_t *guid, > > 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: %s\n", > > + efi_status_to_str(status)); > > return NULL; > > } > > > > @@ -155,7 +158,7 @@ static int __init load_uefi_certs(void) > > if (!uefi_check_ignore_db()) { > > db = get_cert_list(L"db", &secure_var, &dbsize); > > if (!db) { > > - pr_err("MODSIGN: Couldn't get UEFI db list\n"); > > + pr_debug("MODSIGN: Couldn't get UEFI db list\n"); > > Sure, this is fine. > > > } else { > > rc = parse_efi_signature_list("UEFI:db", > > db, dbsize, get_handler_for_db); > > @@ -168,7 +171,7 @@ static int __init load_uefi_certs(void) > > > > mok = get_cert_list(L"MokListRT", &mok_var, &moksize); > > if (!mok) { > > - pr_info("Couldn't get UEFI MokListRT\n"); > > + pr_debug("Couldn't get UEFI MokListRT\n"); > > This is fine too. > > > } else { > > rc = parse_efi_signature_list("UEFI:MokListRT", > > mok, moksize, get_handler_for_db); > > @@ -179,7 +182,7 @@ static int __init load_uefi_certs(void) > > > > dbx = get_cert_list(L"dbx", &secure_var, &dbxsize); > > if (!dbx) { > > - pr_info("Couldn't get UEFI dbx list\n"); > > + pr_debug("Couldn't get UEFI dbx list\n"); > > If there isn't a db or moklist, then this is fine. My concern is not > having an indication that the dbx wasn't installed, when it should > have been. > > Perhaps similar to the "Loading compiled-in X.509 certificates" > informational message there should informational messages for db, mok, > and dbx as well. > OK. I will add message when kernel found db, dbx and mok. It will just like the informaton message for ACPI S0,S3,S4 support. Thanks Joey Lee
Re: [PATCH 2/2] efi: print appropriate status message when loading certificates
Hi Ard, On Fri, Mar 22, 2019 at 03:27:46PM +0100, Ard Biesheuvel wrote: > On Fri, 22 Mar 2019 at 11:34, Lee, Chun-Yi wrote: > > > > When loading certificates list from UEFI variable, the original error > > message direct shows the efi status code from UEFI firmware. It looks > > ugly: > > > > [2.335031] Couldn't get size: 0x800e > > [2.335032] Couldn't get UEFI MokListRT > > [2.339985] Couldn't get size: 0x800e > > [2.339987] Couldn't get UEFI dbx list > > > > Why is it an error in the first place if these EFI variables do not exist? > As you said, the error message should only be exposed when these EFI variables exist. I will change it in next version of my patch. Thanks a lot! Joey Lee > > > So, this patch shows the status string instead of status code. > > > > On the other hand, the error message of EFI_NOT_FOUND > > (0x800e) doesn't need to be exposed because kernel > > already prints "Couldn't get UEFI..." message. This patch also > > filtered out it. > > > > Link: > > https://forums.opensuse.org/showthread.php/535324-MODSIGN-Couldn-t-get-UEFI-db-list?p=2897516#post2897516 > > Cc: James Morris > > Cc: Serge E. Hallyn" > > Cc: David Howells > > Cc: Nayna Jain > > Cc: Josh Boyer > > Cc: Mimi Zohar > > Signed-off-by: "Lee, Chun-Yi" > > --- > > security/integrity/platform_certs/load_uefi.c | 7 +-- > > 1 file changed, 5 insertions(+), 2 deletions(-) > > > > diff --git a/security/integrity/platform_certs/load_uefi.c > > b/security/integrity/platform_certs/load_uefi.c > > index 81b19c52832b..fe261166621f 100644 > > --- a/security/integrity/platform_certs/load_uefi.c > > +++ b/security/integrity/platform_certs/load_uefi.c > > @@ -48,7 +48,9 @@ static __init void *get_cert_list(efi_char16_t *name, > > efi_guid_t *guid, > > > > 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); > > + if (status != EFI_NOT_FOUND) > > + pr_err("Couldn't get size: %s\n", > > + efi_status_to_str(status)); > > return NULL; > > } > > > > @@ -59,7 +61,8 @@ static __init void *get_cert_list(efi_char16_t *name, > > efi_guid_t *guid, > > 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: %s\n", > > + efi_status_to_str(status)); > > return NULL; > > } > > > > -- > > 2.16.4 > >
Re: An actual suggestion (Re: [GIT PULL] Kernel lockdown for secure boot)
Hi Mimi, On Thu, Apr 05, 2018 at 10:01:09AM -0400, Mimi Zohar wrote: > On Thu, 2018-04-05 at 10:16 +0800, joeyli wrote: > > Hi David, > > > > On Wed, Apr 04, 2018 at 05:17:24PM +0100, David Howells wrote: > > > Andy Lutomirski wrote: > > > > > > > Since this thread has devolved horribly, I'm going to propose a > > > > solution. > > > > > > > > 1. Split the "lockdown" state into three levels: (please don't > > > > bikeshed about the names right now.) > > > > > > > > LOCKDOWN_NONE: normal behavior > > > > > > > > LOCKDOWN_PROTECT_INTEGREITY: kernel tries to keep root from writing to > > > > kernel memory > > > > > > > > LOCKDOWN_PROTECT_INTEGRITY_AND_SECRECY: kernel tries to keep root from > > > > reading or writing kernel memory. > > > > > > In theory, it's good idea, but in practice it's not as easy to implement > > > as I > > > think you think. > > > > > > Let me list here the things that currently get restricted by lockdown: > > > > > [...snip] > > > (5) Kexec. > > > > > > > About IMA with kernel module signing and kexec(not on x86_64 yet)... > > Only carrying the measurement list across kexec is architecture > specific, but everything else should work. > > > Because IMA can be used to verify the integrity of kernel module or even > > the image for kexec. I think that the > > IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY must be enabled at > > runtime > > when kernel is locked-down. > > I think we need to understand the problem a bit better. Is the > problem that you're using the secondary keyring and loading the UEFI > keys onto the secondary keyring? > Sorry for my mistake. I want to write INTEGRITY_TRUSTED_KEYRING in above but not IMA_KEYRINGS_PERMIT_SIGNED_BY_BUILTIN_OR_SECONDARY. My brain is not clear when writing the mail. > > Because the root can enroll master key to keyring then IMA trusts the ima > > key > > derived from master key. It causes that the arbitrary signed module can be > > loaded > > when the root compromised. > > With only the builtin keyring, only keys signed by a builtin key can > be added to the IMA keyring. > Thanks for your description. I saw that the IMA_LOAD_X509 already depends on IMA_TRUSTED_KEYRING (INTEGRITY_TRUSTED_KEYRING). Please ignore my concern. Thanks a lot! Joey Lee -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 18/27] bpf: Restrict kernel image access functions when the kernel is locked down
On Fri, Oct 20, 2017 at 05:03:22PM +0100, David Howells wrote: > j...@suse.com wrote: > > > I think that we don't need to lock down sys_bpf() now because > > we didn't lock down other interfaces for reading arbitrary > > address like /dev/mem and /dev/kmem. > > Ummm... See patch 4. You even gave me a Reviewed-by for it ;-) > > David hm... patch 4 only prevents write_mem() but not read_mem(). Or I missed anything? Thanks Joey Lee -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 18/27] bpf: Restrict kernel image access functions when the kernel is locked down
On Fri, Oct 20, 2017 at 09:08:48AM +0100, David Howells wrote: > Hi Joey, > > Should I just lock down sys_bpf() entirely for now? We can always free it up > somewhat later. > > David OK~~ Please just remove my patch until we find out a way to verify bpf code or protect sensitive data in memory. I think that we don't need to lock down sys_bpf() now because we didn't lock down other interfaces for reading arbitrary address like /dev/mem and /dev/kmem. Thanks a lot! Joey Lee -- To unsubscribe from this list: send the line "unsubscribe linux-efi" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html