wire up IMA secure boot for arm64

From: Chester Lin <[email protected]>

Generalize the efi_get_secureboot() function so not only efistub but also other subsystems can use it.

Note that the MokSbState handling is not factored out: the variable is boot time only, and so it cannot be parameterized as easily. Also, the IMA code will switch to this version in a future patch, and it does not incorporate the MokSbState exception in the first place.

Note that the new efi_get_secureboot_mode() helper treats any failures to read SetupMode as setup mode being disabled.

Co-developed-by Chester Lin <[email protected]>

Signed-off-by Chester Lin <[email protected]>

Signed-off-by Ard Biesheuvel <[email protected]>

Quoted 10 lines · Ard Biesheuvel

From: Chester Lin <[email protected]>

Generalize the efi_get_secureboot() function so not only efistub but also other subsystems can use it.

Note that the MokSbState handling is not factored out: the variable is boot time only, and so it cannot be parameterized as easily. Also, the IMA code will switch to this version in a future patch, and it does not incorporate the MokSbState exception in the first place.

Quoted 6 lines

Note that the new efi_get_secureboot_mode() helper treats any failures to read SetupMode as setup mode being disabled.

Co-developed-by Chester Lin <[email protected]>

Signed-off-by Chester Lin <[email protected]>

Signed-off-by Ard Biesheuvel <[email protected]>

Thanks, Ard. Other than one minor thing inline below, the patch looks good. I haven't done any testing yet.

include/linux/efi.h enum efi_secureboot_mode { · line 1087

Quoted 12 lines

include/linux/efi.h +3 -1

index bd9d83a94173..79b2d4de62e0 100644
@@ -1082,7 +1082,28 @@ 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_mode(efi_get_variable_t *get_var)

get_var() should be defined as "efi_status_t". If this is being
upstreamed via integrity, I can make the change.

Quoted 3 lines

get_var() should be defined as "efi_status_t". If this is being
upstreamed via integrity, I can make the change.

No, get_var is a pointer to a function returning efi_status_t, check include/linux/efi.h for details.

From: Chester Lin <[email protected]>

Move the x86 IMA arch code into security/integrity/ima/ima_efi.c, so that we will be able to wire it up for arm64 in a future patch.

Co-developed-by Chester Lin <[email protected]>

Signed-off-by Chester Lin <[email protected]>

Signed-off-by Ard Biesheuvel <[email protected]>

---
 arch/x86/include/asm/efi.h                                     |  3 ++
 arch/x86/kernel/Makefile                                       |  2 -
 security/integrity/ima/Makefile                                |  4 ++
 arch/x86/kernel/ima_arch.c => security/integrity/ima/ima_efi.c | 45 ++++++--------------
 4 files changed, 19 insertions(+), 35 deletions(-)

arch/x86/include/asm/efi.h +3 -0

index 7673dc833232..c98f78330b09 100644
@@ -380,4 +380,7 @@ static inline void efi_fake_memmap_early(void)
 }
 #endif
 
+#define arch_ima_efi_boot_mode	\
+	({ extern struct boot_params boot_params; boot_params.secure_boot; })
+
 #endif /* _ASM_X86_EFI_H */

arch/x86/kernel/Makefile +0 -2

index 68608bd892c0..5eeb808eb024 100644
@@ -161,5 +161,3 @@ ifeq ($(CONFIG_X86_64),y)
 	obj-$(CONFIG_MMCONF_FAM10H)	+= mmconf-fam10h_64.o
 	obj-y				+= vsmp_64.o
 endif
-
-obj-$(CONFIG_IMA_SECURE_AND_OR_TRUSTED_BOOT)	+= ima_arch.o

security/integrity/ima/Makefile +4 -0

index 67dabca670e2..2499f2485c04 100644
@@ -14,3 +14,7 @@ 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
+
+ifeq ($(CONFIG_EFI),y)
+ima-$(CONFIG_IMA_SECURE_AND_OR_TRUSTED_BOOT) += ima_efi.o
+endif

security/integrity/ima/ima_efi.c renamed from arch/x86/kernel/ima_arch.c +0 -0

similarity index 60%
rename from arch/x86/kernel/ima_arch.c
rename to security/integrity/ima/ima_efi.c
index 7dfb1e808928..233627a9d4b8 100644

security/integrity/ima/ima_efi.c renamed from arch/x86/kernel/ima_arch.c +9 -32

+#endif
 
 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);
+	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;
 	}
 
-	/* Get variable contents into buffer */
-	status = efi.get_variable(L"SecureBoot", &efi_variable_guid,
-				  NULL, &size, &secboot);
-	if (status == EFI_NOT_FOUND) {
+	mode = efi_get_secureboot_mode(efi.get_variable);
+	if (mode == efi_secureboot_mode_disabled)
 		pr_info("ima: secureboot mode disabled\n");
-		return efi_secureboot_mode_disabled;
-	}
-
-	if (status != EFI_SUCCESS) {
+	else if (mode == efi_secureboot_mode_unknown)
 		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;
+	else
+		pr_info("ima: secureboot mode enabled\n");
+	return mode;
 }
 
 bool arch_ima_get_secureboot(void)

@@ -57,7 +36,7 @@ bool arch_ima_get_secureboot(void)
 	static bool initialized;
 
 	if (!initialized && efi_enabled(EFI_BOOT)) {
-		sb_mode = boot_params.secure_boot;
+		sb_mode = arch_ima_efi_boot_mode;
 
 		if (sb_mode == efi_secureboot_mode_unset)
 			sb_mode = get_sb_mode();

Signature · 1 line

2.17.1

Hi Ard,

Quoted 62 lines · Ard Biesheuvel

From: Chester Lin <[email protected]>

Move the x86 IMA arch code into security/integrity/ima/ima_efi.c, so that we will be able to wire it up for arm64 in a future patch.

Co-developed-by Chester Lin <[email protected]>

Signed-off-by Chester Lin <[email protected]>

Signed-off-by Ard Biesheuvel <[email protected]>

---
 arch/x86/include/asm/efi.h                                     |  3 ++
 arch/x86/kernel/Makefile                                       |  2 -
 security/integrity/ima/Makefile                                |  4 ++
 arch/x86/kernel/ima_arch.c => security/integrity/ima/ima_efi.c | 45 ++++++--------------
 4 files changed, 19 insertions(+), 35 deletions(-)

arch/x86/include/asm/efi.h +3 -0

index 7673dc833232..c98f78330b09 100644
@@ -380,4 +380,7 @@ static inline void efi_fake_memmap_early(void)
 }
 #endif
 
+#define arch_ima_efi_boot_mode       \
+     ({ extern struct boot_params boot_params; boot_params.secure_boot; })
+
 #endif /* _ASM_X86_EFI_H */

arch/x86/kernel/Makefile +0 -2

index 68608bd892c0..5eeb808eb024 100644
@@ -161,5 +161,3 @@ ifeq ($(CONFIG_X86_64),y)
      obj-$(CONFIG_MMCONF_FAM10H)     += mmconf-fam10h_64.o
      obj-y                           += vsmp_64.o
 endif
-
-obj-$(CONFIG_IMA_SECURE_AND_OR_TRUSTED_BOOT) += ima_arch.o

security/integrity/ima/Makefile +4 -0

index 67dabca670e2..2499f2485c04 100644
@@ -14,3 +14,7 @@ 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
+
+ifeq ($(CONFIG_EFI),y)
+ima-$(CONFIG_IMA_SECURE_AND_OR_TRUSTED_BOOT) += ima_efi.o
+endif

security/integrity/ima/ima_efi.c renamed from arch/x86/kernel/ima_arch.c +1 -0

similarity index 60%
rename from arch/x86/kernel/ima_arch.c
rename to security/integrity/ima/ima_efi.c
index 7dfb1e808928..233627a9d4b8 100644
@@ -5,50 +5,29 @@
 #include <linux/efi.h>
 #include <linux/module.h>
 #include <linux/ima.h>
+#include <asm/efi.h>
 

security/integrity/ima/ima_efi.c line 11

Quoted 3 lines

security/integrity/ima/ima_efi.c renamed from arch/x86/kernel/ima_arch.c +2 -1

-extern struct boot_params boot_params;
+#ifndef arch_ima_efi_boot_mode
+#define arch_ima_efi_boot_mode efi_secureboot_mode_unknown

I think this should be "efi_secureboot_mode_unset" otherwise the get_sb_mode() will never be called. The others look good to me, thanks for your help.

Quoted 65 lines

security/integrity/ima/ima_efi.c renamed from arch/x86/kernel/ima_arch.c +9 -33

+#endif
 
 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);
+     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;
      }
 
-     /* Get variable contents into buffer */
-     status = efi.get_variable(L"SecureBoot", &efi_variable_guid,
-                               NULL, &size, &secboot);
-     if (status == EFI_NOT_FOUND) {
+     mode = efi_get_secureboot_mode(efi.get_variable);
+     if (mode == efi_secureboot_mode_disabled)
              pr_info("ima: secureboot mode disabled\n");
-             return efi_secureboot_mode_disabled;
-     }
-
-     if (status != EFI_SUCCESS) {
+     else if (mode == efi_secureboot_mode_unknown)
              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;
+     else
+             pr_info("ima: secureboot mode enabled\n");
+     return mode;
 }
 
 bool arch_ima_get_secureboot(void)

@@ -57,7 +36,7 @@ bool arch_ima_get_secureboot(void)
      static bool initialized;
 
      if (!initialized && efi_enabled(EFI_BOOT)) {
-             sb_mode = boot_params.secure_boot;
+             sb_mode = arch_ima_efi_boot_mode;
 
              if (sb_mode == efi_secureboot_mode_unset)
                      sb_mode = get_sb_mode();
--

2.17.1

From: Chester Lin <[email protected]>

Add arm64 IMA arch support. The code and arch policy is mainly inherited from x86.

Co-developed-by Chester Lin <[email protected]>

Signed-off-by Chester Lin <[email protected]>

Signed-off-by Ard Biesheuvel <[email protected]>

This is a follow-up to Chester's series [0] to enable IMA to the secure boot state of arm64 platforms, which is EFI based.

This v4 implements the changes I suggested to Chester, in particular:
- disregard MokSbState when factoring out secure boot mode discovery
- turn the x86 IMA arch code into shared code for all architectures.

This reduces the final patch to a one liner enabling a Kconfig option for arm64 when EFI is enabled.

Build tested only.

Hi Ard, Chester,

Quoted 7 lines · Ard Biesheuvel

This is a follow-up to Chester's series [0] to enable IMA to the secure boot state of arm64 platforms, which is EFI based.

This v4 implements the changes I suggested to Chester, in particular:
- disregard MokSbState when factoring out secure boot mode discovery
- turn the x86 IMA arch code into shared code for all architectures.

Quoted 4 lines

This reduces the final patch to a one liner enabling a Kconfig option for arm64 when EFI is enabled.

Build tested only.

Thank you! This patch set is now queued in the linux-integrity next- integrity-testing branch.

Quoted 11 lines · Mimi Zohar

Hi Ard, Chester,

Quoted 7 lines · Ard Biesheuvel

This is a follow-up to Chester's series [0] to enable IMA to the secure boot state of arm64 platforms, which is EFI based.

This v4 implements the changes I suggested to Chester, in particular:
- disregard MokSbState when factoring out secure boot mode discovery
- turn the x86 IMA arch code into shared code for all architectures.

Quoted 8 lines

Quoted 4 lines

This reduces the final patch to a one liner enabling a Kconfig option for arm64 when EFI is enabled.

Build tested only.

Thank you! This patch set is now queued in the linux-integrity next- integrity-testing branch.

I don't mind per se, but this touches a number of different trees, including x86 and arm64, and nobody has acked it yet.

As far as the EFI tree is concerned, it looks like I should be able to avoid any conflicts with other stuff that is in flight, and if not, we can always use your branch up until the last patch in this serires as a shared tag (assuming you won't rebase it).

Quoted 21 lines · Ard Biesheuvel

Quoted 11 lines · Mimi Zohar

Hi Ard, Chester,

Quoted 7 lines · Ard Biesheuvel

This is a follow-up to Chester's series [0] to enable IMA to the secure boot state of arm64 platforms, which is EFI based.

This v4 implements the changes I suggested to Chester, in particular:
- disregard MokSbState when factoring out secure boot mode discovery
- turn the x86 IMA arch code into shared code for all architectures.

Quoted 8 lines

Quoted 4 lines

This reduces the final patch to a one liner enabling a Kconfig option for arm64 when EFI is enabled.

Build tested only.

Thank you! This patch set is now queued in the linux-integrity next- integrity-testing branch.

Quoted 7 lines

I don't mind per se, but this touches a number of different trees, including x86 and arm64, and nobody has acked it yet.

As far as the EFI tree is concerned, it looks like I should be able to avoid any conflicts with other stuff that is in flight, and if not, we can always use your branch up until the last patch in this serires as a shared tag (assuming you won't rebase it).

The next-integrity-testing branch is just a place holder waiting for additional tags. I've reviewed and tested the patch set on x86. Based on the secure boot status and how the kernel is configured, the
appropriate policy rules are enabled. Similarly the IMA appraise mode
(ima_appraise=) is working properly. I have not tested on arm64.

I do not have a problem with this patch set being upstream via EFI.

Quoted 2 lines

I do not have a problem with this patch set being upstream via EFI.

Ah right. That is probably better, as EFI goes via the x86 tree, and I work closely with the arm64 maintainers on other things as well.

Please let me know once you are ready to ack this from IMA pov, and I will carry it further.

Quoted 6 lines · Ard Biesheuvel

Quoted 2 lines

I do not have a problem with this patch set being upstream via EFI.

Ah right. That is probably better, as EFI goes via the x86 tree, and I work closely with the arm64 maintainers on other things as well.

Quoted 2 lines

Please let me know once you are ready to ack this from IMA pov, and I will carry it further.

thanks,

Acked-by Mimi Zohar <[email protected]>