| Message ID | 20201014104032.9776-2-clin@suse.com |
|---|---|
| State | New |
| Headers | show |
| Series | add ima_arch support for ARM64 | expand |
Hi Ard and Mimi, On Wed, Oct 14, 2020 at 06:40:31PM +0800, Chester Lin wrote: > Separate the get_sb_mode() from arch/x86 and treat it as a common function > [rename to efi_get_secureboot_mode] so all EFI-based architectures can > reuse the same logic. > > Signed-off-by: Chester Lin <clin@suse.com> > --- > arch/x86/kernel/ima_arch.c | 47 ++------------------------------------ > drivers/firmware/efi/efi.c | 43 ++++++++++++++++++++++++++++++++++ > include/linux/efi.h | 5 ++++ > 3 files changed, 50 insertions(+), 45 deletions(-) > > diff --git a/arch/x86/kernel/ima_arch.c b/arch/x86/kernel/ima_arch.c > index 7dfb1e808928..ed4623ecda6e 100644 > --- a/arch/x86/kernel/ima_arch.c > +++ b/arch/x86/kernel/ima_arch.c > @@ -8,49 +8,6 @@ > > extern struct boot_params boot_params; > > -static enum efi_secureboot_mode get_sb_mode(void) > -{ > - efi_guid_t efi_variable_guid = EFI_GLOBAL_VARIABLE_GUID; > - efi_status_t status; > - unsigned long size; > - u8 secboot, setupmode; > - > - size = sizeof(secboot); > - > - if (!efi_rt_services_supported(EFI_RT_SUPPORTED_GET_VARIABLE)) { > - pr_info("ima: secureboot mode unknown, no efi\n"); > - return efi_secureboot_mode_unknown; > - } > - > - /* Get variable contents into buffer */ > - status = efi.get_variable(L"SecureBoot", &efi_variable_guid, > - NULL, &size, &secboot); > - if (status == EFI_NOT_FOUND) { > - pr_info("ima: secureboot mode disabled\n"); > - return efi_secureboot_mode_disabled; > - } > - > - if (status != EFI_SUCCESS) { > - pr_info("ima: secureboot mode unknown\n"); > - return efi_secureboot_mode_unknown; > - } > - > - size = sizeof(setupmode); > - status = efi.get_variable(L"SetupMode", &efi_variable_guid, > - NULL, &size, &setupmode); > - > - if (status != EFI_SUCCESS) /* ignore unknown SetupMode */ > - setupmode = 0; > - > - if (secboot == 0 || setupmode == 1) { > - pr_info("ima: secureboot mode disabled\n"); > - return efi_secureboot_mode_disabled; > - } > - > - pr_info("ima: secureboot mode enabled\n"); > - return efi_secureboot_mode_enabled; > -} > - > bool arch_ima_get_secureboot(void) > { > static enum efi_secureboot_mode sb_mode; > @@ -60,7 +17,7 @@ bool arch_ima_get_secureboot(void) > sb_mode = boot_params.secure_boot; > > if (sb_mode == efi_secureboot_mode_unset) > - sb_mode = get_sb_mode(); > + sb_mode = efi_get_secureboot_mode(); > initialized = true; > } > > @@ -70,7 +27,7 @@ bool arch_ima_get_secureboot(void) > return false; > } > > -/* secureboot arch rules */ > +/* secure and trusted boot arch rules */ > static const char * const sb_arch_rules[] = { > #if !IS_ENABLED(CONFIG_KEXEC_SIG) > "appraise func=KEXEC_KERNEL_CHECK appraise_type=imasig", > diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c > index 5e5480a0a32d..68ffa6a069c0 100644 > --- a/drivers/firmware/efi/efi.c > +++ b/drivers/firmware/efi/efi.c I hope you don't mind that I temporarily move the get_sb_mode() to efi since I'm not sure which place is better. Please let me know if any suggestions. Thanks, Chester > @@ -1022,3 +1022,46 @@ static int __init register_update_efi_random_seed(void) > } > late_initcall(register_update_efi_random_seed); > #endif > + > +enum efi_secureboot_mode efi_get_secureboot_mode(void) > +{ > + efi_guid_t efi_variable_guid = EFI_GLOBAL_VARIABLE_GUID; > + efi_status_t status; > + unsigned long size; > + u8 secboot, setupmode; > + > + size = sizeof(secboot); > + > + if (!efi_rt_services_supported(EFI_RT_SUPPORTED_GET_VARIABLE)) { > + pr_info("ima: secureboot mode unknown, no efi\n"); > + return efi_secureboot_mode_unknown; > + } > + > + /* Get variable contents into buffer */ > + status = efi.get_variable(L"SecureBoot", &efi_variable_guid, > + NULL, &size, &secboot); > + if (status == EFI_NOT_FOUND) { > + pr_info("ima: secureboot mode disabled\n"); > + return efi_secureboot_mode_disabled; > + } > + > + if (status != EFI_SUCCESS) { > + pr_info("ima: secureboot mode unknown\n"); > + return efi_secureboot_mode_unknown; > + } > + > + size = sizeof(setupmode); > + status = efi.get_variable(L"SetupMode", &efi_variable_guid, > + NULL, &size, &setupmode); > + > + if (status != EFI_SUCCESS) /* ignore unknown SetupMode */ > + setupmode = 0; > + > + if (secboot == 0 || setupmode == 1) { > + pr_info("ima: secureboot mode disabled\n"); > + return efi_secureboot_mode_disabled; > + } > + > + pr_info("ima: secureboot mode enabled\n"); > + return efi_secureboot_mode_enabled; > +} > diff --git a/include/linux/efi.h b/include/linux/efi.h > index d7c0e73af2b9..a73e5ae04672 100644 > --- a/include/linux/efi.h > +++ b/include/linux/efi.h > @@ -1076,8 +1076,13 @@ static inline int efi_runtime_map_copy(void *buf, size_t bufsz) > > #ifdef CONFIG_EFI > extern bool efi_runtime_disabled(void); > +extern enum efi_secureboot_mode efi_get_secureboot_mode(void); > #else > static inline bool efi_runtime_disabled(void) { return true; } > +static inline enum efi_secureboot_mode efi_get_secureboot_mode(void) > +{ > + return efi_secureboot_mode_disabled; > +} > #endif > > extern void efi_call_virt_check_flags(unsigned long flags, const char *call); > -- > 2.26.1 >
Hello Chester, Thanks for looking into this. Some comments below. On Wed, 14 Oct 2020 at 12:41, Chester Lin <clin@suse.com> wrote: > > Separate the get_sb_mode() from arch/x86 and treat it as a common function > [rename to efi_get_secureboot_mode] so all EFI-based architectures can > reuse the same logic. > > Signed-off-by: Chester Lin <clin@suse.com> > --- > arch/x86/kernel/ima_arch.c | 47 ++------------------------------------ > drivers/firmware/efi/efi.c | 43 ++++++++++++++++++++++++++++++++++ > include/linux/efi.h | 5 ++++ > 3 files changed, 50 insertions(+), 45 deletions(-) > > diff --git a/arch/x86/kernel/ima_arch.c b/arch/x86/kernel/ima_arch.c > index 7dfb1e808928..ed4623ecda6e 100644 > --- a/arch/x86/kernel/ima_arch.c > +++ b/arch/x86/kernel/ima_arch.c > @@ -8,49 +8,6 @@ > > extern struct boot_params boot_params; > > -static enum efi_secureboot_mode get_sb_mode(void) > -{ > - efi_guid_t efi_variable_guid = EFI_GLOBAL_VARIABLE_GUID; > - efi_status_t status; > - unsigned long size; > - u8 secboot, setupmode; > - > - size = sizeof(secboot); > - > - if (!efi_rt_services_supported(EFI_RT_SUPPORTED_GET_VARIABLE)) { > - pr_info("ima: secureboot mode unknown, no efi\n"); > - return efi_secureboot_mode_unknown; > - } > - > - /* Get variable contents into buffer */ > - status = efi.get_variable(L"SecureBoot", &efi_variable_guid, > - NULL, &size, &secboot); > - if (status == EFI_NOT_FOUND) { > - pr_info("ima: secureboot mode disabled\n"); > - return efi_secureboot_mode_disabled; > - } > - > - if (status != EFI_SUCCESS) { > - pr_info("ima: secureboot mode unknown\n"); > - return efi_secureboot_mode_unknown; > - } > - > - size = sizeof(setupmode); > - status = efi.get_variable(L"SetupMode", &efi_variable_guid, > - NULL, &size, &setupmode); > - > - if (status != EFI_SUCCESS) /* ignore unknown SetupMode */ > - setupmode = 0; > - > - if (secboot == 0 || setupmode == 1) { > - pr_info("ima: secureboot mode disabled\n"); > - return efi_secureboot_mode_disabled; > - } > - > - pr_info("ima: secureboot mode enabled\n"); > - return efi_secureboot_mode_enabled; > -} > - > bool arch_ima_get_secureboot(void) > { > static enum efi_secureboot_mode sb_mode; > @@ -60,7 +17,7 @@ bool arch_ima_get_secureboot(void) > sb_mode = boot_params.secure_boot; > > if (sb_mode == efi_secureboot_mode_unset) > - sb_mode = get_sb_mode(); > + sb_mode = efi_get_secureboot_mode(); > initialized = true; > } > > @@ -70,7 +27,7 @@ bool arch_ima_get_secureboot(void) > return false; > } > > -/* secureboot arch rules */ > +/* secure and trusted boot arch rules */ > static const char * const sb_arch_rules[] = { > #if !IS_ENABLED(CONFIG_KEXEC_SIG) > "appraise func=KEXEC_KERNEL_CHECK appraise_type=imasig", > diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c > index 5e5480a0a32d..68ffa6a069c0 100644 > --- a/drivers/firmware/efi/efi.c > +++ b/drivers/firmware/efi/efi.c > @@ -1022,3 +1022,46 @@ static int __init register_update_efi_random_seed(void) > } > late_initcall(register_update_efi_random_seed); > #endif > + > +enum efi_secureboot_mode efi_get_secureboot_mode(void) > +{ > + efi_guid_t efi_variable_guid = EFI_GLOBAL_VARIABLE_GUID; > + efi_status_t status; > + unsigned long size; > + u8 secboot, setupmode; > + > + size = sizeof(secboot); > + > + if (!efi_rt_services_supported(EFI_RT_SUPPORTED_GET_VARIABLE)) { > + pr_info("ima: secureboot mode unknown, no efi\n"); These prints don't belong here anymore. Also, it would be useful if we could reuse this logic in the EFI stub as well, which is built as a separate executable, and does not provide efi.get_variable(). So, you could you please break this up into static inline enum efi_secureboot_mode efi_get_secureboot_mode(efi_get_variable_t *get_var) { } placed into include/linux/efi.h, which encapsulates the core logic, but using get_var(), and without the prints. Then, we could put something like bool efi_ima_get_secureboot(void) { } in drivers/firmware/efi/efi.c (guarded by #ifdef CONFIG_IMA_xxx), which performs the efi_rt_services_supported(EFI_RT_SUPPORTED_GET_VARIABLE)) check, calls efi_get_secureboot_mode(efi.get_variable), and implements the logic. And actually, if the logic is identical between x86 and arm64, I wonder if it wouldn't be better to put the whole thing into drivers/firmware/efi/efi-ima.c or security/integrity/ima/ima-efi.c with the only difference being the boot_params->secure_boot access for x86, which we can factor out to a static inline helper. Mimi, any thoughts here? > + return efi_secureboot_mode_unknown; > + } > + > + /* Get variable contents into buffer */ > + status = efi.get_variable(L"SecureBoot", &efi_variable_guid, > + NULL, &size, &secboot); > + if (status == EFI_NOT_FOUND) { > + pr_info("ima: secureboot mode disabled\n"); > + return efi_secureboot_mode_disabled; > + } > + > + if (status != EFI_SUCCESS) { > + pr_info("ima: secureboot mode unknown\n"); > + return efi_secureboot_mode_unknown; > + } > + > + size = sizeof(setupmode); > + status = efi.get_variable(L"SetupMode", &efi_variable_guid, > + NULL, &size, &setupmode); > + > + if (status != EFI_SUCCESS) /* ignore unknown SetupMode */ > + setupmode = 0; > + > + if (secboot == 0 || setupmode == 1) { > + pr_info("ima: secureboot mode disabled\n"); > + return efi_secureboot_mode_disabled; > + } > + > + pr_info("ima: secureboot mode enabled\n"); > + return efi_secureboot_mode_enabled; > +} > diff --git a/include/linux/efi.h b/include/linux/efi.h > index d7c0e73af2b9..a73e5ae04672 100644 > --- a/include/linux/efi.h > +++ b/include/linux/efi.h > @@ -1076,8 +1076,13 @@ static inline int efi_runtime_map_copy(void *buf, size_t bufsz) > > #ifdef CONFIG_EFI > extern bool efi_runtime_disabled(void); > +extern enum efi_secureboot_mode efi_get_secureboot_mode(void); > #else > static inline bool efi_runtime_disabled(void) { return true; } > +static inline enum efi_secureboot_mode efi_get_secureboot_mode(void) > +{ > + return efi_secureboot_mode_disabled; > +} > #endif > > extern void efi_call_virt_check_flags(unsigned long flags, const char *call); > -- > 2.26.1 >
On Wed, 2020-10-14 at 13:00 +0200, Ard Biesheuvel wrote: > Hello Chester, > > Thanks for looking into this. > > Some comments below. > > On Wed, 14 Oct 2020 at 12:41, Chester Lin <clin@suse.com> wrote: > > > > Separate the get_sb_mode() from arch/x86 and treat it as a common function > > [rename to efi_get_secureboot_mode] so all EFI-based architectures can > > reuse the same logic. > > > > Signed-off-by: Chester Lin <clin@suse.com> > > --- > > arch/x86/kernel/ima_arch.c | 47 ++------------------------------------ > > drivers/firmware/efi/efi.c | 43 ++++++++++++++++++++++++++++++++++ > > include/linux/efi.h | 5 ++++ > > 3 files changed, 50 insertions(+), 45 deletions(-) > > > > diff --git a/arch/x86/kernel/ima_arch.c b/arch/x86/kernel/ima_arch.c > > index 7dfb1e808928..ed4623ecda6e 100644 > > --- a/arch/x86/kernel/ima_arch.c > > +++ b/arch/x86/kernel/ima_arch.c > > @@ -8,49 +8,6 @@ > > > > extern struct boot_params boot_params; > > > > -static enum efi_secureboot_mode get_sb_mode(void) > > -{ > > - efi_guid_t efi_variable_guid = EFI_GLOBAL_VARIABLE_GUID; > > - efi_status_t status; > > - unsigned long size; > > - u8 secboot, setupmode; > > - > > - size = sizeof(secboot); > > - > > - if (!efi_rt_services_supported(EFI_RT_SUPPORTED_GET_VARIABLE)) { > > - pr_info("ima: secureboot mode unknown, no efi\n"); > > - return efi_secureboot_mode_unknown; > > - } > > - > > - /* Get variable contents into buffer */ > > - status = efi.get_variable(L"SecureBoot", &efi_variable_guid, > > - NULL, &size, &secboot); > > - if (status == EFI_NOT_FOUND) { > > - pr_info("ima: secureboot mode disabled\n"); > > - return efi_secureboot_mode_disabled; > > - } > > - > > - if (status != EFI_SUCCESS) { > > - pr_info("ima: secureboot mode unknown\n"); > > - return efi_secureboot_mode_unknown; > > - } > > - > > - size = sizeof(setupmode); > > - status = efi.get_variable(L"SetupMode", &efi_variable_guid, > > - NULL, &size, &setupmode); > > - > > - if (status != EFI_SUCCESS) /* ignore unknown SetupMode */ > > - setupmode = 0; > > - > > - if (secboot == 0 || setupmode == 1) { > > - pr_info("ima: secureboot mode disabled\n"); > > - return efi_secureboot_mode_disabled; > > - } > > - > > - pr_info("ima: secureboot mode enabled\n"); > > - return efi_secureboot_mode_enabled; > > -} > > - > > bool arch_ima_get_secureboot(void) > > { > > static enum efi_secureboot_mode sb_mode; > > @@ -60,7 +17,7 @@ bool arch_ima_get_secureboot(void) > > sb_mode = boot_params.secure_boot; > > > > if (sb_mode == efi_secureboot_mode_unset) > > - sb_mode = get_sb_mode(); > > + sb_mode = efi_get_secureboot_mode(); > > initialized = true; > > } > > > > @@ -70,7 +27,7 @@ bool arch_ima_get_secureboot(void) > > return false; > > } > > > > -/* secureboot arch rules */ > > +/* secure and trusted boot arch rules */ > > static const char * const sb_arch_rules[] = { > > #if !IS_ENABLED(CONFIG_KEXEC_SIG) > > "appraise func=KEXEC_KERNEL_CHECK appraise_type=imasig", > > diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c > > index 5e5480a0a32d..68ffa6a069c0 100644 > > --- a/drivers/firmware/efi/efi.c > > +++ b/drivers/firmware/efi/efi.c > > @@ -1022,3 +1022,46 @@ static int __init register_update_efi_random_seed(void) > > } > > late_initcall(register_update_efi_random_seed); > > #endif > > + > > +enum efi_secureboot_mode efi_get_secureboot_mode(void) > > +{ > > + efi_guid_t efi_variable_guid = EFI_GLOBAL_VARIABLE_GUID; > > + efi_status_t status; > > + unsigned long size; > > + u8 secboot, setupmode; > > + > > + size = sizeof(secboot); > > + > > + if (!efi_rt_services_supported(EFI_RT_SUPPORTED_GET_VARIABLE)) { > > + pr_info("ima: secureboot mode unknown, no efi\n"); > > These prints don't belong here anymore. > > Also, it would be useful if we could reuse this logic in the EFI stub > as well, which is built as a separate executable, and does not provide > efi.get_variable(). > > So, you could you please break this up into > > static inline > enum efi_secureboot_mode efi_get_secureboot_mode(efi_get_variable_t *get_var) > { > } > > placed into include/linux/efi.h, which encapsulates the core logic, > but using get_var(), and without the prints. > > Then, we could put something like > > bool efi_ima_get_secureboot(void) > { > } > > in drivers/firmware/efi/efi.c (guarded by #ifdef CONFIG_IMA_xxx), > which performs the > efi_rt_services_supported(EFI_RT_SUPPORTED_GET_VARIABLE)) check, calls > efi_get_secureboot_mode(efi.get_variable), and implements the logic. > > And actually, if the logic is identical between x86 and arm64, I > wonder if it wouldn't be better to put the whole thing into > > drivers/firmware/efi/efi-ima.c > > or > > security/integrity/ima/ima-efi.c > > with the only difference being the boot_params->secure_boot access for > x86, which we can factor out to a static inline helper. > > Mimi, any thoughts here? Sounds good. Keeping as much IMA code in the IMA directory makes sense. The IMA Makefile would then include ima-efi.c based on an EFI Kconfig option. thanks, Mimi > > > > > + return efi_secureboot_mode_unknown; > > + } > > + > > + /* Get variable contents into buffer */ > > + status = efi.get_variable(L"SecureBoot", &efi_variable_guid, > > + NULL, &size, &secboot); > > + if (status == EFI_NOT_FOUND) { > > + pr_info("ima: secureboot mode disabled\n"); > > + return efi_secureboot_mode_disabled; > > + } > > + > > + if (status != EFI_SUCCESS) { > > + pr_info("ima: secureboot mode unknown\n"); > > + return efi_secureboot_mode_unknown; > > + } > > + > > + size = sizeof(setupmode); > > + status = efi.get_variable(L"SetupMode", &efi_variable_guid, > > + NULL, &size, &setupmode); > > + > > + if (status != EFI_SUCCESS) /* ignore unknown SetupMode */ > > + setupmode = 0; > > + > > + if (secboot == 0 || setupmode == 1) { > > + pr_info("ima: secureboot mode disabled\n"); > > + return efi_secureboot_mode_disabled; > > + } > > + > > + pr_info("ima: secureboot mode enabled\n"); > > + return efi_secureboot_mode_enabled; > > +} > > diff --git a/include/linux/efi.h b/include/linux/efi.h > > index d7c0e73af2b9..a73e5ae04672 100644 > > --- a/include/linux/efi.h > > +++ b/include/linux/efi.h > > @@ -1076,8 +1076,13 @@ static inline int efi_runtime_map_copy(void *buf, size_t bufsz) > > > > #ifdef CONFIG_EFI > > extern bool efi_runtime_disabled(void); > > +extern enum efi_secureboot_mode efi_get_secureboot_mode(void); > > #else > > static inline bool efi_runtime_disabled(void) { return true; } > > +static inline enum efi_secureboot_mode efi_get_secureboot_mode(void) > > +{ > > + return efi_secureboot_mode_disabled; > > +} > > #endif > > > > extern void efi_call_virt_check_flags(unsigned long flags, const char *call); > > -- > > 2.26.1 > >
Hi Ard and Mimi, On Wed, Oct 14, 2020 at 07:56:17AM -0400, Mimi Zohar wrote: > On Wed, 2020-10-14 at 13:00 +0200, Ard Biesheuvel wrote: > > Hello Chester, > > > > Thanks for looking into this. > > > > Some comments below. > > > > On Wed, 14 Oct 2020 at 12:41, Chester Lin <clin@suse.com> wrote: > > > > > > Separate the get_sb_mode() from arch/x86 and treat it as a common function > > > [rename to efi_get_secureboot_mode] so all EFI-based architectures can > > > reuse the same logic. > > > > > > Signed-off-by: Chester Lin <clin@suse.com> > > > --- > > > arch/x86/kernel/ima_arch.c | 47 ++------------------------------------ > > > drivers/firmware/efi/efi.c | 43 ++++++++++++++++++++++++++++++++++ > > > include/linux/efi.h | 5 ++++ > > > 3 files changed, 50 insertions(+), 45 deletions(-) > > > > > > diff --git a/arch/x86/kernel/ima_arch.c b/arch/x86/kernel/ima_arch.c > > > index 7dfb1e808928..ed4623ecda6e 100644 > > > --- a/arch/x86/kernel/ima_arch.c > > > +++ b/arch/x86/kernel/ima_arch.c > > > @@ -8,49 +8,6 @@ > > > > > > extern struct boot_params boot_params; > > > > > > -static enum efi_secureboot_mode get_sb_mode(void) > > > -{ > > > - efi_guid_t efi_variable_guid = EFI_GLOBAL_VARIABLE_GUID; > > > - efi_status_t status; > > > - unsigned long size; > > > - u8 secboot, setupmode; > > > - > > > - size = sizeof(secboot); > > > - > > > - if (!efi_rt_services_supported(EFI_RT_SUPPORTED_GET_VARIABLE)) { > > > - pr_info("ima: secureboot mode unknown, no efi\n"); > > > - return efi_secureboot_mode_unknown; > > > - } > > > - > > > - /* Get variable contents into buffer */ > > > - status = efi.get_variable(L"SecureBoot", &efi_variable_guid, > > > - NULL, &size, &secboot); > > > - if (status == EFI_NOT_FOUND) { > > > - pr_info("ima: secureboot mode disabled\n"); > > > - return efi_secureboot_mode_disabled; > > > - } > > > - > > > - if (status != EFI_SUCCESS) { > > > - pr_info("ima: secureboot mode unknown\n"); > > > - return efi_secureboot_mode_unknown; > > > - } > > > - > > > - size = sizeof(setupmode); > > > - status = efi.get_variable(L"SetupMode", &efi_variable_guid, > > > - NULL, &size, &setupmode); > > > - > > > - if (status != EFI_SUCCESS) /* ignore unknown SetupMode */ > > > - setupmode = 0; > > > - > > > - if (secboot == 0 || setupmode == 1) { > > > - pr_info("ima: secureboot mode disabled\n"); > > > - return efi_secureboot_mode_disabled; > > > - } > > > - > > > - pr_info("ima: secureboot mode enabled\n"); > > > - return efi_secureboot_mode_enabled; > > > -} > > > - > > > bool arch_ima_get_secureboot(void) > > > { > > > static enum efi_secureboot_mode sb_mode; > > > @@ -60,7 +17,7 @@ bool arch_ima_get_secureboot(void) > > > sb_mode = boot_params.secure_boot; > > > > > > if (sb_mode == efi_secureboot_mode_unset) > > > - sb_mode = get_sb_mode(); > > > + sb_mode = efi_get_secureboot_mode(); > > > initialized = true; > > > } > > > > > > @@ -70,7 +27,7 @@ bool arch_ima_get_secureboot(void) > > > return false; > > > } > > > > > > -/* secureboot arch rules */ > > > +/* secure and trusted boot arch rules */ > > > static const char * const sb_arch_rules[] = { > > > #if !IS_ENABLED(CONFIG_KEXEC_SIG) > > > "appraise func=KEXEC_KERNEL_CHECK appraise_type=imasig", > > > diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c > > > index 5e5480a0a32d..68ffa6a069c0 100644 > > > --- a/drivers/firmware/efi/efi.c > > > +++ b/drivers/firmware/efi/efi.c > > > @@ -1022,3 +1022,46 @@ static int __init register_update_efi_random_seed(void) > > > } > > > late_initcall(register_update_efi_random_seed); > > > #endif > > > + > > > +enum efi_secureboot_mode efi_get_secureboot_mode(void) > > > +{ > > > + efi_guid_t efi_variable_guid = EFI_GLOBAL_VARIABLE_GUID; > > > + efi_status_t status; > > > + unsigned long size; > > > + u8 secboot, setupmode; > > > + > > > + size = sizeof(secboot); > > > + > > > + if (!efi_rt_services_supported(EFI_RT_SUPPORTED_GET_VARIABLE)) { > > > + pr_info("ima: secureboot mode unknown, no efi\n"); > > > > These prints don't belong here anymore. > > > > Also, it would be useful if we could reuse this logic in the EFI stub > > as well, which is built as a separate executable, and does not provide > > efi.get_variable(). > > > > So, you could you please break this up into > > > > static inline > > enum efi_secureboot_mode efi_get_secureboot_mode(efi_get_variable_t *get_var) > > { > > } > > > > placed into include/linux/efi.h, which encapsulates the core logic, > > but using get_var(), and without the prints. > > > > Then, we could put something like > > > > bool efi_ima_get_secureboot(void) > > { > > } > > > > in drivers/firmware/efi/efi.c (guarded by #ifdef CONFIG_IMA_xxx), > > which performs the > > efi_rt_services_supported(EFI_RT_SUPPORTED_GET_VARIABLE)) check, calls > > efi_get_secureboot_mode(efi.get_variable), and implements the logic. > > > > And actually, if the logic is identical between x86 and arm64, I > > wonder if it wouldn't be better to put the whole thing into > > > > drivers/firmware/efi/efi-ima.c > > > > or > > > > security/integrity/ima/ima-efi.c > > > > with the only difference being the boot_params->secure_boot access for > > x86, which we can factor out to a static inline helper. > > > > Mimi, any thoughts here? > > Sounds good. Keeping as much IMA code in the IMA directory makes > sense. The IMA Makefile would then include ima-efi.c based on an EFI > Kconfig option. > > thanks, > > Mimi Thanks for your suggestions. I will include them in v3. Regards, Chester > > > > > > > > > + return efi_secureboot_mode_unknown; > > > + } > > > + > > > + /* Get variable contents into buffer */ > > > + status = efi.get_variable(L"SecureBoot", &efi_variable_guid, > > > + NULL, &size, &secboot); > > > + if (status == EFI_NOT_FOUND) { > > > + pr_info("ima: secureboot mode disabled\n"); > > > + return efi_secureboot_mode_disabled; > > > + } > > > + > > > + if (status != EFI_SUCCESS) { > > > + pr_info("ima: secureboot mode unknown\n"); > > > + return efi_secureboot_mode_unknown; > > > + } > > > + > > > + size = sizeof(setupmode); > > > + status = efi.get_variable(L"SetupMode", &efi_variable_guid, > > > + NULL, &size, &setupmode); > > > + > > > + if (status != EFI_SUCCESS) /* ignore unknown SetupMode */ > > > + setupmode = 0; > > > + > > > + if (secboot == 0 || setupmode == 1) { > > > + pr_info("ima: secureboot mode disabled\n"); > > > + return efi_secureboot_mode_disabled; > > > + } > > > + > > > + pr_info("ima: secureboot mode enabled\n"); > > > + return efi_secureboot_mode_enabled; > > > +} > > > diff --git a/include/linux/efi.h b/include/linux/efi.h > > > index d7c0e73af2b9..a73e5ae04672 100644 > > > --- a/include/linux/efi.h > > > +++ b/include/linux/efi.h > > > @@ -1076,8 +1076,13 @@ static inline int efi_runtime_map_copy(void *buf, size_t bufsz) > > > > > > #ifdef CONFIG_EFI > > > extern bool efi_runtime_disabled(void); > > > +extern enum efi_secureboot_mode efi_get_secureboot_mode(void); > > > #else > > > static inline bool efi_runtime_disabled(void) { return true; } > > > +static inline enum efi_secureboot_mode efi_get_secureboot_mode(void) > > > +{ > > > + return efi_secureboot_mode_disabled; > > > +} > > > #endif > > > > > > extern void efi_call_virt_check_flags(unsigned long flags, const char *call); > > > -- > > > 2.26.1 > > > > >
diff --git a/arch/x86/kernel/ima_arch.c b/arch/x86/kernel/ima_arch.c index 7dfb1e808928..ed4623ecda6e 100644 --- a/arch/x86/kernel/ima_arch.c +++ b/arch/x86/kernel/ima_arch.c @@ -8,49 +8,6 @@ extern struct boot_params boot_params; -static enum efi_secureboot_mode get_sb_mode(void) -{ - efi_guid_t efi_variable_guid = EFI_GLOBAL_VARIABLE_GUID; - efi_status_t status; - unsigned long size; - u8 secboot, setupmode; - - size = sizeof(secboot); - - if (!efi_rt_services_supported(EFI_RT_SUPPORTED_GET_VARIABLE)) { - pr_info("ima: secureboot mode unknown, no efi\n"); - return efi_secureboot_mode_unknown; - } - - /* Get variable contents into buffer */ - status = efi.get_variable(L"SecureBoot", &efi_variable_guid, - NULL, &size, &secboot); - if (status == EFI_NOT_FOUND) { - pr_info("ima: secureboot mode disabled\n"); - return efi_secureboot_mode_disabled; - } - - if (status != EFI_SUCCESS) { - pr_info("ima: secureboot mode unknown\n"); - return efi_secureboot_mode_unknown; - } - - size = sizeof(setupmode); - status = efi.get_variable(L"SetupMode", &efi_variable_guid, - NULL, &size, &setupmode); - - if (status != EFI_SUCCESS) /* ignore unknown SetupMode */ - setupmode = 0; - - if (secboot == 0 || setupmode == 1) { - pr_info("ima: secureboot mode disabled\n"); - return efi_secureboot_mode_disabled; - } - - pr_info("ima: secureboot mode enabled\n"); - return efi_secureboot_mode_enabled; -} - bool arch_ima_get_secureboot(void) { static enum efi_secureboot_mode sb_mode; @@ -60,7 +17,7 @@ bool arch_ima_get_secureboot(void) sb_mode = boot_params.secure_boot; if (sb_mode == efi_secureboot_mode_unset) - sb_mode = get_sb_mode(); + sb_mode = efi_get_secureboot_mode(); initialized = true; } @@ -70,7 +27,7 @@ bool arch_ima_get_secureboot(void) return false; } -/* secureboot arch rules */ +/* secure and trusted boot arch rules */ static const char * const sb_arch_rules[] = { #if !IS_ENABLED(CONFIG_KEXEC_SIG) "appraise func=KEXEC_KERNEL_CHECK appraise_type=imasig", diff --git a/drivers/firmware/efi/efi.c b/drivers/firmware/efi/efi.c index 5e5480a0a32d..68ffa6a069c0 100644 --- a/drivers/firmware/efi/efi.c +++ b/drivers/firmware/efi/efi.c @@ -1022,3 +1022,46 @@ static int __init register_update_efi_random_seed(void) } late_initcall(register_update_efi_random_seed); #endif + +enum efi_secureboot_mode efi_get_secureboot_mode(void) +{ + efi_guid_t efi_variable_guid = EFI_GLOBAL_VARIABLE_GUID; + efi_status_t status; + unsigned long size; + u8 secboot, setupmode; + + size = sizeof(secboot); + + if (!efi_rt_services_supported(EFI_RT_SUPPORTED_GET_VARIABLE)) { + pr_info("ima: secureboot mode unknown, no efi\n"); + return efi_secureboot_mode_unknown; + } + + /* Get variable contents into buffer */ + status = efi.get_variable(L"SecureBoot", &efi_variable_guid, + NULL, &size, &secboot); + if (status == EFI_NOT_FOUND) { + pr_info("ima: secureboot mode disabled\n"); + return efi_secureboot_mode_disabled; + } + + if (status != EFI_SUCCESS) { + pr_info("ima: secureboot mode unknown\n"); + return efi_secureboot_mode_unknown; + } + + size = sizeof(setupmode); + status = efi.get_variable(L"SetupMode", &efi_variable_guid, + NULL, &size, &setupmode); + + if (status != EFI_SUCCESS) /* ignore unknown SetupMode */ + setupmode = 0; + + if (secboot == 0 || setupmode == 1) { + pr_info("ima: secureboot mode disabled\n"); + return efi_secureboot_mode_disabled; + } + + pr_info("ima: secureboot mode enabled\n"); + return efi_secureboot_mode_enabled; +} diff --git a/include/linux/efi.h b/include/linux/efi.h index d7c0e73af2b9..a73e5ae04672 100644 --- a/include/linux/efi.h +++ b/include/linux/efi.h @@ -1076,8 +1076,13 @@ static inline int efi_runtime_map_copy(void *buf, size_t bufsz) #ifdef CONFIG_EFI extern bool efi_runtime_disabled(void); +extern enum efi_secureboot_mode efi_get_secureboot_mode(void); #else static inline bool efi_runtime_disabled(void) { return true; } +static inline enum efi_secureboot_mode efi_get_secureboot_mode(void) +{ + return efi_secureboot_mode_disabled; +} #endif extern void efi_call_virt_check_flags(unsigned long flags, const char *call);
Separate the get_sb_mode() from arch/x86 and treat it as a common function [rename to efi_get_secureboot_mode] so all EFI-based architectures can reuse the same logic. Signed-off-by: Chester Lin <clin@suse.com> --- arch/x86/kernel/ima_arch.c | 47 ++------------------------------------ drivers/firmware/efi/efi.c | 43 ++++++++++++++++++++++++++++++++++ include/linux/efi.h | 5 ++++ 3 files changed, 50 insertions(+), 45 deletions(-)