[PATCH v2] integrity: Extract secure boot enquiry function out of IMA

[PATCH v2] integrity: Extract secure boot enquiry function out of IMA

[GONG Ruiqi]

Commit 92ad19559ea9 ("integrity: Do not load MOK and MOKx when secure
boot be disabled") utilizes arch_ima_get_secureboot() to perform a
secure boot status check before loading the Machine Owner Key (MOK).
However, only when CONFIG_IMA_SECURE_AND_OR_TRUSTED_BOOT=y can this
function be functional, while this config has nothing to do with secure
boot or MOK loading.

Given that arch_ima_get_secureboot() is just a helper to retrieve info
about secure boot via EFI and doesn't necessarily be a part of IMA,
rename it to arch_integrity_get_secureboot(), decouple its functionality
from IMA and extract it to be a integrity subsystem helper, so that both
certificate loading and IMA can make use of it.

Compile-tested on powerpc, s390 and x86, with CONFIG_IMA_ARCH_POLICY=n
and =y based on defconfig and allmodconfig.

Signed-off-by: GONG Ruiqi <gongruiqi1@huawei.com>
---

v2:
- Fix compile errors for CONFIG_IMA_ARCH_POLICY=n on s390 & powerpc

 arch/powerpc/kernel/Makefile                  |  3 +-
 arch/powerpc/kernel/ima_arch.c                |  5 +-
 arch/s390/kernel/Makefile                     |  2 +-
 arch/s390/kernel/ima_arch.c                   |  5 +-
 include/linux/ima.h                           |  6 ---
 include/linux/integrity.h                     |  1 +
 security/integrity/Makefile                   |  3 +-
 security/integrity/ima/Makefile               |  2 +-
 security/integrity/ima/ima_appraise.c         |  2 +-
 security/integrity/ima/ima_efi.c              | 47 +-----------------
 security/integrity/ima/ima_main.c             |  2 +-
 security/integrity/platform_certs/load_uefi.c |  2 +-
 security/integrity/secureboot.c               | 48 +++++++++++++++++++
 13 files changed, 68 insertions(+), 60 deletions(-)
 create mode 100644 security/integrity/secureboot.c

diff --git a/arch/powerpc/kernel/Makefile b/arch/powerpc/kernel/Makefile
index fb2b95267022..4d5e3c9dde93 100644
--- a/arch/powerpc/kernel/Makefile
+++ b/arch/powerpc/kernel/Makefile
@@ -168,7 +168,8 @@ ifneq ($(CONFIG_PPC_POWERNV)$(CONFIG_PPC_SVM),)
 obj-y				+= ucall.o
 endif
 
-obj-$(CONFIG_PPC_SECURE_BOOT)	+= secure_boot.o ima_arch.o secvar-ops.o
+obj-$(CONFIG_IMA)		+= ima_arch.o
+obj-$(CONFIG_PPC_SECURE_BOOT)	+= secure_boot.o secvar-ops.o
 obj-$(CONFIG_PPC_SECVAR_SYSFS)	+= secvar-sysfs.o
 
 # Disable GCOV, KCOV & sanitizers in odd or sensitive code
diff --git a/arch/powerpc/kernel/ima_arch.c b/arch/powerpc/kernel/ima_arch.c
index b7029beed847..2cb248a88eeb 100644
--- a/arch/powerpc/kernel/ima_arch.c
+++ b/arch/powerpc/kernel/ima_arch.c
@@ -5,9 +5,10 @@
  */
 
 #include <linux/ima.h>
+#include <linux/integrity.h>
 #include <asm/secure_boot.h>
 
-bool arch_ima_get_secureboot(void)
+bool arch_integrity_get_secureboot(void)
 {
 	return is_ppc_secureboot_enabled();
 }
@@ -56,6 +57,7 @@ static const char *const secure_and_trusted_rules[] = {
 	NULL
 };
 
+#ifdef CONFIG_IMA_SECURE_AND_OR_TRUSTED_BOOT
 /*
  * Returns the relevant IMA arch-specific policies based on the system secure
  * boot state.
@@ -76,3 +78,4 @@ const char *const *arch_get_ima_policy(void)
 
 	return NULL;
 }
+#endif
diff --git a/arch/s390/kernel/Makefile b/arch/s390/kernel/Makefile
index ea5ed6654050..961943cbf283 100644
--- a/arch/s390/kernel/Makefile
+++ b/arch/s390/kernel/Makefile
@@ -74,7 +74,7 @@ obj-$(CONFIG_JUMP_LABEL)	+= jump_label.o
 obj-$(CONFIG_KEXEC_FILE)	+= machine_kexec_file.o kexec_image.o
 obj-$(CONFIG_KEXEC_FILE)	+= kexec_elf.o
 obj-$(CONFIG_CERT_STORE)	+= cert_store.o
-obj-$(CONFIG_IMA_SECURE_AND_OR_TRUSTED_BOOT)	+= ima_arch.o
+obj-$(CONFIG_IMA)		+= ima_arch.o
 
 obj-$(CONFIG_PERF_EVENTS)	+= perf_event.o
 obj-$(CONFIG_PERF_EVENTS)	+= perf_cpum_cf.o perf_cpum_sf.o
diff --git a/arch/s390/kernel/ima_arch.c b/arch/s390/kernel/ima_arch.c
index f3c3e6e1c5d3..a69199afb286 100644
--- a/arch/s390/kernel/ima_arch.c
+++ b/arch/s390/kernel/ima_arch.c
@@ -1,14 +1,17 @@
 // SPDX-License-Identifier: GPL-2.0
 
 #include <linux/ima.h>
+#include <linux/integrity.h>
 #include <asm/boot_data.h>
 
-bool arch_ima_get_secureboot(void)
+bool arch_integrity_get_secureboot(void)
 {
 	return ipl_secure_flag;
 }
 
+#ifdef CONFIG_IMA_SECURE_AND_OR_TRUSTED_BOOT
 const char * const *arch_get_ima_policy(void)
 {
 	return NULL;
 }
+#endif
diff --git a/include/linux/ima.h b/include/linux/ima.h
index 8e29cb4e6a01..9faf3b964314 100644
--- a/include/linux/ima.h
+++ b/include/linux/ima.h
@@ -72,14 +72,8 @@ int __init ima_get_kexec_buffer(void **addr, size_t *size);
 #endif
 
 #ifdef CONFIG_IMA_SECURE_AND_OR_TRUSTED_BOOT
-extern bool arch_ima_get_secureboot(void);
 extern const char * const *arch_get_ima_policy(void);
 #else
-static inline bool arch_ima_get_secureboot(void)
-{
-	return false;
-}
-
 static inline const char * const *arch_get_ima_policy(void)
 {
 	return NULL;
diff --git a/include/linux/integrity.h b/include/linux/integrity.h
index f5842372359b..4bc81fe4253e 100644
--- a/include/linux/integrity.h
+++ b/include/linux/integrity.h
@@ -61,5 +61,6 @@ integrity_inode_attrs_changed(const struct integrity_inode_attributes *attrs,
 		!inode_eq_iversion(inode, attrs->version));
 }
 
+extern bool arch_integrity_get_secureboot(void);
 
 #endif /* _LINUX_INTEGRITY_H */
diff --git a/security/integrity/Makefile b/security/integrity/Makefile
index 92b63039c654..0770c6554a8f 100644
--- a/security/integrity/Makefile
+++ b/security/integrity/Makefile
@@ -11,7 +11,8 @@ integrity-$(CONFIG_INTEGRITY_SIGNATURE) += digsig.o
 integrity-$(CONFIG_INTEGRITY_ASYMMETRIC_KEYS) += digsig_asymmetric.o
 integrity-$(CONFIG_INTEGRITY_PLATFORM_KEYRING) += platform_certs/platform_keyring.o
 integrity-$(CONFIG_INTEGRITY_MACHINE_KEYRING) += platform_certs/machine_keyring.o
-integrity-$(CONFIG_LOAD_UEFI_KEYS) += platform_certs/efi_parser.o \
+integrity-$(CONFIG_LOAD_UEFI_KEYS) += secureboot.o \
+				      platform_certs/efi_parser.o \
 				      platform_certs/load_uefi.o \
 				      platform_certs/keyring_handler.o
 integrity-$(CONFIG_LOAD_IPL_KEYS) += platform_certs/load_ipl_s390.o
diff --git a/security/integrity/ima/Makefile b/security/integrity/ima/Makefile
index b376d38b4ee6..f81be17e25a8 100644
--- a/security/integrity/ima/Makefile
+++ b/security/integrity/ima/Makefile
@@ -16,5 +16,5 @@ ima-$(CONFIG_IMA_MEASURE_ASYMMETRIC_KEYS) += ima_asymmetric_keys.o
 ima-$(CONFIG_IMA_QUEUE_EARLY_BOOT_KEYS) += ima_queue_keys.o
 
 ifeq ($(CONFIG_EFI),y)
-ima-$(CONFIG_IMA_SECURE_AND_OR_TRUSTED_BOOT) += ima_efi.o
+ima-$(CONFIG_IMA_SECURE_AND_OR_TRUSTED_BOOT) += ima_efi.o ../secureboot.o
 endif
diff --git a/security/integrity/ima/ima_appraise.c b/security/integrity/ima/ima_appraise.c
index f435eff4667f..41bece645348 100644
--- a/security/integrity/ima/ima_appraise.c
+++ b/security/integrity/ima/ima_appraise.c
@@ -27,7 +27,7 @@ core_param(ima_appraise, ima_appraise_cmdline_default, charp, 0);
 void __init ima_appraise_parse_cmdline(void)
 {
 	const char *str = ima_appraise_cmdline_default;
-	bool sb_state = arch_ima_get_secureboot();
+	bool sb_state = arch_integrity_get_secureboot();
 	int appraisal_state = ima_appraise;
 
 	if (!str)
diff --git a/security/integrity/ima/ima_efi.c b/security/integrity/ima/ima_efi.c
index 138029bfcce1..fcbc0727469e 100644
--- a/security/integrity/ima/ima_efi.c
+++ b/security/integrity/ima/ima_efi.c
@@ -2,52 +2,9 @@
 /*
  * Copyright (C) 2018 IBM Corporation
  */
-#include <linux/efi.h>
 #include <linux/module.h>
 #include <linux/ima.h>
-#include <asm/efi.h>
-
-#ifndef arch_ima_efi_boot_mode
-#define arch_ima_efi_boot_mode efi_secureboot_mode_unset
-#endif
-
-static enum efi_secureboot_mode get_sb_mode(void)
-{
-	enum efi_secureboot_mode mode;
-
-	if (!efi_rt_services_supported(EFI_RT_SUPPORTED_GET_VARIABLE)) {
-		pr_info("ima: secureboot mode unknown, no efi\n");
-		return efi_secureboot_mode_unknown;
-	}
-
-	mode = efi_get_secureboot_mode(efi.get_variable);
-	if (mode == efi_secureboot_mode_disabled)
-		pr_info("ima: secureboot mode disabled\n");
-	else if (mode == efi_secureboot_mode_unknown)
-		pr_info("ima: secureboot mode unknown\n");
-	else
-		pr_info("ima: secureboot mode enabled\n");
-	return mode;
-}
-
-bool arch_ima_get_secureboot(void)
-{
-	static enum efi_secureboot_mode sb_mode;
-	static bool initialized;
-
-	if (!initialized && efi_enabled(EFI_BOOT)) {
-		sb_mode = arch_ima_efi_boot_mode;
-
-		if (sb_mode == efi_secureboot_mode_unset)
-			sb_mode = get_sb_mode();
-		initialized = true;
-	}
-
-	if (sb_mode == efi_secureboot_mode_enabled)
-		return true;
-	else
-		return false;
-}
+#include <linux/integrity.h>
 
 /* secureboot arch rules */
 static const char * const sb_arch_rules[] = {
@@ -67,7 +24,7 @@ static const char * const sb_arch_rules[] = {
 
 const char * const *arch_get_ima_policy(void)
 {
-	if (IS_ENABLED(CONFIG_IMA_ARCH_POLICY) && arch_ima_get_secureboot()) {
+	if (IS_ENABLED(CONFIG_IMA_ARCH_POLICY) && arch_integrity_get_secureboot()) {
 		if (IS_ENABLED(CONFIG_MODULE_SIG))
 			set_module_sig_enforced();
 		if (IS_ENABLED(CONFIG_KEXEC_SIG))
diff --git a/security/integrity/ima/ima_main.c b/security/integrity/ima/ima_main.c
index f99ab1a3b0f0..9974d89f3eca 100644
--- a/security/integrity/ima/ima_main.c
+++ b/security/integrity/ima/ima_main.c
@@ -899,7 +899,7 @@ static int ima_load_data(enum kernel_load_data_id id, bool contents)
 	switch (id) {
 	case LOADING_KEXEC_IMAGE:
 		if (IS_ENABLED(CONFIG_KEXEC_SIG)
-		    && arch_ima_get_secureboot()) {
+		    && arch_integrity_get_secureboot()) {
 			pr_err("impossible to appraise a kernel image without a file descriptor; try using kexec_file_load syscall.\n");
 			return -EACCES;
 		}
diff --git a/security/integrity/platform_certs/load_uefi.c b/security/integrity/platform_certs/load_uefi.c
index d1fdd113450a..3042a0c536d6 100644
--- a/security/integrity/platform_certs/load_uefi.c
+++ b/security/integrity/platform_certs/load_uefi.c
@@ -212,7 +212,7 @@ static int __init load_uefi_certs(void)
 	}
 
 	/* the MOK/MOKx can not be trusted when secure boot is disabled */
-	if (!arch_ima_get_secureboot())
+	if (!arch_integrity_get_secureboot())
 		return 0;
 
 	mokx = get_cert_list(L"MokListXRT", &mok_var, &mokxsize, &status);
diff --git a/security/integrity/secureboot.c b/security/integrity/secureboot.c
new file mode 100644
index 000000000000..5c50f8be6053
--- /dev/null
+++ b/security/integrity/secureboot.c
@@ -0,0 +1,48 @@
+// SPDX-License-Identifier: GPL-2.0+
+/*
+ * Copyright (C) 2025 Huawei Technologies Co., Ltd
+ */
+#include <linux/module.h>
+#include <linux/efi.h>
+#include <linux/integrity.h>
+
+#include <asm/efi.h>
+
+#ifndef arch_integrity_efi_boot_mode
+#define arch_integrity_efi_boot_mode efi_secureboot_mode_unset
+#endif
+
+static enum efi_secureboot_mode get_sb_mode(void)
+{
+	enum efi_secureboot_mode mode;
+
+	if (!efi_rt_services_supported(EFI_RT_SUPPORTED_GET_VARIABLE)) {
+		pr_info("integrity: secureboot mode unknown, no efi\n");
+		return efi_secureboot_mode_unknown;
+	}
+
+	mode = efi_get_secureboot_mode(efi.get_variable);
+	if (mode == efi_secureboot_mode_disabled)
+		pr_info("integrity: secureboot mode disabled\n");
+	else if (mode == efi_secureboot_mode_unknown)
+		pr_info("integrity: secureboot mode unknown\n");
+	else
+		pr_info("integrity: secureboot mode enabled\n");
+	return mode;
+}
+
+bool __weak arch_integrity_get_secureboot(void)
+{
+	static enum efi_secureboot_mode sb_mode;
+	static bool initialized;
+
+	if (!initialized && efi_enabled(EFI_BOOT)) {
+		sb_mode = arch_integrity_efi_boot_mode;
+
+		if (sb_mode == efi_secureboot_mode_unset)
+			sb_mode = get_sb_mode();
+		initialized = true;
+	}
+
+	return sb_mode == efi_secureboot_mode_enabled;
+}
-- 
2.25.1

Re: [PATCH v2] integrity: Extract secure boot enquiry function out of IMA

[kernel test robot]

Hi GONG,

kernel test robot noticed the following build errors:

[auto build test ERROR on zohar-integrity/next-integrity]
[also build test ERROR on powerpc/next powerpc/fixes s390/features linus/master v6.16-rc4 next-20250627]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch#_base_tree_information]

url:    https://github.com/intel-lab-lkp/linux/commits/GONG-Ruiqi/integrity-Extract-secure-boot-enquiry-function-out-of-IMA/20250628-142236
base:   https://git.kernel.org/pub/scm/linux/kernel/git/zohar/linux-integrity.git next-integrity
patch link:    https://lore.kernel.org/r/20250628063251.321370-1-gongruiqi1%40huawei.com
patch subject: [PATCH v2] integrity: Extract secure boot enquiry function out of IMA
config: x86_64-randconfig-102-20250630 (https://download.01.org/0day-ci/archive/20250630/202506301150.yT6MxuHD-lkp@intel.com/config)
compiler: clang version 20.1.7 (https://github.com/llvm/llvm-project 6146a88f60492b520a36f8f8f3231e15f3cc6082)
reproduce (this is a W=1 build): (https://download.01.org/0day-ci/archive/20250630/202506301150.yT6MxuHD-lkp@intel.com/reproduce)

If you fix the issue in a separate patch/commit (i.e. not just a new version of
the same patch/commit), kindly add following tags
| Reported-by: kernel test robot <lkp@intel.com>
| Closes: https://lore.kernel.org/oe-kbuild-all/202506301150.yT6MxuHD-lkp@intel.com/

All errors (new ones prefixed by >>):

>> ld.lld: error: undefined symbol: arch_integrity_get_secureboot
   >>> referenced by ima_main.c:922 (security/integrity/ima/ima_main.c:922)
   >>>               vmlinux.o:(ima_load_data)

-- 
0-DAY CI Kernel Test Service
https://github.com/intel/lkp-tests/wiki

Re: [PATCH v2] integrity: Extract secure boot enquiry function out of IMA

[Mimi Zohar]

[CC: Nayna Jain]

On Sat, 2025-06-28 at 14:32 +0800, GONG Ruiqi wrote:
> Commit 92ad19559ea9 ("integrity: Do not load MOK and MOKx when secure
> boot be disabled") utilizes arch_ima_get_secureboot() to perform a
> secure boot status check before loading the Machine Owner Key (MOK).
> However, only when CONFIG_IMA_SECURE_AND_OR_TRUSTED_BOOT=y can this
> function be functional, while this config has nothing to do with secure
> boot or MOK loading.
> 
> Given that arch_ima_get_secureboot() is just a helper to retrieve info
> about secure boot via EFI and doesn't necessarily be a part of IMA,
> rename it to arch_integrity_get_secureboot(), decouple its functionality
> from IMA and extract it to be a integrity subsystem helper, so that both
> certificate loading and IMA can make use of it.
> 
> Compile-tested on powerpc, s390 and x86, with CONFIG_IMA_ARCH_POLICY=n
> and =y based on defconfig and allmodconfig.
> 
> Signed-off-by: GONG Ruiqi <gongruiqi1@huawei.com>

The original reason for querying the secure boot status of the system was in
order to differentiate IMA policies.  Subsequently, the secure boot check was
also added to safely allow loading of the certificates stored in MOK. So loading
IMA policies and the MOK certificates ARE dependent on the secure boot mode.
                                                                                
What is your real motivation for moving the secure boot checking out of IMA?    
                                                                                
FYI, there are a number of problems with the patch itself.  From a very high
level:  
                                                                                
- The EFI secure boot check is co-located with loading the architecture specific
policies.  By co-locating the secure boot check with loading the architecture
specific IMA policies, there aren't any ifdef's in C code.  Please refer to the
"conditional compilation" section in the kernel coding-style documentation on
avoiding ifdef's in C code.
                                                                                
- Each architecture has it's own method of detecting secure boot. Originally the
x86 code was in arch/x86, but to prevent code duplication it was moved to IMA. 
The new file should at least be named efi_secureboot.c.  
                                                                                
- The patch title should be about moving and renaming the secure boot check. 
The patch description should include a valid reason for the change.

Mimi & Nayna

Re: [PATCH v2] integrity: Extract secure boot enquiry function out of IMA

[GONG Ruiqi]

Hi Mimi,

On 7/3/2025 9:38 AM, Mimi Zohar wrote:
> [CC: Nayna Jain]
> 
> On Sat, 2025-06-28 at 14:32 +0800, GONG Ruiqi wrote:
>> ...
> 
> The original reason for querying the secure boot status of the system was in
> order to differentiate IMA policies.  Subsequently, the secure boot check was
> also added to safely allow loading of the certificates stored in MOK. So loading
> IMA policies and the MOK certificates ARE dependent on the secure boot mode.
>                                                                                 
> What is your real motivation for moving the secure boot checking out of IMA?    
>                                                                                 

Sorry for not stating that clearly in this patch. I think the cover
letter of V3 I just sent few minutes ago can answer your question, and I
quote:

"We encountered a boot failure issue in an in-house testing, where the
kernel refused to load its modules since it couldn't verify their
signature. The root cause turned out to be the early return of
load_uefi_certs(), where arch_ima_get_secureboot() returned false
unconditionally due to CONFIG_IMA_SECURE_AND_OR_TRUSTED_BOOT=n, even
though the secure boot was enabled.

This patch set attempts to remove this implicit dependency by shifting
the functionality of efi secure boot enquiry from IMA to the integrity
subsystem, so that both certificate loading and IMA can make use of it
independently."

Here's the link of V3, and please take a look:
https://lore.kernel.org/all/20250703014353.3366268-1-gongruiqi1@huawei.com/T/#mef6d5ea47a4ee19745c5292ab8948eba9e16628d

> FYI, there are a number of problems with the patch itself.  From a very high
> level:  
>                                                                                 
> - The EFI secure boot check is co-located with loading the architecture specific
> policies.  By co-locating the secure boot check with loading the architecture
> specific IMA policies, there aren't any ifdef's in C code.  Please refer to the
> "conditional compilation" section in the kernel coding-style documentation on
> avoiding ifdef's in C code.
>                                                                                 
> - Each architecture has it's own method of detecting secure boot. Originally the
> x86 code was in arch/x86, but to prevent code duplication it was moved to IMA. 
> The new file should at least be named efi_secureboot.c.  

You're right. I didn't realize it's arch-specific in the first place,
and moving and renaming arch_ima_get_secureboot() turned out to be a
real mess ...

So the V3 keeps the prototype of arch_ima_get_secureboot(), and only
moves out its body, which I think can also better represent the
intention of the patch.

As of the name of the new file, as V3 has been sent earlier and still
uses secureboot.c, I can't change it there. I can do it in V4.

-Ruiqi

>                                                                                 
> - The patch title should be about moving and renaming the secure boot check. 
> The patch description should include a valid reason for the change.
> 
> Mimi & Nayna

Re: [PATCH v2] integrity: Extract secure boot enquiry function out of IMA

[Mimi Zohar]

On Thu, 2025-07-03 at 10:07 +0800, GONG Ruiqi wrote:
> Hi Mimi,
> 
> On 7/3/2025 9:38 AM, Mimi Zohar wrote:
> > [CC: Nayna Jain]
> > 
> > On Sat, 2025-06-28 at 14:32 +0800, GONG Ruiqi wrote:
> > > ...
> > 
> > The original reason for querying the secure boot status of the system was in
> > order to differentiate IMA policies.  Subsequently, the secure boot check was
> > also added to safely allow loading of the certificates stored in MOK. So loading
> > IMA policies and the MOK certificates ARE dependent on the secure boot mode.
> >                                                                                 
> > What is your real motivation for moving the secure boot checking out of IMA?    
> >                                                                                 
> 
> Sorry for not stating that clearly in this patch. I think the cover
> letter of V3 I just sent few minutes ago can answer your question, and I
> quote:
> 
> "We encountered a boot failure issue in an in-house testing, where the
> kernel refused to load its modules since it couldn't verify their
> signature. The root cause turned out to be the early return of
> load_uefi_certs(), where arch_ima_get_secureboot() returned false
> unconditionally due to CONFIG_IMA_SECURE_AND_OR_TRUSTED_BOOT=n, even
> though the secure boot was enabled.
> 
> This patch set attempts to remove this implicit dependency by shifting
> the functionality of efi secure boot enquiry from IMA to the integrity
> subsystem, so that both certificate loading and IMA can make use of it
> independently."
> 
> Here's the link of V3, and please take a look:
> https://lore.kernel.org/all/20250703014353.3366268-1-gongruiqi1@huawei.com/T/#mef6d5ea47a4ee19745c5292ab8948eba9e16628d
> 
> > FYI, there are a number of problems with the patch itself.  From a very high
> > level:  
> >                                                                                 
> > - The EFI secure boot check is co-located with loading the architecture specific
> > policies.  By co-locating the secure boot check with loading the architecture
> > specific IMA policies, there aren't any ifdef's in C code.  Please refer to the
> > "conditional compilation" section in the kernel coding-style documentation on
> > avoiding ifdef's in C code.
> >                                                                                 
> > - Each architecture has it's own method of detecting secure boot. Originally the
> > x86 code was in arch/x86, but to prevent code duplication it was moved to IMA. 
> > The new file should at least be named efi_secureboot.c.  
> 
> You're right. I didn't realize it's arch-specific in the first place,
> and moving and renaming arch_ima_get_secureboot() turned out to be a
> real mess ...
> 
> So the V3 keeps the prototype of arch_ima_get_secureboot(), and only
> moves out its body, which I think can also better represent the
> intention of the patch.

It's definitely much better.  To summarize, arch_ima_get_secureboot() becomes a
wrapper for integrity_get_efi_secureboot().  Before loading the MOK/MOKx keys,
load_uefi_certs() calls integrity_get_efi_secureboot() directly.

With load_uefi_certs() calling integrity_get_efi_secureboot() directly, please
check to see whether an integrity_get_efi_secureboot() stub function needs to be
defined.

Mimi

> 
> As of the name of the new file, as V3 has been sent earlier and still
> uses secureboot.c, I can't change it there. I can do it in V4.
> 
> -Ruiqi

Re: [PATCH v2] integrity: Extract secure boot enquiry function out of IMA

[GONG Ruiqi]

On 7/3/2025 11:35 AM, Mimi Zohar wrote:
> ...
>>
>> You're right. I didn't realize it's arch-specific in the first place,
>> and moving and renaming arch_ima_get_secureboot() turned out to be a
>> real mess ...
>>
>> So the V3 keeps the prototype of arch_ima_get_secureboot(), and only
>> moves out its body, which I think can also better represent the
>> intention of the patch.
> 
> It's definitely much better.  To summarize, arch_ima_get_secureboot() becomes a
> wrapper for integrity_get_efi_secureboot().  Before loading the MOK/MOKx keys,
> load_uefi_certs() calls integrity_get_efi_secureboot() directly.

Exactly.

> 
> With load_uefi_certs() calling integrity_get_efi_secureboot() directly, please
> check to see whether an integrity_get_efi_secureboot() stub function needs to be
> defined.

For CONFIG_LOAD_UEFI_KEYS and CONFIG_IMA_SECURE_AND_OR_TRUSTED_BOOT, the
former depends on CONFIG_EFI while the latter is implied by the same, so

  integrity-$(CONFIG_EFI) += secureboot.o

should be enough. I've compile-tested the V3 on x86/arm64/powerpc/s390,
with various config combinations as much as I can think of. Let's see if
the kernel test robot could find out some corner cases.

-Ruiqi

> 
> Mimi
> 
>>
>> As of the name of the new file, as V3 has been sent earlier and still
>> uses secureboot.c, I can't change it there. I can do it in V4.
>>
>> -Ruiqi
> 

Re: [PATCH v2] integrity: Extract secure boot enquiry function out of IMA

[Nayna Jain]

On 7/2/25 10:07 PM, GONG Ruiqi wrote:
> Hi Mimi,
>
> On 7/3/2025 9:38 AM, Mimi Zohar wrote:
>> [CC: Nayna Jain]
>>
>> On Sat, 2025-06-28 at 14:32 +0800, GONG Ruiqi wrote:
>>> ...
>> The original reason for querying the secure boot status of the system was in
>> order to differentiate IMA policies.  Subsequently, the secure boot check was
>> also added to safely allow loading of the certificates stored in MOK. So loading
>> IMA policies and the MOK certificates ARE dependent on the secure boot mode.
>>                                                                                  
>> What is your real motivation for moving the secure boot checking out of IMA?
>>                                                                                  
> Sorry for not stating that clearly in this patch. I think the cover
> letter of V3 I just sent few minutes ago can answer your question, and I
> quote:
>
> "We encountered a boot failure issue in an in-house testing, where the
> kernel refused to load its modules since it couldn't verify their
> signature. The root cause turned out to be the early return of
> load_uefi_certs(), where arch_ima_get_secureboot() returned false
> unconditionally due to CONFIG_IMA_SECURE_AND_OR_TRUSTED_BOOT=n, even
> though the secure boot was enabled.
Thanks for sharing additional details.

 From x86 Kconfig:

/For config x86:

     imply IMA_SECURE_AND_OR_TRUSTED_BOOT    if EFI
/
And IMA_SECURE_AND_OR_TRUSTED_BOOT is dependent on IMA_ARCH_POLICY .

And from Linux Kernel Kbuild documentation( 
https://docs.kernel.org/kbuild/kconfig-language.html) :

/weak reverse dependencies: “imply” <symbol> [“if” <expr>]

This is similar to “select” as it enforces a lower limit on another 
symbol except that the “implied” symbol’s value may still be set to n 
from a direct dependency or with a visible prompt.

/Following the example from the documentation, if  it is EFI enabled and 
IMA_ARCH_POLICY is set to y then this config should be default enabled.

If it is EFI enabled and IMA_ARCH_POLICY is set to N, then the setting 
for IMA_SECURE_AND_OR_TRUSTED_BOOT should be prompted during the build. 
The default setting for prompt is N. So, the person doing the build 
should actually select Y to enable IMA_ARCH_POLICY.

Wondering what is the scenario for you? Unless you have IMA_ARCH_POLICY 
set to N, this config should have been ideally enabled. If you have 
explicitly set it to N, am curious any specific reason for that.

Thanks & Regards,

    - Nayna
>
> This patch set attempts to remove this implicit dependency by shifting
> the functionality of efi secure boot enquiry from IMA to the integrity
> subsystem, so that both certificate loading and IMA can make use of it
> independently."
>
> Here's the link of V3, and please take a look:
> https://lore.kernel.org/all/20250703014353.3366268-1-gongruiqi1@huawei.com/T/#mef6d5ea47a4ee19745c5292ab8948eba9e16628d
>
>> FYI, there are a number of problems with the patch itself.  From a very high
>> level:
>>                                                                                  
>> - The EFI secure boot check is co-located with loading the architecture specific
>> policies.  By co-locating the secure boot check with loading the architecture
>> specific IMA policies, there aren't any ifdef's in C code.  Please refer to the
>> "conditional compilation" section in the kernel coding-style documentation on
>> avoiding ifdef's in C code.
>>                                                                                  
>> - Each architecture has it's own method of detecting secure boot. Originally the
>> x86 code was in arch/x86, but to prevent code duplication it was moved to IMA.
>> The new file should at least be named efi_secureboot.c.
> You're right. I didn't realize it's arch-specific in the first place,
> and moving and renaming arch_ima_get_secureboot() turned out to be a
> real mess ...
>
> So the V3 keeps the prototype of arch_ima_get_secureboot(), and only
> moves out its body, which I think can also better represent the
> intention of the patch.
>
> As of the name of the new file, as V3 has been sent earlier and still
> uses secureboot.c, I can't change it there. I can do it in V4.
>
> -Ruiqi
>
>>                                                                                  
>> - The patch title should be about moving and renaming the secure boot check.
>> The patch description should include a valid reason for the change.
>>
>> Mimi & Nayna

Re: [PATCH v2] integrity: Extract secure boot enquiry function out of IMA

[GONG Ruiqi]

On 7/8/2025 4:35 AM, Nayna Jain wrote:
> 
> On 7/2/25 10:07 PM, GONG Ruiqi wrote:
>> 
>> ...
>>
>> "We encountered a boot failure issue in an in-house testing, where the
>> kernel refused to load its modules since it couldn't verify their
>> signature. The root cause turned out to be the early return of
>> load_uefi_certs(), where arch_ima_get_secureboot() returned false
>> unconditionally due to CONFIG_IMA_SECURE_AND_OR_TRUSTED_BOOT=n, even
>> though the secure boot was enabled.
> Thanks for sharing additional details.
> 
> From x86 Kconfig:
> 
> /For config x86:
> 
>     imply IMA_SECURE_AND_OR_TRUSTED_BOOT    if EFI
> /
> And IMA_SECURE_AND_OR_TRUSTED_BOOT is dependent on IMA_ARCH_POLICY .
> 
> And from Linux Kernel Kbuild documentation( https://docs.kernel.org/
> kbuild/kconfig-language.html) :
> 
> /weak reverse dependencies: “imply” <symbol> [“if” <expr>]
> 
> This is similar to “select” as it enforces a lower limit on another
> symbol except that the “implied” symbol’s value may still be set to n
> from a direct dependency or with a visible prompt.
> 
> /Following the example from the documentation, if  it is EFI enabled and
> IMA_ARCH_POLICY is set to y then this config should be default enabled.
> 
> If it is EFI enabled and IMA_ARCH_POLICY is set to N, then the setting
> for IMA_SECURE_AND_OR_TRUSTED_BOOT should be prompted during the build.
> The default setting for prompt is N. So, the person doing the build
> should actually select Y to enable IMA_ARCH_POLICY.
> 
> Wondering what is the scenario for you? Unless you have IMA_ARCH_POLICY
> set to N, this config should have been ideally enabled. If you have
> explicitly set it to N, am curious any specific reason for that.

Hi Nayna. Sorry for the late reply. Super busy these days...

Yes, IMA_ARCH_POLICY was not set. The testing was conducted on
openEuler[1], a Linux distro mainly for arm64 & x86, and the kernel was
compiled based on its own openeuler_defconfig[2], which set
IMA_ARCH_POLICY to N.

-Ruiqi

[1]: https://www.openeuler.org/en/
[2]:
https://gitee.com/openeuler/kernel/blob/OLK-6.6/arch/arm64/configs/openeuler_defconfig


> 
> Thanks & Regards,
> 
>    - Nayna
>>
>> ...

Re: [PATCH v2] integrity: Extract secure boot enquiry function out of IMA

[Nayna Jain]

On 7/17/25 8:29 AM, GONG Ruiqi wrote:
> On 7/8/2025 4:35 AM, Nayna Jain wrote:
>> On 7/2/25 10:07 PM, GONG Ruiqi wrote:
>>> ...
>>>
>>> "We encountered a boot failure issue in an in-house testing, where the
>>> kernel refused to load its modules since it couldn't verify their
>>> signature. The root cause turned out to be the early return of
>>> load_uefi_certs(), where arch_ima_get_secureboot() returned false
>>> unconditionally due to CONFIG_IMA_SECURE_AND_OR_TRUSTED_BOOT=n, even
>>> though the secure boot was enabled.
>> Thanks for sharing additional details.
>>
>>  From x86 Kconfig:
>>
>> /For config x86:
>>
>>      imply IMA_SECURE_AND_OR_TRUSTED_BOOT    if EFI
>> /
>> And IMA_SECURE_AND_OR_TRUSTED_BOOT is dependent on IMA_ARCH_POLICY .
>>
>> And from Linux Kernel Kbuild documentation( https://docs.kernel.org/
>> kbuild/kconfig-language.html) :
>>
>> /weak reverse dependencies: “imply” <symbol> [“if” <expr>]
>>
>> This is similar to “select” as it enforces a lower limit on another
>> symbol except that the “implied” symbol’s value may still be set to n
>> from a direct dependency or with a visible prompt.
>>
>> /Following the example from the documentation, if  it is EFI enabled and
>> IMA_ARCH_POLICY is set to y then this config should be default enabled.
>>
>> If it is EFI enabled and IMA_ARCH_POLICY is set to N, then the setting
>> for IMA_SECURE_AND_OR_TRUSTED_BOOT should be prompted during the build.
>> The default setting for prompt is N. So, the person doing the build
>> should actually select Y to enable IMA_ARCH_POLICY.
>>
>> Wondering what is the scenario for you? Unless you have IMA_ARCH_POLICY
>> set to N, this config should have been ideally enabled. If you have
>> explicitly set it to N, am curious any specific reason for that.
> Hi Nayna. Sorry for the late reply. Super busy these days...
>
> Yes, IMA_ARCH_POLICY was not set. The testing was conducted on
> openEuler[1], a Linux distro mainly for arm64 & x86, and the kernel was
> compiled based on its own openeuler_defconfig[2], which set
> IMA_ARCH_POLICY to N.

Thanks Ruiqi for the response.

It seems the main cause of the problem was that IMA_ARCH_POLICY config 
wasn't enabled; and it sounds like you don't need IMA arch policies but 
you do need the arch specific function to get the secure boot status.

In that case, removing IMA_SECURE_AND_OR_TRUSTED_BOOT config dependency 
on IMA_ARCH_POLICY config and updating the corresponding help is all 
that is needed.

The help text can be updated to:
This option is selected by architectures to detect systems with secure 
and/or trusted boot enabled, in order to load the appropriate IMA 
runtime policies and keys.

Thanks & Regards,

     - Nayna

Re: [PATCH v2] integrity: Extract secure boot enquiry function out of IMA

[GONG Ruiqi]

On 7/26/2025 2:29 AM, Nayna Jain wrote:
> 
> On 7/17/25 8:29 AM, GONG Ruiqi wrote:
>> On 7/8/2025 4:35 AM, Nayna Jain wrote:
>>> On 7/2/25 10:07 PM, GONG Ruiqi wrote:
>>>> ...
>>
>> Yes, IMA_ARCH_POLICY was not set. The testing was conducted on
>> openEuler[1], a Linux distro mainly for arm64 & x86, and the kernel was
>> compiled based on its own openeuler_defconfig[2], which set
>> IMA_ARCH_POLICY to N.
> 
> Thanks Ruiqi for the response.
> 
> It seems the main cause of the problem was that IMA_ARCH_POLICY config
> wasn't enabled; and it sounds like you don't need IMA arch policies but
> you do need the arch specific function to get the secure boot status.
> 
> In that case, removing IMA_SECURE_AND_OR_TRUSTED_BOOT config dependency
> on IMA_ARCH_POLICY config and updating the corresponding help is all
> that is needed.

I think it doesn't solve the real problems, which are: 1. the implicit
dependency of LOAD_UEFI_KEYS to IMA_SECURE_AND_OR_TRUSTED_BOOT, which
surprises people, and 2. what arch_ima_get_secureboot() does is
essentially a stand-alone function and it's not necessarily be a part of
IMA, but it's still controlled by IMA_SECURE_AND_OR_TRUSTED_BOOT.

I agree that adjusting Kconfig could be simpler, but breaking
IMA_SECURE_AND_OR_TRUSTED_BOOT's dependency to IMA_ARCH_POLICY doesn't
help on both. If that's gonna be the way we will take, what I would
propose is to let LOAD_UEFI_KEYS select IMA_SECURE_AND_OR_TRUSTED_BOOT,
which states the dependency explicitly so at least solves the problem 1.

-Ruiqi

> 
> The help text can be updated to:
> This option is selected by architectures to detect systems with secure
> and/or trusted boot enabled, in order to load the appropriate IMA
> runtime policies and keys.
> 
> Thanks & Regards,
> 
>     - Nayna
> 

Re: [PATCH v2] integrity: Extract secure boot enquiry function out of IMA

[Nayna Jain]

On 7/28/25 8:17 AM, GONG Ruiqi wrote:
> On 7/26/2025 2:29 AM, Nayna Jain wrote:
>> On 7/17/25 8:29 AM, GONG Ruiqi wrote:
>>> On 7/8/2025 4:35 AM, Nayna Jain wrote:
>>>> On 7/2/25 10:07 PM, GONG Ruiqi wrote:
>>>>> ...
>>> Yes, IMA_ARCH_POLICY was not set. The testing was conducted on
>>> openEuler[1], a Linux distro mainly for arm64 & x86, and the kernel was
>>> compiled based on its own openeuler_defconfig[2], which set
>>> IMA_ARCH_POLICY to N.
>> Thanks Ruiqi for the response.
>>
>> It seems the main cause of the problem was that IMA_ARCH_POLICY config
>> wasn't enabled; and it sounds like you don't need IMA arch policies but
>> you do need the arch specific function to get the secure boot status.
>>
>> In that case, removing IMA_SECURE_AND_OR_TRUSTED_BOOT config dependency
>> on IMA_ARCH_POLICY config and updating the corresponding help is all
>> that is needed.
> I think it doesn't solve the real problems, which are: 1. the implicit
> dependency of LOAD_UEFI_KEYS to IMA_SECURE_AND_OR_TRUSTED_BOOT, which
> surprises people, and 2. what arch_ima_get_secureboot() does is
> essentially a stand-alone function and it's not necessarily be a part of
> IMA, but it's still controlled by IMA_SECURE_AND_OR_TRUSTED_BOOT.
>
> I agree that adjusting Kconfig could be simpler, but breaking
> IMA_SECURE_AND_OR_TRUSTED_BOOT's dependency to IMA_ARCH_POLICY doesn't
> help on both. If that's gonna be the way we will take, what I would
> propose is to let LOAD_UEFI_KEYS select IMA_SECURE_AND_OR_TRUSTED_BOOT,
> which states the dependency explicitly so at least solves the problem 1.

Hi Ruiqi,

IMA_SECURE_AND_OR_TRUSTED_BOOT is already enabled by different 
architectures. Having LOAD_UEFI_KEYS select it would help only if 
IMA_ARCH_POLICY is also selected.

Thanks & Regards,

    - Nayna