* [PATCH v2 0/2] add ima_arch support for ARM64 @ 2020-10-14 10:40 Chester Lin 2020-10-14 10:40 ` [PATCH v2 1/2] efi: add secure boot get helper Chester Lin 2020-10-14 10:40 ` [PATCH v2 2/2] arm64/ima: add ima_arch support Chester Lin 0 siblings, 2 replies; 7+ messages in thread From: Chester Lin @ 2020-10-14 10:40 UTC (permalink / raw) To: zohar, ardb, catalin.marinas, will, tglx, mingo, bp, hpa, vincenzo.frascino, mark.rutland, samitolvanen, masahiroy Cc: clin, linux-efi, x86, linux-kernel, jlee, linux-integrity, linux-arm-kernel Add IMA arch dependent support for ARM64. Some IMA functions can check arch-specific status before running. For example, the ima_load_data function or the boot param "ima_appraise=" should not be executed when UEFI secure boot is enabled. We want to fill the gap in order to complete the IMA support on ARM64. Changes in v2: - Separate get_sb_mode() from x86 so all EFI-based architectures can reuse the same function. - Refactor arch/arm64/kernel/ima_arch.c based on Ard's patch[1]. Test platforms: - QEMU [aarch64-virt] + EDK2/OVMF - NXP LX2160A-RDB + EDK2 [1] https://www.spinics.net/lists/linux-efi/msg20645.html Chester Lin (2): efi: add secure boot get helper arm64/ima: add ima_arch support arch/arm64/Kconfig | 1 + arch/arm64/kernel/Makefile | 2 ++ arch/arm64/kernel/ima_arch.c | 46 +++++++++++++++++++++++++++++++++++ arch/x86/kernel/ima_arch.c | 47 ++---------------------------------- drivers/firmware/efi/efi.c | 43 +++++++++++++++++++++++++++++++++ include/linux/efi.h | 5 ++++ 6 files changed, 99 insertions(+), 45 deletions(-) create mode 100644 arch/arm64/kernel/ima_arch.c -- 2.26.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2 1/2] efi: add secure boot get helper 2020-10-14 10:40 [PATCH v2 0/2] add ima_arch support for ARM64 Chester Lin @ 2020-10-14 10:40 ` Chester Lin 2020-10-14 10:51 ` Chester Lin 2020-10-14 11:00 ` Ard Biesheuvel 2020-10-14 10:40 ` [PATCH v2 2/2] arm64/ima: add ima_arch support Chester Lin 1 sibling, 2 replies; 7+ messages in thread From: Chester Lin @ 2020-10-14 10:40 UTC (permalink / raw) To: zohar, ardb, catalin.marinas, will, tglx, mingo, bp, hpa, vincenzo.frascino, mark.rutland, samitolvanen, masahiroy Cc: clin, linux-efi, x86, linux-kernel, jlee, linux-integrity, linux-arm-kernel 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"); + 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 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/2] efi: add secure boot get helper 2020-10-14 10:40 ` [PATCH v2 1/2] efi: add secure boot get helper Chester Lin @ 2020-10-14 10:51 ` Chester Lin 2020-10-14 11:00 ` Ard Biesheuvel 1 sibling, 0 replies; 7+ messages in thread From: Chester Lin @ 2020-10-14 10:51 UTC (permalink / raw) To: zohar, ardb, catalin.marinas, will, tglx, mingo, bp, hpa, vincenzo.frascino, mark.rutland, samitolvanen, masahiroy Cc: clin, linux-efi, x86, linux-kernel, jlee, linux-integrity, linux-arm-kernel 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 > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/2] efi: add secure boot get helper 2020-10-14 10:40 ` [PATCH v2 1/2] efi: add secure boot get helper Chester Lin 2020-10-14 10:51 ` Chester Lin @ 2020-10-14 11:00 ` Ard Biesheuvel 2020-10-14 11:56 ` Mimi Zohar 1 sibling, 1 reply; 7+ messages in thread From: Ard Biesheuvel @ 2020-10-14 11:00 UTC (permalink / raw) To: Chester Lin Cc: Mark Rutland, linux-efi, Catalin Marinas, Masahiro Yamada, X86 ML, Linux Kernel Mailing List, Mimi Zohar, Lee, Chun-Yi, Ingo Molnar, Borislav Petkov, Sami Tolvanen, H. Peter Anvin, Thomas Gleixner, Vincenzo Frascino, Will Deacon, linux-integrity, Linux ARM 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 > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/2] efi: add secure boot get helper 2020-10-14 11:00 ` Ard Biesheuvel @ 2020-10-14 11:56 ` Mimi Zohar 2020-10-15 12:21 ` Chester Lin 0 siblings, 1 reply; 7+ messages in thread From: Mimi Zohar @ 2020-10-14 11:56 UTC (permalink / raw) To: Ard Biesheuvel, Chester Lin Cc: Mark Rutland, linux-efi, Catalin Marinas, Masahiro Yamada, X86 ML, Linux Kernel Mailing List, Lee, Chun-Yi, Ingo Molnar, Borislav Petkov, Sami Tolvanen, H. Peter Anvin, Thomas Gleixner, Vincenzo Frascino, Will Deacon, linux-integrity, Linux ARM 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 > > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2 1/2] efi: add secure boot get helper 2020-10-14 11:56 ` Mimi Zohar @ 2020-10-15 12:21 ` Chester Lin 0 siblings, 0 replies; 7+ messages in thread From: Chester Lin @ 2020-10-15 12:21 UTC (permalink / raw) To: Mimi Zohar Cc: Mark Rutland, clin, linux-efi, Catalin Marinas, Masahiro Yamada, X86 ML, Linux Kernel Mailing List, Lee, Chun-Yi, Ingo Molnar, Borislav Petkov, Sami Tolvanen, H. Peter Anvin, linux-integrity, Thomas Gleixner, Vincenzo Frascino, Will Deacon, Ard Biesheuvel, Linux ARM 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 > > > > > _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2 2/2] arm64/ima: add ima_arch support 2020-10-14 10:40 [PATCH v2 0/2] add ima_arch support for ARM64 Chester Lin 2020-10-14 10:40 ` [PATCH v2 1/2] efi: add secure boot get helper Chester Lin @ 2020-10-14 10:40 ` Chester Lin 1 sibling, 0 replies; 7+ messages in thread From: Chester Lin @ 2020-10-14 10:40 UTC (permalink / raw) To: zohar, ardb, catalin.marinas, will, tglx, mingo, bp, hpa, vincenzo.frascino, mark.rutland, samitolvanen, masahiroy Cc: clin, linux-efi, x86, linux-kernel, jlee, linux-integrity, linux-arm-kernel Add arm64 IMA arch support. The code and arch policy is mainly inherited from x86. Signed-off-by: Chester Lin <clin@suse.com> --- arch/arm64/Kconfig | 1 + arch/arm64/kernel/Makefile | 2 ++ arch/arm64/kernel/ima_arch.c | 46 ++++++++++++++++++++++++++++++++++++ 3 files changed, 49 insertions(+) create mode 100644 arch/arm64/kernel/ima_arch.c diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig index 08fa3a1c50f0..15b7a3bbb7e5 100644 --- a/arch/arm64/Kconfig +++ b/arch/arm64/Kconfig @@ -199,6 +199,7 @@ config ARM64 select SWIOTLB select SYSCTL_EXCEPTION_TRACE select THREAD_INFO_IN_TASK + imply IMA_SECURE_AND_OR_TRUSTED_BOOT if EFI help ARM 64-bit (AArch64) Linux support. diff --git a/arch/arm64/kernel/Makefile b/arch/arm64/kernel/Makefile index bbaf0bc4ad60..0f6cbb50668c 100644 --- a/arch/arm64/kernel/Makefile +++ b/arch/arm64/kernel/Makefile @@ -69,3 +69,5 @@ extra-y += $(head-y) vmlinux.lds ifeq ($(CONFIG_DEBUG_EFI),y) AFLAGS_head.o += -DVMLINUX_PATH="\"$(realpath $(objtree)/vmlinux)\"" endif + +obj-$(CONFIG_IMA_SECURE_AND_OR_TRUSTED_BOOT) += ima_arch.o diff --git a/arch/arm64/kernel/ima_arch.c b/arch/arm64/kernel/ima_arch.c new file mode 100644 index 000000000000..f3103c8a2cab --- /dev/null +++ b/arch/arm64/kernel/ima_arch.c @@ -0,0 +1,46 @@ +// SPDX-License-Identifier: GPL-2.0+ +/* + * Copyright (C) 2018 IBM Corporation + */ +#include <linux/efi.h> +#include <linux/module.h> +#include <linux/ima.h> + +bool arch_ima_get_secureboot(void) +{ + static enum efi_secureboot_mode sb_mode; + static bool initialized; + + if (!initialized && efi_enabled(EFI_BOOT)) { + sb_mode = efi_get_secureboot_mode(); + initialized = true; + } + + if (sb_mode == efi_secureboot_mode_enabled) + return true; + else + return false; +} + +/* 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", +#endif /* CONFIG_KEXEC_SIG */ + "measure func=KEXEC_KERNEL_CHECK", +#if !IS_ENABLED(CONFIG_MODULE_SIG) + "appraise func=MODULE_CHECK appraise_type=imasig", +#endif + "measure func=MODULE_CHECK", + NULL +}; + +const char * const *arch_get_ima_policy(void) +{ + if (IS_ENABLED(CONFIG_IMA_ARCH_POLICY) && arch_ima_get_secureboot()) { + if (IS_ENABLED(CONFIG_MODULE_SIG)) + set_module_sig_enforced(); + return sb_arch_rules; + } + return NULL; +} -- 2.26.1 _______________________________________________ linux-arm-kernel mailing list linux-arm-kernel@lists.infradead.org http://lists.infradead.org/mailman/listinfo/linux-arm-kernel ^ permalink raw reply related [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-10-15 12:23 UTC | newest] Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-10-14 10:40 [PATCH v2 0/2] add ima_arch support for ARM64 Chester Lin 2020-10-14 10:40 ` [PATCH v2 1/2] efi: add secure boot get helper Chester Lin 2020-10-14 10:51 ` Chester Lin 2020-10-14 11:00 ` Ard Biesheuvel 2020-10-14 11:56 ` Mimi Zohar 2020-10-15 12:21 ` Chester Lin 2020-10-14 10:40 ` [PATCH v2 2/2] arm64/ima: add ima_arch support Chester Lin
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).