* [PATCH v3 0/3] add ima_arch support for ARM64
@ 2020-10-30 6:08 Chester Lin
2020-10-30 6:08 ` [PATCH v3 1/3] efi: generalize efi_get_secureboot Chester Lin
` (2 more replies)
0 siblings, 3 replies; 10+ messages in thread
From: Chester Lin @ 2020-10-30 6:08 UTC (permalink / raw)
To: ardb, zohar, jmorris, serge, dmitry.kasatkin, catalin.marinas,
will, tglx, mingo, bp, hpa
Cc: linux-kernel, linux-arm-kernel, linux-efi, linux-integrity,
linux-security-module, x86, 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 v3:
- Generalize efi_get_secureboot() so both ima_arch and efistub can reuse
it.
- Implement ima_get_efi_secureboot() as the replacement of get_sb_mode()
so x86 and arm64 can share the same logic.
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:
- ARM64: QEMU [aarch64-virt] + EDK2/OVMF
- ARM64: NXP LX2160A-RDB + EDK2
- X86_64: Dell Lattitude 7490 + (BIOS 1.14.0 01/22/2020)
[1] https://www.spinics.net/lists/linux-efi/msg20645.html
Chester Lin (3):
efi: generalize efi_get_secureboot
ima: remove get_sb_mode() and create ima_get_efi_secureboot()
arm64/ima: add ima_arch support
arch/arm64/Kconfig | 1 +
arch/arm64/kernel/Makefile | 2 +
arch/arm64/kernel/ima_arch.c | 43 +++++++++++++
arch/x86/kernel/ima_arch.c | 69 +++++---------------
drivers/firmware/efi/libstub/Makefile | 2 +-
drivers/firmware/efi/libstub/efi-stub.c | 2 +-
drivers/firmware/efi/libstub/efistub.h | 22 ++++---
drivers/firmware/efi/libstub/secureboot.c | 76 -----------------------
drivers/firmware/efi/libstub/x86-stub.c | 2 +-
include/linux/efi.h | 41 +++++++++++-
include/linux/ima.h | 10 +++
security/integrity/ima/Makefile | 1 +
security/integrity/ima/ima_efi.c | 26 ++++++++
13 files changed, 154 insertions(+), 143 deletions(-)
create mode 100644 arch/arm64/kernel/ima_arch.c
delete mode 100644 drivers/firmware/efi/libstub/secureboot.c
create mode 100644 security/integrity/ima/ima_efi.c
--
2.28.0
^ permalink raw reply [flat|nested] 10+ messages in thread* [PATCH v3 1/3] efi: generalize efi_get_secureboot 2020-10-30 6:08 [PATCH v3 0/3] add ima_arch support for ARM64 Chester Lin @ 2020-10-30 6:08 ` Chester Lin 2020-10-30 11:51 ` Ard Biesheuvel 2020-10-30 6:08 ` [PATCH v3 2/3] ima: replace arch-specific get_sb_mode() with a common helper ima_get_efi_secureboot() Chester Lin 2020-10-30 6:08 ` [PATCH v3 3/3] arm64/ima: add ima_arch support Chester Lin 2 siblings, 1 reply; 10+ messages in thread From: Chester Lin @ 2020-10-30 6:08 UTC (permalink / raw) To: ardb, zohar, jmorris, serge, dmitry.kasatkin, catalin.marinas, will, tglx, mingo, bp, hpa Cc: linux-kernel, linux-arm-kernel, linux-efi, linux-integrity, linux-security-module, x86, jlee, clin Generalize the efi_get_secureboot() function so not only efistub but also other subsystems can use it. Signed-off-by: Chester Lin <clin@suse.com> --- drivers/firmware/efi/libstub/Makefile | 2 +- drivers/firmware/efi/libstub/efi-stub.c | 2 +- drivers/firmware/efi/libstub/efistub.h | 22 ++++--- drivers/firmware/efi/libstub/secureboot.c | 76 ----------------------- drivers/firmware/efi/libstub/x86-stub.c | 2 +- include/linux/efi.h | 41 +++++++++++- 6 files changed, 57 insertions(+), 88 deletions(-) delete mode 100644 drivers/firmware/efi/libstub/secureboot.c diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile index 8a94388e38b3..88e47b0ca09d 100644 --- a/drivers/firmware/efi/libstub/Makefile +++ b/drivers/firmware/efi/libstub/Makefile @@ -49,7 +49,7 @@ OBJECT_FILES_NON_STANDARD := y # Prevents link failures: __sanitizer_cov_trace_pc() is not linked in. KCOV_INSTRUMENT := n -lib-y := efi-stub-helper.o gop.o secureboot.o tpm.o \ +lib-y := efi-stub-helper.o gop.o tpm.o \ file.o mem.o random.o randomalloc.o pci.o \ skip_spaces.o lib-cmdline.o lib-ctype.o \ alignedmem.o relocate.o vsprintf.o diff --git a/drivers/firmware/efi/libstub/efi-stub.c b/drivers/firmware/efi/libstub/efi-stub.c index 914a343c7785..ad96f1d786a9 100644 --- a/drivers/firmware/efi/libstub/efi-stub.c +++ b/drivers/firmware/efi/libstub/efi-stub.c @@ -196,7 +196,7 @@ efi_status_t __efiapi efi_pe_entry(efi_handle_t handle, /* Ask the firmware to clear memory on unclean shutdown */ efi_enable_reset_attack_mitigation(); - secure_boot = efi_get_secureboot(); + secure_boot = efi_get_secureboot(get_efi_var); /* * Unauthenticated device tree data is a security hazard, so ignore diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h index 2d7abcd99de9..b1833b51e6d6 100644 --- a/drivers/firmware/efi/libstub/efistub.h +++ b/drivers/firmware/efi/libstub/efistub.h @@ -91,14 +91,6 @@ efi_status_t __efiapi efi_pe_entry(efi_handle_t handle, fdt_setprop((fdt), (node_offset), (name), &(var), sizeof(var)) #endif -#define get_efi_var(name, vendor, ...) \ - efi_rt_call(get_variable, (efi_char16_t *)(name), \ - (efi_guid_t *)(vendor), __VA_ARGS__) - -#define set_efi_var(name, vendor, ...) \ - efi_rt_call(set_variable, (efi_char16_t *)(name), \ - (efi_guid_t *)(vendor), __VA_ARGS__) - #define efi_get_handle_at(array, idx) \ (efi_is_native() ? (array)[idx] \ : (efi_handle_t)(unsigned long)((u32 *)(array))[idx]) @@ -112,6 +104,20 @@ efi_status_t __efiapi efi_pe_entry(efi_handle_t handle, ((handle = efi_get_handle_at((array), i)) || true); \ i++) +static inline +efi_status_t get_efi_var(efi_char16_t *name, efi_guid_t *vendor, u32 *attr, + unsigned long *size, void *data) +{ + return efi_rt_call(get_variable, name, vendor, attr, size, data); +} + +static inline +efi_status_t set_efi_var(efi_char16_t *name, efi_guid_t *vendor, u32 attr, + unsigned long size, void *data) +{ + return efi_rt_call(set_variable, name, vendor, attr, size, data); +} + static inline void efi_set_u64_split(u64 data, u32 *lo, u32 *hi) { diff --git a/drivers/firmware/efi/libstub/secureboot.c b/drivers/firmware/efi/libstub/secureboot.c deleted file mode 100644 index 5efc524b14be..000000000000 --- a/drivers/firmware/efi/libstub/secureboot.c +++ /dev/null @@ -1,76 +0,0 @@ -// SPDX-License-Identifier: GPL-2.0 -/* - * Secure boot handling. - * - * Copyright (C) 2013,2014 Linaro Limited - * Roy Franz <roy.franz@linaro.org - * Copyright (C) 2013 Red Hat, Inc. - * Mark Salter <msalter@redhat.com> - */ -#include <linux/efi.h> -#include <asm/efi.h> - -#include "efistub.h" - -/* BIOS variables */ -static const efi_guid_t efi_variable_guid = EFI_GLOBAL_VARIABLE_GUID; -static const efi_char16_t efi_SecureBoot_name[] = L"SecureBoot"; -static const efi_char16_t efi_SetupMode_name[] = L"SetupMode"; - -/* SHIM variables */ -static const efi_guid_t shim_guid = EFI_SHIM_LOCK_GUID; -static const efi_char16_t shim_MokSBState_name[] = L"MokSBState"; - -/* - * Determine whether we're in secure boot mode. - * - * Please keep the logic in sync with - * arch/x86/xen/efi.c:xen_efi_get_secureboot(). - */ -enum efi_secureboot_mode efi_get_secureboot(void) -{ - u32 attr; - u8 secboot, setupmode, moksbstate; - unsigned long size; - efi_status_t status; - - size = sizeof(secboot); - status = get_efi_var(efi_SecureBoot_name, &efi_variable_guid, - NULL, &size, &secboot); - if (status == EFI_NOT_FOUND) - return efi_secureboot_mode_disabled; - if (status != EFI_SUCCESS) - goto out_efi_err; - - size = sizeof(setupmode); - status = get_efi_var(efi_SetupMode_name, &efi_variable_guid, - NULL, &size, &setupmode); - if (status != EFI_SUCCESS) - goto out_efi_err; - - if (secboot == 0 || setupmode == 1) - return efi_secureboot_mode_disabled; - - /* - * See if a user has put the shim into insecure mode. If so, and if the - * variable doesn't have the runtime attribute set, we might as well - * honor that. - */ - size = sizeof(moksbstate); - status = get_efi_var(shim_MokSBState_name, &shim_guid, - &attr, &size, &moksbstate); - - /* If it fails, we don't care why. Default to secure */ - if (status != EFI_SUCCESS) - goto secure_boot_enabled; - if (!(attr & EFI_VARIABLE_RUNTIME_ACCESS) && moksbstate == 1) - return efi_secureboot_mode_disabled; - -secure_boot_enabled: - efi_info("UEFI Secure Boot is enabled.\n"); - return efi_secureboot_mode_enabled; - -out_efi_err: - efi_err("Could not determine UEFI Secure Boot status.\n"); - return efi_secureboot_mode_unknown; -} diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c index 3672539cb96e..3f9b492c566b 100644 --- a/drivers/firmware/efi/libstub/x86-stub.c +++ b/drivers/firmware/efi/libstub/x86-stub.c @@ -781,7 +781,7 @@ unsigned long efi_main(efi_handle_t handle, * otherwise we ask the BIOS. */ if (boot_params->secure_boot == efi_secureboot_mode_unset) - boot_params->secure_boot = efi_get_secureboot(); + boot_params->secure_boot = efi_get_secureboot(get_efi_var); /* Ask the firmware to clear memory on unclean shutdown */ efi_enable_reset_attack_mitigation(); diff --git a/include/linux/efi.h b/include/linux/efi.h index d7c0e73af2b9..cc2d3de39031 100644 --- a/include/linux/efi.h +++ b/include/linux/efi.h @@ -1089,7 +1089,46 @@ enum efi_secureboot_mode { efi_secureboot_mode_disabled, efi_secureboot_mode_enabled, }; -enum efi_secureboot_mode efi_get_secureboot(void); + +static inline enum efi_secureboot_mode efi_get_secureboot(efi_get_variable_t *get_var) +{ + efi_guid_t var_guid = EFI_GLOBAL_VARIABLE_GUID; + efi_guid_t shim_guid = EFI_SHIM_LOCK_GUID; + efi_status_t status; + unsigned long size; + u8 secboot, setupmode, moksbstate; + u32 attr; + + size = sizeof(secboot); + status = get_var(L"SecureBoot", &var_guid, NULL, &size, &secboot); + + if (status == EFI_NOT_FOUND) + return efi_secureboot_mode_disabled; + if (status != EFI_SUCCESS) + return efi_secureboot_mode_unknown; + + size = sizeof(setupmode); + status = get_var(L"SetupMode", &var_guid, NULL, &size, &setupmode); + + if (status != EFI_SUCCESS) + return efi_secureboot_mode_unknown; + if (secboot == 0 || setupmode == 1) + return efi_secureboot_mode_disabled; + + /* + * See if a user has put the shim into insecure mode. If so, and if the + * variable doesn't have the runtime attribute set, we might as well + * honor that. + */ + size = sizeof(moksbstate); + status = get_var(L"MokSBState", &shim_guid, &attr, &size, &moksbstate); + /* If it fails, we don't care why. Default to secure */ + if (status == EFI_SUCCESS && moksbstate == 1 + && !(attr & EFI_VARIABLE_RUNTIME_ACCESS)) + return efi_secureboot_mode_disabled; + + return efi_secureboot_mode_enabled; +} #ifdef CONFIG_RESET_ATTACK_MITIGATION void efi_enable_reset_attack_mitigation(void); -- 2.28.0 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v3 1/3] efi: generalize efi_get_secureboot 2020-10-30 6:08 ` [PATCH v3 1/3] efi: generalize efi_get_secureboot Chester Lin @ 2020-10-30 11:51 ` Ard Biesheuvel 2020-11-02 5:30 ` Chester Lin 0 siblings, 1 reply; 10+ messages in thread From: Ard Biesheuvel @ 2020-10-30 11:51 UTC (permalink / raw) To: Chester Lin Cc: Mimi Zohar, James Morris, Serge E. Hallyn, Dmitry Kasatkin, Catalin Marinas, Will Deacon, Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin, Linux Kernel Mailing List, Linux ARM, linux-efi, linux-integrity, linux-security-module, X86 ML, Lee, Chun-Yi Hello Chester, Thanks again for looking into this. On Fri, 30 Oct 2020 at 07:09, Chester Lin <clin@suse.com> wrote: > > Generalize the efi_get_secureboot() function so not only efistub but also > other subsystems can use it. > > Signed-off-by: Chester Lin <clin@suse.com> > --- > drivers/firmware/efi/libstub/Makefile | 2 +- > drivers/firmware/efi/libstub/efi-stub.c | 2 +- > drivers/firmware/efi/libstub/efistub.h | 22 ++++--- > drivers/firmware/efi/libstub/secureboot.c | 76 ----------------------- > drivers/firmware/efi/libstub/x86-stub.c | 2 +- > include/linux/efi.h | 41 +++++++++++- > 6 files changed, 57 insertions(+), 88 deletions(-) > delete mode 100644 drivers/firmware/efi/libstub/secureboot.c > > diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile > index 8a94388e38b3..88e47b0ca09d 100644 > --- a/drivers/firmware/efi/libstub/Makefile > +++ b/drivers/firmware/efi/libstub/Makefile > @@ -49,7 +49,7 @@ OBJECT_FILES_NON_STANDARD := y > # Prevents link failures: __sanitizer_cov_trace_pc() is not linked in. > KCOV_INSTRUMENT := n > > -lib-y := efi-stub-helper.o gop.o secureboot.o tpm.o \ > +lib-y := efi-stub-helper.o gop.o tpm.o \ > file.o mem.o random.o randomalloc.o pci.o \ > skip_spaces.o lib-cmdline.o lib-ctype.o \ > alignedmem.o relocate.o vsprintf.o > diff --git a/drivers/firmware/efi/libstub/efi-stub.c b/drivers/firmware/efi/libstub/efi-stub.c > index 914a343c7785..ad96f1d786a9 100644 > --- a/drivers/firmware/efi/libstub/efi-stub.c > +++ b/drivers/firmware/efi/libstub/efi-stub.c > @@ -196,7 +196,7 @@ efi_status_t __efiapi efi_pe_entry(efi_handle_t handle, > /* Ask the firmware to clear memory on unclean shutdown */ > efi_enable_reset_attack_mitigation(); > > - secure_boot = efi_get_secureboot(); > + secure_boot = efi_get_secureboot(get_efi_var); > > /* > * Unauthenticated device tree data is a security hazard, so ignore > diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h > index 2d7abcd99de9..b1833b51e6d6 100644 > --- a/drivers/firmware/efi/libstub/efistub.h > +++ b/drivers/firmware/efi/libstub/efistub.h > @@ -91,14 +91,6 @@ efi_status_t __efiapi efi_pe_entry(efi_handle_t handle, > fdt_setprop((fdt), (node_offset), (name), &(var), sizeof(var)) > #endif > > -#define get_efi_var(name, vendor, ...) \ > - efi_rt_call(get_variable, (efi_char16_t *)(name), \ > - (efi_guid_t *)(vendor), __VA_ARGS__) > - > -#define set_efi_var(name, vendor, ...) \ > - efi_rt_call(set_variable, (efi_char16_t *)(name), \ > - (efi_guid_t *)(vendor), __VA_ARGS__) > - > #define efi_get_handle_at(array, idx) \ > (efi_is_native() ? (array)[idx] \ > : (efi_handle_t)(unsigned long)((u32 *)(array))[idx]) > @@ -112,6 +104,20 @@ efi_status_t __efiapi efi_pe_entry(efi_handle_t handle, > ((handle = efi_get_handle_at((array), i)) || true); \ > i++) > > +static inline > +efi_status_t get_efi_var(efi_char16_t *name, efi_guid_t *vendor, u32 *attr, > + unsigned long *size, void *data) > +{ > + return efi_rt_call(get_variable, name, vendor, attr, size, data); > +} > + > +static inline > +efi_status_t set_efi_var(efi_char16_t *name, efi_guid_t *vendor, u32 attr, > + unsigned long size, void *data) > +{ > + return efi_rt_call(set_variable, name, vendor, attr, size, data); > +} > + > static inline > void efi_set_u64_split(u64 data, u32 *lo, u32 *hi) > { > diff --git a/drivers/firmware/efi/libstub/secureboot.c b/drivers/firmware/efi/libstub/secureboot.c > deleted file mode 100644 > index 5efc524b14be..000000000000 > --- a/drivers/firmware/efi/libstub/secureboot.c > +++ /dev/null Please keep this file (see below) > @@ -1,76 +0,0 @@ > -// SPDX-License-Identifier: GPL-2.0 > -/* > - * Secure boot handling. > - * > - * Copyright (C) 2013,2014 Linaro Limited > - * Roy Franz <roy.franz@linaro.org > - * Copyright (C) 2013 Red Hat, Inc. > - * Mark Salter <msalter@redhat.com> > - */ > -#include <linux/efi.h> > -#include <asm/efi.h> > - > -#include "efistub.h" > - > -/* BIOS variables */ > -static const efi_guid_t efi_variable_guid = EFI_GLOBAL_VARIABLE_GUID; > -static const efi_char16_t efi_SecureBoot_name[] = L"SecureBoot"; > -static const efi_char16_t efi_SetupMode_name[] = L"SetupMode"; > - > -/* SHIM variables */ > -static const efi_guid_t shim_guid = EFI_SHIM_LOCK_GUID; > -static const efi_char16_t shim_MokSBState_name[] = L"MokSBState"; > - > -/* > - * Determine whether we're in secure boot mode. > - * > - * Please keep the logic in sync with > - * arch/x86/xen/efi.c:xen_efi_get_secureboot(). > - */ > -enum efi_secureboot_mode efi_get_secureboot(void) > -{ > - u32 attr; > - u8 secboot, setupmode, moksbstate; > - unsigned long size; > - efi_status_t status; > - > - size = sizeof(secboot); > - status = get_efi_var(efi_SecureBoot_name, &efi_variable_guid, > - NULL, &size, &secboot); > - if (status == EFI_NOT_FOUND) > - return efi_secureboot_mode_disabled; > - if (status != EFI_SUCCESS) > - goto out_efi_err; > - > - size = sizeof(setupmode); > - status = get_efi_var(efi_SetupMode_name, &efi_variable_guid, > - NULL, &size, &setupmode); > - if (status != EFI_SUCCESS) > - goto out_efi_err; > - > - if (secboot == 0 || setupmode == 1) > - return efi_secureboot_mode_disabled; > - > - /* > - * See if a user has put the shim into insecure mode. If so, and if the > - * variable doesn't have the runtime attribute set, we might as well > - * honor that. > - */ > - size = sizeof(moksbstate); > - status = get_efi_var(shim_MokSBState_name, &shim_guid, > - &attr, &size, &moksbstate); > - MokSBState is a boot time variable, so we cannot access it when running under the OS. Xen also has a code flow similar to this one, but it looks at MokSbStateRt instead (which may be a mistake but let's forget about that for now) So what we will need to do is factor out only the top part of this function (which, incidentally, is the only part that IMA uses in the first place) > - /* If it fails, we don't care why. Default to secure */ > - if (status != EFI_SUCCESS) > - goto secure_boot_enabled; > - if (!(attr & EFI_VARIABLE_RUNTIME_ACCESS) && moksbstate == 1) > - return efi_secureboot_mode_disabled; > - > -secure_boot_enabled: > - efi_info("UEFI Secure Boot is enabled.\n"); > - return efi_secureboot_mode_enabled; > - > -out_efi_err: > - efi_err("Could not determine UEFI Secure Boot status.\n"); > - return efi_secureboot_mode_unknown; > -} So let's keep this file, and also, let's put a wrapper function around get_efi_var() here, of which you can take the address and pass to the static inline function. > diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c > index 3672539cb96e..3f9b492c566b 100644 > --- a/drivers/firmware/efi/libstub/x86-stub.c > +++ b/drivers/firmware/efi/libstub/x86-stub.c > @@ -781,7 +781,7 @@ unsigned long efi_main(efi_handle_t handle, > * otherwise we ask the BIOS. > */ > if (boot_params->secure_boot == efi_secureboot_mode_unset) > - boot_params->secure_boot = efi_get_secureboot(); > + boot_params->secure_boot = efi_get_secureboot(get_efi_var); > > /* Ask the firmware to clear memory on unclean shutdown */ > efi_enable_reset_attack_mitigation(); > diff --git a/include/linux/efi.h b/include/linux/efi.h > index d7c0e73af2b9..cc2d3de39031 100644 > --- a/include/linux/efi.h > +++ b/include/linux/efi.h > @@ -1089,7 +1089,46 @@ enum efi_secureboot_mode { > efi_secureboot_mode_disabled, > efi_secureboot_mode_enabled, > }; > -enum efi_secureboot_mode efi_get_secureboot(void); > + > +static inline enum efi_secureboot_mode efi_get_secureboot(efi_get_variable_t *get_var) > +{ > + efi_guid_t var_guid = EFI_GLOBAL_VARIABLE_GUID; > + efi_guid_t shim_guid = EFI_SHIM_LOCK_GUID; > + efi_status_t status; > + unsigned long size; > + u8 secboot, setupmode, moksbstate; > + u32 attr; > + > + size = sizeof(secboot); > + status = get_var(L"SecureBoot", &var_guid, NULL, &size, &secboot); > + > + if (status == EFI_NOT_FOUND) > + return efi_secureboot_mode_disabled; > + if (status != EFI_SUCCESS) > + return efi_secureboot_mode_unknown; > + > + size = sizeof(setupmode); > + status = get_var(L"SetupMode", &var_guid, NULL, &size, &setupmode); > + > + if (status != EFI_SUCCESS) > + return efi_secureboot_mode_unknown; > + if (secboot == 0 || setupmode == 1) > + return efi_secureboot_mode_disabled; > + So keep until here and move the rest back into the .c file > + /* > + * See if a user has put the shim into insecure mode. If so, and if the > + * variable doesn't have the runtime attribute set, we might as well > + * honor that. > + */ > + size = sizeof(moksbstate); > + status = get_var(L"MokSBState", &shim_guid, &attr, &size, &moksbstate); > + /* If it fails, we don't care why. Default to secure */ > + if (status == EFI_SUCCESS && moksbstate == 1 > + && !(attr & EFI_VARIABLE_RUNTIME_ACCESS)) > + return efi_secureboot_mode_disabled; > + > + return efi_secureboot_mode_enabled; > +} > > #ifdef CONFIG_RESET_ATTACK_MITIGATION > void efi_enable_reset_attack_mitigation(void); > -- > 2.28.0 > ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3 1/3] efi: generalize efi_get_secureboot 2020-10-30 11:51 ` Ard Biesheuvel @ 2020-11-02 5:30 ` Chester Lin 0 siblings, 0 replies; 10+ messages in thread From: Chester Lin @ 2020-11-02 5:30 UTC (permalink / raw) To: Ard Biesheuvel Cc: Mimi Zohar, James Morris, Serge E. Hallyn, Dmitry Kasatkin, Catalin Marinas, Will Deacon, Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin, Linux Kernel Mailing List, Linux ARM, linux-efi, linux-integrity, linux-security-module, X86 ML, Lee, Chun-Yi Hi Ard, Thanks for your time and reviewing. On Fri, Oct 30, 2020 at 12:51:10PM +0100, Ard Biesheuvel wrote: > Hello Chester, > > Thanks again for looking into this. > > On Fri, 30 Oct 2020 at 07:09, Chester Lin <clin@suse.com> wrote: > > > > Generalize the efi_get_secureboot() function so not only efistub but also > > other subsystems can use it. > > > > Signed-off-by: Chester Lin <clin@suse.com> > > --- > > drivers/firmware/efi/libstub/Makefile | 2 +- > > drivers/firmware/efi/libstub/efi-stub.c | 2 +- > > drivers/firmware/efi/libstub/efistub.h | 22 ++++--- > > drivers/firmware/efi/libstub/secureboot.c | 76 ----------------------- > > drivers/firmware/efi/libstub/x86-stub.c | 2 +- > > include/linux/efi.h | 41 +++++++++++- > > 6 files changed, 57 insertions(+), 88 deletions(-) > > delete mode 100644 drivers/firmware/efi/libstub/secureboot.c > > > > diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile > > index 8a94388e38b3..88e47b0ca09d 100644 > > --- a/drivers/firmware/efi/libstub/Makefile > > +++ b/drivers/firmware/efi/libstub/Makefile > > @@ -49,7 +49,7 @@ OBJECT_FILES_NON_STANDARD := y > > # Prevents link failures: __sanitizer_cov_trace_pc() is not linked in. > > KCOV_INSTRUMENT := n > > > > -lib-y := efi-stub-helper.o gop.o secureboot.o tpm.o \ > > +lib-y := efi-stub-helper.o gop.o tpm.o \ > > file.o mem.o random.o randomalloc.o pci.o \ > > skip_spaces.o lib-cmdline.o lib-ctype.o \ > > alignedmem.o relocate.o vsprintf.o > > diff --git a/drivers/firmware/efi/libstub/efi-stub.c b/drivers/firmware/efi/libstub/efi-stub.c > > index 914a343c7785..ad96f1d786a9 100644 > > --- a/drivers/firmware/efi/libstub/efi-stub.c > > +++ b/drivers/firmware/efi/libstub/efi-stub.c > > @@ -196,7 +196,7 @@ efi_status_t __efiapi efi_pe_entry(efi_handle_t handle, > > /* Ask the firmware to clear memory on unclean shutdown */ > > efi_enable_reset_attack_mitigation(); > > > > - secure_boot = efi_get_secureboot(); > > + secure_boot = efi_get_secureboot(get_efi_var); > > > > /* > > * Unauthenticated device tree data is a security hazard, so ignore > > diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h > > index 2d7abcd99de9..b1833b51e6d6 100644 > > --- a/drivers/firmware/efi/libstub/efistub.h > > +++ b/drivers/firmware/efi/libstub/efistub.h > > @@ -91,14 +91,6 @@ efi_status_t __efiapi efi_pe_entry(efi_handle_t handle, > > fdt_setprop((fdt), (node_offset), (name), &(var), sizeof(var)) > > #endif > > > > -#define get_efi_var(name, vendor, ...) \ > > - efi_rt_call(get_variable, (efi_char16_t *)(name), \ > > - (efi_guid_t *)(vendor), __VA_ARGS__) > > - > > -#define set_efi_var(name, vendor, ...) \ > > - efi_rt_call(set_variable, (efi_char16_t *)(name), \ > > - (efi_guid_t *)(vendor), __VA_ARGS__) > > - > > #define efi_get_handle_at(array, idx) \ > > (efi_is_native() ? (array)[idx] \ > > : (efi_handle_t)(unsigned long)((u32 *)(array))[idx]) > > @@ -112,6 +104,20 @@ efi_status_t __efiapi efi_pe_entry(efi_handle_t handle, > > ((handle = efi_get_handle_at((array), i)) || true); \ > > i++) > > > > +static inline > > +efi_status_t get_efi_var(efi_char16_t *name, efi_guid_t *vendor, u32 *attr, > > + unsigned long *size, void *data) > > +{ > > + return efi_rt_call(get_variable, name, vendor, attr, size, data); > > +} > > + > > +static inline > > +efi_status_t set_efi_var(efi_char16_t *name, efi_guid_t *vendor, u32 attr, > > + unsigned long size, void *data) > > +{ > > + return efi_rt_call(set_variable, name, vendor, attr, size, data); > > +} > > + > > static inline > > void efi_set_u64_split(u64 data, u32 *lo, u32 *hi) > > { > > diff --git a/drivers/firmware/efi/libstub/secureboot.c b/drivers/firmware/efi/libstub/secureboot.c > > deleted file mode 100644 > > index 5efc524b14be..000000000000 > > --- a/drivers/firmware/efi/libstub/secureboot.c > > +++ /dev/null > > Please keep this file (see below) > > > @@ -1,76 +0,0 @@ > > -// SPDX-License-Identifier: GPL-2.0 > > -/* > > - * Secure boot handling. > > - * > > - * Copyright (C) 2013,2014 Linaro Limited > > - * Roy Franz <roy.franz@linaro.org > > - * Copyright (C) 2013 Red Hat, Inc. > > - * Mark Salter <msalter@redhat.com> > > - */ > > -#include <linux/efi.h> > > -#include <asm/efi.h> > > - > > -#include "efistub.h" > > - > > -/* BIOS variables */ > > -static const efi_guid_t efi_variable_guid = EFI_GLOBAL_VARIABLE_GUID; > > -static const efi_char16_t efi_SecureBoot_name[] = L"SecureBoot"; > > -static const efi_char16_t efi_SetupMode_name[] = L"SetupMode"; > > - > > -/* SHIM variables */ > > -static const efi_guid_t shim_guid = EFI_SHIM_LOCK_GUID; > > -static const efi_char16_t shim_MokSBState_name[] = L"MokSBState"; > > - > > -/* > > - * Determine whether we're in secure boot mode. > > - * > > - * Please keep the logic in sync with > > - * arch/x86/xen/efi.c:xen_efi_get_secureboot(). > > - */ > > -enum efi_secureboot_mode efi_get_secureboot(void) > > -{ > > - u32 attr; > > - u8 secboot, setupmode, moksbstate; > > - unsigned long size; > > - efi_status_t status; > > - > > - size = sizeof(secboot); > > - status = get_efi_var(efi_SecureBoot_name, &efi_variable_guid, > > - NULL, &size, &secboot); > > - if (status == EFI_NOT_FOUND) > > - return efi_secureboot_mode_disabled; > > - if (status != EFI_SUCCESS) > > - goto out_efi_err; > > - > > - size = sizeof(setupmode); > > - status = get_efi_var(efi_SetupMode_name, &efi_variable_guid, > > - NULL, &size, &setupmode); > > - if (status != EFI_SUCCESS) > > - goto out_efi_err; > > - > > - if (secboot == 0 || setupmode == 1) > > - return efi_secureboot_mode_disabled; > > - > > - /* > > - * See if a user has put the shim into insecure mode. If so, and if the > > - * variable doesn't have the runtime attribute set, we might as well > > - * honor that. > > - */ > > - size = sizeof(moksbstate); > > - status = get_efi_var(shim_MokSBState_name, &shim_guid, > > - &attr, &size, &moksbstate); > > - > > MokSBState is a boot time variable, so we cannot access it when > running under the OS. Xen also has a code flow similar to this one, > but it looks at MokSbStateRt instead (which may be a mistake but let's > forget about that for now) > > So what we will need to do is factor out only the top part of this > function (which, incidentally, is the only part that IMA uses in the i> first place) > Thanks for the reminder. I will take this change into next revision. > > - /* If it fails, we don't care why. Default to secure */ > > - if (status != EFI_SUCCESS) > > - goto secure_boot_enabled; > > - if (!(attr & EFI_VARIABLE_RUNTIME_ACCESS) && moksbstate == 1) > > - return efi_secureboot_mode_disabled; > > - > > -secure_boot_enabled: > > - efi_info("UEFI Secure Boot is enabled.\n"); > > - return efi_secureboot_mode_enabled; > > - > > -out_efi_err: > > - efi_err("Could not determine UEFI Secure Boot status.\n"); > > - return efi_secureboot_mode_unknown; > > -} > > So let's keep this file, and also, let's put a wrapper function around > get_efi_var() here, of which you can take the address and pass to the > static inline function. If I understand correctly, that means it's better to define a new wrapper function around the get_efi_var() rather than changing it from a macro to an inline function. Please feel free to let me know if I misunderstand it. > > > diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c > > index 3672539cb96e..3f9b492c566b 100644 > > --- a/drivers/firmware/efi/libstub/x86-stub.c > > +++ b/drivers/firmware/efi/libstub/x86-stub.c > > @@ -781,7 +781,7 @@ unsigned long efi_main(efi_handle_t handle, > > * otherwise we ask the BIOS. > > */ > > if (boot_params->secure_boot == efi_secureboot_mode_unset) > > - boot_params->secure_boot = efi_get_secureboot(); > > + boot_params->secure_boot = efi_get_secureboot(get_efi_var); > > > > /* Ask the firmware to clear memory on unclean shutdown */ > > efi_enable_reset_attack_mitigation(); > > diff --git a/include/linux/efi.h b/include/linux/efi.h > > index d7c0e73af2b9..cc2d3de39031 100644 > > --- a/include/linux/efi.h > > +++ b/include/linux/efi.h > > @@ -1089,7 +1089,46 @@ enum efi_secureboot_mode { > > efi_secureboot_mode_disabled, > > efi_secureboot_mode_enabled, > > }; > > -enum efi_secureboot_mode efi_get_secureboot(void); > > + > > +static inline enum efi_secureboot_mode efi_get_secureboot(efi_get_variable_t *get_var) > > +{ > > + efi_guid_t var_guid = EFI_GLOBAL_VARIABLE_GUID; > > + efi_guid_t shim_guid = EFI_SHIM_LOCK_GUID; > > + efi_status_t status; > > + unsigned long size; > > + u8 secboot, setupmode, moksbstate; > > + u32 attr; > > + > > + size = sizeof(secboot); > > + status = get_var(L"SecureBoot", &var_guid, NULL, &size, &secboot); > > + > > + if (status == EFI_NOT_FOUND) > > + return efi_secureboot_mode_disabled; > > + if (status != EFI_SUCCESS) > > + return efi_secureboot_mode_unknown; > > + > > + size = sizeof(setupmode); > > + status = get_var(L"SetupMode", &var_guid, NULL, &size, &setupmode); > > + > > + if (status != EFI_SUCCESS) > > + return efi_secureboot_mode_unknown; > > + if (secboot == 0 || setupmode == 1) > > + return efi_secureboot_mode_disabled; > > + > > So keep until here and move the rest back into the .c file > > > + /* > > + * See if a user has put the shim into insecure mode. If so, and if the > > + * variable doesn't have the runtime attribute set, we might as well > > + * honor that. > > + */ > > + size = sizeof(moksbstate); > > + status = get_var(L"MokSBState", &shim_guid, &attr, &size, &moksbstate); > > + /* If it fails, we don't care why. Default to secure */ > > + if (status == EFI_SUCCESS && moksbstate == 1 > > + && !(attr & EFI_VARIABLE_RUNTIME_ACCESS)) > > + return efi_secureboot_mode_disabled; > > + > > + return efi_secureboot_mode_enabled; > > +} > > > > #ifdef CONFIG_RESET_ATTACK_MITIGATION > > void efi_enable_reset_attack_mitigation(void); > > -- > > 2.28.0 > > > ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v3 2/3] ima: replace arch-specific get_sb_mode() with a common helper ima_get_efi_secureboot() 2020-10-30 6:08 [PATCH v3 0/3] add ima_arch support for ARM64 Chester Lin 2020-10-30 6:08 ` [PATCH v3 1/3] efi: generalize efi_get_secureboot Chester Lin @ 2020-10-30 6:08 ` Chester Lin 2020-10-30 11:52 ` Ard Biesheuvel 2020-10-30 6:08 ` [PATCH v3 3/3] arm64/ima: add ima_arch support Chester Lin 2 siblings, 1 reply; 10+ messages in thread From: Chester Lin @ 2020-10-30 6:08 UTC (permalink / raw) To: ardb, zohar, jmorris, serge, dmitry.kasatkin, catalin.marinas, will, tglx, mingo, bp, hpa Cc: linux-kernel, linux-arm-kernel, linux-efi, linux-integrity, linux-security-module, x86, jlee, clin remove the get_sb_mode() from x86/kernel/ima_arch.c and create a common helper ima_get_efi_secureboot() in IMA so that all EFI-based architectures can refer to the same procedure. Signed-off-by: Chester Lin <clin@suse.com> --- arch/x86/kernel/ima_arch.c | 69 +++++++------------------------- include/linux/ima.h | 10 +++++ security/integrity/ima/Makefile | 1 + security/integrity/ima/ima_efi.c | 26 ++++++++++++ 4 files changed, 51 insertions(+), 55 deletions(-) create mode 100644 security/integrity/ima/ima_efi.c diff --git a/arch/x86/kernel/ima_arch.c b/arch/x86/kernel/ima_arch.c index 7dfb1e808928..2c773532ff0a 100644 --- a/arch/x86/kernel/ima_arch.c +++ b/arch/x86/kernel/ima_arch.c @@ -8,69 +8,28 @@ 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; - static bool initialized; - - if (!initialized && efi_enabled(EFI_BOOT)) { - sb_mode = boot_params.secure_boot; + static bool sb_enabled, initialized; - if (sb_mode == efi_secureboot_mode_unset) - sb_mode = get_sb_mode(); + if (initialized) { + return sb_enabled; + } else if (efi_enabled(EFI_BOOT)) { initialized = true; + + if (boot_params.secure_boot == efi_secureboot_mode_unset) { + sb_enabled = ima_get_efi_secureboot(); + return sb_enabled; + } } - if (sb_mode == efi_secureboot_mode_enabled) - return true; - else - return false; + if (boot_params.secure_boot == efi_secureboot_mode_enabled) + sb_enabled = true; + + return sb_enabled; } -/* 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/include/linux/ima.h b/include/linux/ima.h index 8fa7bcfb2da2..9f9699f017be 100644 --- a/include/linux/ima.h +++ b/include/linux/ima.h @@ -50,6 +50,16 @@ static inline const char * const *arch_get_ima_policy(void) } #endif +#if defined(CONFIG_EFI) && defined(CONFIG_IMA_SECURE_AND_OR_TRUSTED_BOOT) +extern bool ima_get_efi_secureboot(void); +#else +static inline bool ima_get_efi_secureboot(void) +{ + return false; +} +#endif + + #else static inline int ima_bprm_check(struct linux_binprm *bprm) { diff --git a/security/integrity/ima/Makefile b/security/integrity/ima/Makefile index 67dabca670e2..32076b3fd292 100644 --- a/security/integrity/ima/Makefile +++ b/security/integrity/ima/Makefile @@ -14,3 +14,4 @@ ima-$(CONFIG_HAVE_IMA_KEXEC) += ima_kexec.o ima-$(CONFIG_IMA_BLACKLIST_KEYRING) += ima_mok.o ima-$(CONFIG_IMA_MEASURE_ASYMMETRIC_KEYS) += ima_asymmetric_keys.o ima-$(CONFIG_IMA_QUEUE_EARLY_BOOT_KEYS) += ima_queue_keys.o +ima-$(CONFIG_EFI) += ima_efi.o diff --git a/security/integrity/ima/ima_efi.c b/security/integrity/ima/ima_efi.c new file mode 100644 index 000000000000..a78f66e19689 --- /dev/null +++ b/security/integrity/ima/ima_efi.c @@ -0,0 +1,26 @@ +// SPDX-License-Identifier: GPL-2.0-only +/* + * Copyright (C) 2020 SUSE LLC + * + * Author: + * Chester Lin <clin@suse.com> + */ + +#include <linux/efi.h> +#include <linux/ima.h> + +#ifdef CONFIG_IMA_SECURE_AND_OR_TRUSTED_BOOT +bool ima_get_efi_secureboot(void) +{ + enum efi_secureboot_mode sb_mode; + + if (!efi_rt_services_supported(EFI_RT_SUPPORTED_GET_VARIABLE)) { + pr_info("ima: secureboot mode unknown, no efi\n"); + return false; + } + + sb_mode = efi_get_secureboot(efi.get_variable); + + return (sb_mode == efi_secureboot_mode_enabled) ? true : false; +} +#endif -- 2.28.0 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v3 2/3] ima: replace arch-specific get_sb_mode() with a common helper ima_get_efi_secureboot() 2020-10-30 6:08 ` [PATCH v3 2/3] ima: replace arch-specific get_sb_mode() with a common helper ima_get_efi_secureboot() Chester Lin @ 2020-10-30 11:52 ` Ard Biesheuvel 0 siblings, 0 replies; 10+ messages in thread From: Ard Biesheuvel @ 2020-10-30 11:52 UTC (permalink / raw) To: Chester Lin Cc: Mimi Zohar, James Morris, Serge E. Hallyn, Dmitry Kasatkin, Catalin Marinas, Will Deacon, Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin, Linux Kernel Mailing List, Linux ARM, linux-efi, linux-integrity, linux-security-module, X86 ML, Lee, Chun-Yi On Fri, 30 Oct 2020 at 07:09, Chester Lin <clin@suse.com> wrote: > > remove the get_sb_mode() from x86/kernel/ima_arch.c and create a common > helper ima_get_efi_secureboot() in IMA so that all EFI-based architectures > can refer to the same procedure. > > Signed-off-by: Chester Lin <clin@suse.com> > --- > arch/x86/kernel/ima_arch.c | 69 +++++++------------------------- > include/linux/ima.h | 10 +++++ > security/integrity/ima/Makefile | 1 + > security/integrity/ima/ima_efi.c | 26 ++++++++++++ > 4 files changed, 51 insertions(+), 55 deletions(-) > create mode 100644 security/integrity/ima/ima_efi.c > > diff --git a/arch/x86/kernel/ima_arch.c b/arch/x86/kernel/ima_arch.c > index 7dfb1e808928..2c773532ff0a 100644 > --- a/arch/x86/kernel/ima_arch.c > +++ b/arch/x86/kernel/ima_arch.c > @@ -8,69 +8,28 @@ > > 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; > - static bool initialized; > - > - if (!initialized && efi_enabled(EFI_BOOT)) { > - sb_mode = boot_params.secure_boot; > + static bool sb_enabled, initialized; > > - if (sb_mode == efi_secureboot_mode_unset) > - sb_mode = get_sb_mode(); > + if (initialized) { > + return sb_enabled; > + } else if (efi_enabled(EFI_BOOT)) { > initialized = true; > + > + if (boot_params.secure_boot == efi_secureboot_mode_unset) { > + sb_enabled = ima_get_efi_secureboot(); > + return sb_enabled; > + } > } > > - if (sb_mode == efi_secureboot_mode_enabled) > - return true; > - else > - return false; > + if (boot_params.secure_boot == efi_secureboot_mode_enabled) > + sb_enabled = true; > + > + return sb_enabled; > } > > -/* 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/include/linux/ima.h b/include/linux/ima.h > index 8fa7bcfb2da2..9f9699f017be 100644 > --- a/include/linux/ima.h > +++ b/include/linux/ima.h > @@ -50,6 +50,16 @@ static inline const char * const *arch_get_ima_policy(void) > } > #endif > > +#if defined(CONFIG_EFI) && defined(CONFIG_IMA_SECURE_AND_OR_TRUSTED_BOOT) > +extern bool ima_get_efi_secureboot(void); > +#else > +static inline bool ima_get_efi_secureboot(void) > +{ > + return false; > +} > +#endif > + > + > #else > static inline int ima_bprm_check(struct linux_binprm *bprm) > { > diff --git a/security/integrity/ima/Makefile b/security/integrity/ima/Makefile > index 67dabca670e2..32076b3fd292 100644 > --- a/security/integrity/ima/Makefile > +++ b/security/integrity/ima/Makefile > @@ -14,3 +14,4 @@ ima-$(CONFIG_HAVE_IMA_KEXEC) += ima_kexec.o > ima-$(CONFIG_IMA_BLACKLIST_KEYRING) += ima_mok.o > ima-$(CONFIG_IMA_MEASURE_ASYMMETRIC_KEYS) += ima_asymmetric_keys.o > ima-$(CONFIG_IMA_QUEUE_EARLY_BOOT_KEYS) += ima_queue_keys.o > +ima-$(CONFIG_EFI) += ima_efi.o > diff --git a/security/integrity/ima/ima_efi.c b/security/integrity/ima/ima_efi.c > new file mode 100644 > index 000000000000..a78f66e19689 > --- /dev/null > +++ b/security/integrity/ima/ima_efi.c > @@ -0,0 +1,26 @@ > +// SPDX-License-Identifier: GPL-2.0-only > +/* > + * Copyright (C) 2020 SUSE LLC > + * > + * Author: > + * Chester Lin <clin@suse.com> > + */ > + > +#include <linux/efi.h> > +#include <linux/ima.h> > + > +#ifdef CONFIG_IMA_SECURE_AND_OR_TRUSTED_BOOT > +bool ima_get_efi_secureboot(void) > +{ > + enum efi_secureboot_mode sb_mode; > + > + if (!efi_rt_services_supported(EFI_RT_SUPPORTED_GET_VARIABLE)) { > + pr_info("ima: secureboot mode unknown, no efi\n"); > + return false; > + } > + > + sb_mode = efi_get_secureboot(efi.get_variable); > + As I mentioned in the other patch, these are not equivalent - you are introducing a MokSbState check which doesn't make sense at runtime (or at all perhaps) > + return (sb_mode == efi_secureboot_mode_enabled) ? true : false; > +} > +#endif > -- > 2.28.0 > ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v3 3/3] arm64/ima: add ima_arch support 2020-10-30 6:08 [PATCH v3 0/3] add ima_arch support for ARM64 Chester Lin 2020-10-30 6:08 ` [PATCH v3 1/3] efi: generalize efi_get_secureboot Chester Lin 2020-10-30 6:08 ` [PATCH v3 2/3] ima: replace arch-specific get_sb_mode() with a common helper ima_get_efi_secureboot() Chester Lin @ 2020-10-30 6:08 ` Chester Lin 2020-10-30 11:53 ` Ard Biesheuvel 2 siblings, 1 reply; 10+ messages in thread From: Chester Lin @ 2020-10-30 6:08 UTC (permalink / raw) To: ardb, zohar, jmorris, serge, dmitry.kasatkin, catalin.marinas, will, tglx, mingo, bp, hpa Cc: linux-kernel, linux-arm-kernel, linux-efi, linux-integrity, linux-security-module, x86, 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 | 43 ++++++++++++++++++++++++++++++++++++ 3 files changed, 46 insertions(+) create mode 100644 arch/arm64/kernel/ima_arch.c diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig index a42e8d13cc88..496a4a26afc6 100644 --- a/arch/arm64/Kconfig +++ b/arch/arm64/Kconfig @@ -201,6 +201,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..564236d77adc --- /dev/null +++ b/arch/arm64/kernel/ima_arch.c @@ -0,0 +1,43 @@ +// 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 bool sb_enabled; + static bool initialized; + + if (!initialized & efi_enabled(EFI_BOOT)) { + sb_enabled = ima_get_efi_secureboot(); + initialized = true; + } + + return sb_enabled; +} + +/* 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.28.0 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v3 3/3] arm64/ima: add ima_arch support 2020-10-30 6:08 ` [PATCH v3 3/3] arm64/ima: add ima_arch support Chester Lin @ 2020-10-30 11:53 ` Ard Biesheuvel 2020-11-02 7:20 ` Chester Lin 0 siblings, 1 reply; 10+ messages in thread From: Ard Biesheuvel @ 2020-10-30 11:53 UTC (permalink / raw) To: Chester Lin Cc: Mimi Zohar, James Morris, Serge E. Hallyn, Dmitry Kasatkin, Catalin Marinas, Will Deacon, Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin, Linux Kernel Mailing List, Linux ARM, linux-efi, linux-integrity, linux-security-module, X86 ML, Lee, Chun-Yi On Fri, 30 Oct 2020 at 07:09, Chester Lin <clin@suse.com> wrote: > > 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 | 43 ++++++++++++++++++++++++++++++++++++ > 3 files changed, 46 insertions(+) > create mode 100644 arch/arm64/kernel/ima_arch.c > > diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig > index a42e8d13cc88..496a4a26afc6 100644 > --- a/arch/arm64/Kconfig > +++ b/arch/arm64/Kconfig > @@ -201,6 +201,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..564236d77adc > --- /dev/null > +++ b/arch/arm64/kernel/ima_arch.c > @@ -0,0 +1,43 @@ > +// 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 bool sb_enabled; > + static bool initialized; > + > + if (!initialized & efi_enabled(EFI_BOOT)) { > + sb_enabled = ima_get_efi_secureboot(); > + initialized = true; > + } > + > + return sb_enabled; > +} > + > +/* 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.28.0 > Can we move all this stuff into security/integrity/ima/ima_efi.c instead? ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3 3/3] arm64/ima: add ima_arch support 2020-10-30 11:53 ` Ard Biesheuvel @ 2020-11-02 7:20 ` Chester Lin 2020-11-02 12:13 ` Mimi Zohar 0 siblings, 1 reply; 10+ messages in thread From: Chester Lin @ 2020-11-02 7:20 UTC (permalink / raw) To: Ard Biesheuvel Cc: Mimi Zohar, James Morris, Serge E. Hallyn, Dmitry Kasatkin, Catalin Marinas, Will Deacon, Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin, Linux Kernel Mailing List, Linux ARM, linux-efi, linux-integrity, linux-security-module, X86 ML, Lee, Chun-Yi On Fri, Oct 30, 2020 at 12:53:25PM +0100, Ard Biesheuvel wrote: > On Fri, 30 Oct 2020 at 07:09, Chester Lin <clin@suse.com> wrote: > > > > 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 | 43 ++++++++++++++++++++++++++++++++++++ > > 3 files changed, 46 insertions(+) > > create mode 100644 arch/arm64/kernel/ima_arch.c > > > > diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig > > index a42e8d13cc88..496a4a26afc6 100644 > > --- a/arch/arm64/Kconfig > > +++ b/arch/arm64/Kconfig > > @@ -201,6 +201,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..564236d77adc > > --- /dev/null > > +++ b/arch/arm64/kernel/ima_arch.c > > @@ -0,0 +1,43 @@ > > +// 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 bool sb_enabled; > > + static bool initialized; > > + > > + if (!initialized & efi_enabled(EFI_BOOT)) { > > + sb_enabled = ima_get_efi_secureboot(); > > + initialized = true; > > + } > > + > > + return sb_enabled; > > +} > > + > > +/* 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.28.0 > > > > Can we move all this stuff into security/integrity/ima/ima_efi.c instead? > Actually I hesitated to move all this stuff into ima_efi.c when coding v3 because I haven't figured out a clear picture to achieve it. Since each architecture could still have different details to trigger secure boot detection and define their arch-specific rules [e.g. Having boot_params in x86_64 creates more conditions that need to be determined before calling get_sb_mode()], moving all this stuff seems to decrease the flexibility. Besides, it might also affect the consistency of ima_arch as well, for example, ppc and s390 still use these function prototypes defined in ima.h. Since these functions are already referred by non-EFI architectures, why don't we still reuse these prototypes? For example, we could remain a smaller arch_ima_get_secureboot() and the arch-specific rules but move the major part of arch_get_ima_policy() into ima_efi.c. For example, we could implement an efi_ima_policy() for arch_get_ima_policy() to call so that the arch_get_ima_policy() doesn't have to know some details such as checking conditions or calling set_module_sig_enforced(). Please feel free to let me know if any suggestions. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH v3 3/3] arm64/ima: add ima_arch support 2020-11-02 7:20 ` Chester Lin @ 2020-11-02 12:13 ` Mimi Zohar 0 siblings, 0 replies; 10+ messages in thread From: Mimi Zohar @ 2020-11-02 12:13 UTC (permalink / raw) To: Chester Lin, Ard Biesheuvel Cc: James Morris, Serge E. Hallyn, Dmitry Kasatkin, Catalin Marinas, Will Deacon, Thomas Gleixner, Ingo Molnar, Borislav Petkov, H. Peter Anvin, Linux Kernel Mailing List, Linux ARM, linux-efi, linux-integrity, linux-security-module, X86 ML, Lee, Chun-Yi On Mon, 2020-11-02 at 15:20 +0800, Chester Lin wrote: > On Fri, Oct 30, 2020 at 12:53:25PM +0100, Ard Biesheuvel wrote: > > On Fri, 30 Oct 2020 at 07:09, Chester Lin <clin@suse.com> wrote: > > > > > > 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 | 43 ++++++++++++++++++++++++++++++++++++ > > > 3 files changed, 46 insertions(+) > > > create mode 100644 arch/arm64/kernel/ima_arch.c > > > > > > diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig > > > index a42e8d13cc88..496a4a26afc6 100644 > > > --- a/arch/arm64/Kconfig > > > +++ b/arch/arm64/Kconfig > > > @@ -201,6 +201,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..564236d77adc > > > --- /dev/null > > > +++ b/arch/arm64/kernel/ima_arch.c > > > @@ -0,0 +1,43 @@ > > > +// 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 bool sb_enabled; > > > + static bool initialized; > > > + > > > + if (!initialized & efi_enabled(EFI_BOOT)) { > > > + sb_enabled = ima_get_efi_secureboot(); > > > + initialized = true; > > > + } > > > + > > > + return sb_enabled; > > > +} > > > + > > > +/* 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.28.0 > > > > > > > Can we move all this stuff into security/integrity/ima/ima_efi.c instead? > > > Actually I hesitated to move all this stuff into ima_efi.c when coding v3 > because I haven't figured out a clear picture to achieve it. Since each > architecture could still have different details to trigger secure boot detection > and define their arch-specific rules [e.g. Having boot_params in x86_64 creates > more conditions that need to be determined before calling get_sb_mode()], moving > all this stuff seems to decrease the flexibility. Besides, it might also affect > the consistency of ima_arch as well, for example, ppc and s390 still use these > function prototypes defined in ima.h. Since these functions are already referred > by non-EFI architectures, why don't we still reuse these prototypes? For example, > we could remain a smaller arch_ima_get_secureboot() and the arch-specific rules > but move the major part of arch_get_ima_policy() into ima_efi.c. For example, > we could implement an efi_ima_policy() for arch_get_ima_policy() to call so that > the arch_get_ima_policy() doesn't have to know some details such as checking > conditions or calling set_module_sig_enforced(). > > Please feel free to let me know if any suggestions. Yes, that is the point and the reason for defining ima_efi.c and conditionally including it only for EFI systems. The existing ppc and s390 code should remain unaffected by this change. thanks, Mimi ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-11-02 12:14 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-10-30 6:08 [PATCH v3 0/3] add ima_arch support for ARM64 Chester Lin 2020-10-30 6:08 ` [PATCH v3 1/3] efi: generalize efi_get_secureboot Chester Lin 2020-10-30 11:51 ` Ard Biesheuvel 2020-11-02 5:30 ` Chester Lin 2020-10-30 6:08 ` [PATCH v3 2/3] ima: replace arch-specific get_sb_mode() with a common helper ima_get_efi_secureboot() Chester Lin 2020-10-30 11:52 ` Ard Biesheuvel 2020-10-30 6:08 ` [PATCH v3 3/3] arm64/ima: add ima_arch support Chester Lin 2020-10-30 11:53 ` Ard Biesheuvel 2020-11-02 7:20 ` Chester Lin 2020-11-02 12:13 ` Mimi Zohar
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).