* [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: linux-arm-kernel, linux-kernel, x86, linux-integrity, linux-efi, jlee, clin 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 ^ 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: linux-arm-kernel, linux-kernel, x86, linux-integrity, linux-efi, jlee, clin 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 ^ 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: linux-arm-kernel, linux-kernel, x86, linux-integrity, linux-efi, jlee, clin 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 > ^ 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: Mimi Zohar, Catalin Marinas, Will Deacon, Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin, Vincenzo Frascino, Mark Rutland, Sami Tolvanen, Masahiro Yamada, Linux ARM, Linux Kernel Mailing List, X86 ML, linux-integrity, linux-efi, Lee, Chun-Yi 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 > ^ 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: Catalin Marinas, Will Deacon, Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin, Vincenzo Frascino, Mark Rutland, Sami Tolvanen, Masahiro Yamada, Linux ARM, Linux Kernel Mailing List, X86 ML, linux-integrity, linux-efi, Lee, Chun-Yi 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 > > ^ 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: Ard Biesheuvel, Catalin Marinas, Will Deacon, Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin, Vincenzo Frascino, Mark Rutland, Sami Tolvanen, Masahiro Yamada, Linux ARM, Linux Kernel Mailing List, X86 ML, linux-integrity, linux-efi, Lee, Chun-Yi, clin 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 > > > > > ^ 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: linux-arm-kernel, linux-kernel, x86, linux-integrity, linux-efi, jlee, clin 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 ^ permalink raw reply related [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-10-15 12:21 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).