* [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
* [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
* 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
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).