[PATCH v4 0/3] wire up IMA secure boot for arm64

[PATCH v4 0/3] wire up IMA secure boot for arm64

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

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

Build tested only.

[0] https://lore.kernel.org/linux-arm-kernel/20201030060840.1810-1-clin@suse.com/

Cc: zohar@linux.ibm.com
Cc: jmorris@namei.org
Cc: serge@hallyn.com
Cc: dmitry.kasatkin@gmail.com
Cc: catalin.marinas@arm.com
Cc: will@kernel.org
Cc: clin@suse.com
Cc: x86@kernel.org
Cc: jlee@suse.com
Cc: linux-integrity@vger.kernel.org,
Cc: linux-arm-kernel@lists.infradead.org
  
Chester Lin (3):
  efi: generalize efi_get_secureboot
  ima: generalize x86/EFI arch glue for other EFI architectures
  arm64/ima: add ima_arch support

 arch/arm64/Kconfig                            |  1 +
 arch/x86/boot/compressed/Makefile             |  2 +-
 arch/x86/include/asm/efi.h                    |  3 ++
 arch/x86/kernel/Makefile                      |  2 -
 drivers/firmware/efi/libstub/efistub.h        |  2 +
 drivers/firmware/efi/libstub/secureboot.c     | 41 +++++++----------
 include/linux/efi.h                           | 23 +++++++++-
 security/integrity/ima/Makefile               |  4 ++
 .../integrity/ima/ima_efi.c                   | 45 +++++--------------
 9 files changed, 60 insertions(+), 63 deletions(-)
 rename arch/x86/kernel/ima_arch.c => security/integrity/ima/ima_efi.c (60%)

-- 
2.17.1

[PATCH v4 1/3] efi: generalize efi_get_secureboot

[Ard Biesheuvel]

From: Chester Lin <clin@suse.com>

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 <clin@suse.com>
Signed-off-by: Chester Lin <clin@suse.com>
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 arch/x86/boot/compressed/Makefile         |  2 +-
 drivers/firmware/efi/libstub/efistub.h    |  2 +
 drivers/firmware/efi/libstub/secureboot.c | 41 +++++++-------------
 include/linux/efi.h                       | 23 ++++++++++-
 4 files changed, 40 insertions(+), 28 deletions(-)

diff --git a/arch/x86/boot/compressed/Makefile b/arch/x86/boot/compressed/Makefile
index ee249088cbfe..8d358a6fe6ec 100644
--- a/arch/x86/boot/compressed/Makefile
+++ b/arch/x86/boot/compressed/Makefile
@@ -35,7 +35,7 @@ cflags-$(CONFIG_X86_32) := -march=i386
 cflags-$(CONFIG_X86_64) := -mcmodel=small -mno-red-zone
 KBUILD_CFLAGS += $(cflags-y)
 KBUILD_CFLAGS += -mno-mmx -mno-sse
-KBUILD_CFLAGS += -ffreestanding
+KBUILD_CFLAGS += -ffreestanding -fshort-wchar
 KBUILD_CFLAGS += -fno-stack-protector
 KBUILD_CFLAGS += $(call cc-disable-warning, address-of-packed-member)
 KBUILD_CFLAGS += $(call cc-disable-warning, gnu)
diff --git a/drivers/firmware/efi/libstub/efistub.h b/drivers/firmware/efi/libstub/efistub.h
index 2d7abcd99de9..b8ec29d6a74a 100644
--- a/drivers/firmware/efi/libstub/efistub.h
+++ b/drivers/firmware/efi/libstub/efistub.h
@@ -848,4 +848,6 @@ asmlinkage void __noreturn efi_enter_kernel(unsigned long entrypoint,
 
 void efi_handle_post_ebs_state(void);
 
+enum efi_secureboot_mode efi_get_secureboot(void);
+
 #endif
diff --git a/drivers/firmware/efi/libstub/secureboot.c b/drivers/firmware/efi/libstub/secureboot.c
index 5efc524b14be..af18d86c1604 100644
--- a/drivers/firmware/efi/libstub/secureboot.c
+++ b/drivers/firmware/efi/libstub/secureboot.c
@@ -12,15 +12,16 @@
 
 #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";
 
+static efi_status_t get_var(efi_char16_t *name, efi_guid_t *vendor, u32 *attr,
+			    unsigned long *data_size, void *data)
+{
+	return get_efi_var(name, vendor, attr, data_size, data);
+}
+
 /*
  * Determine whether we're in secure boot mode.
  *
@@ -30,26 +31,18 @@ static const efi_char16_t shim_MokSBState_name[] = L"MokSBState";
 enum efi_secureboot_mode efi_get_secureboot(void)
 {
 	u32 attr;
-	u8 secboot, setupmode, moksbstate;
 	unsigned long size;
+	enum efi_secureboot_mode mode;
 	efi_status_t status;
+	u8 moksbstate;
 
-	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;
+	mode = efi_get_secureboot_mode(get_var);
+	if (mode == efi_secureboot_mode_unknown) {
+		efi_err("Could not determine UEFI Secure Boot status.\n");
+		return efi_secureboot_mode_unknown;
+	}
+	if (mode != efi_secureboot_mode_enabled)
+		return mode;
 
 	/*
 	 * See if a user has put the shim into insecure mode. If so, and if the
@@ -69,8 +62,4 @@ enum efi_secureboot_mode efi_get_secureboot(void)
 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/include/linux/efi.h b/include/linux/efi.h
index bd9d83a94173..79b2d4de62e0 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -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)
+{
+	u8 secboot, setupmode = 0;
+	efi_status_t status;
+	unsigned long size;
+
+	size = sizeof(secboot);
+	status = get_var(L"SecureBoot", &EFI_GLOBAL_VARIABLE_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);
+	get_var(L"SetupMode", &EFI_GLOBAL_VARIABLE_GUID, NULL, &size, &setupmode);
+	if (secboot == 0 || setupmode == 1)
+		return efi_secureboot_mode_disabled;
+	return efi_secureboot_mode_enabled;
+}
 
 #ifdef CONFIG_RESET_ATTACK_MITIGATION
 void efi_enable_reset_attack_mitigation(void);
-- 
2.17.1

Re: [PATCH v4 1/3] efi: generalize efi_get_secureboot

[Mimi Zohar]

On Mon, 2020-11-02 at 23:37 +0100, Ard Biesheuvel wrote:
> From: Chester Lin <clin@suse.com>
> 
> 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 <clin@suse.com>
> Signed-off-by: Chester Lin <clin@suse.com>
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>

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

> diff --git a/include/linux/efi.h b/include/linux/efi.h
> index bd9d83a94173..79b2d4de62e0 100644
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -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.

thanks,

Mimi

Re: [PATCH v4 1/3] efi: generalize efi_get_secureboot

[Ard Biesheuvel]

On Tue, 3 Nov 2020 at 19:49, Mimi Zohar <zohar@linux.ibm.com> wrote:
>
> On Mon, 2020-11-02 at 23:37 +0100, Ard Biesheuvel wrote:
> > From: Chester Lin <clin@suse.com>
> >
> > 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 <clin@suse.com>
> > Signed-off-by: Chester Lin <clin@suse.com>
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
>
> Thanks, Ard.  Other than one minor thing inline below, the patch looks
> good.  I haven't done any testing yet.
>
> > diff --git a/include/linux/efi.h b/include/linux/efi.h
> > index bd9d83a94173..79b2d4de62e0 100644
> > --- a/include/linux/efi.h
> > +++ b/include/linux/efi.h
> > @@ -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.
>

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

Re: [PATCH v4 1/3] efi: generalize efi_get_secureboot

[Mimi Zohar]

On Tue, 2020-11-03 at 20:01 +0100, Ard Biesheuvel wrote:
> > 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.

Got it.

thanks,

Mimi

[PATCH v4 2/3] ima: generalize x86/EFI arch glue for other EFI architectures

[Ard Biesheuvel]

From: Chester Lin <clin@suse.com>

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 <clin@suse.com>
Signed-off-by: Chester Lin <clin@suse.com>
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 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(-)

diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
index 7673dc833232..c98f78330b09 100644
--- a/arch/x86/include/asm/efi.h
+++ b/arch/x86/include/asm/efi.h
@@ -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 */
diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
index 68608bd892c0..5eeb808eb024 100644
--- a/arch/x86/kernel/Makefile
+++ b/arch/x86/kernel/Makefile
@@ -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
diff --git a/security/integrity/ima/Makefile b/security/integrity/ima/Makefile
index 67dabca670e2..2499f2485c04 100644
--- a/security/integrity/ima/Makefile
+++ b/security/integrity/ima/Makefile
@@ -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
diff --git a/arch/x86/kernel/ima_arch.c b/security/integrity/ima/ima_efi.c
similarity index 60%
rename from arch/x86/kernel/ima_arch.c
rename to security/integrity/ima/ima_efi.c
index 7dfb1e808928..233627a9d4b8 100644
--- a/arch/x86/kernel/ima_arch.c
+++ b/security/integrity/ima/ima_efi.c
@@ -5,50 +5,29 @@
 #include <linux/efi.h>
 #include <linux/module.h>
 #include <linux/ima.h>
+#include <asm/efi.h>
 
-extern struct boot_params boot_params;
+#ifndef arch_ima_efi_boot_mode
+#define arch_ima_efi_boot_mode efi_secureboot_mode_unknown
+#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

Re: [PATCH v4 2/3] ima: generalize x86/EFI arch glue for other EFI architectures

[Chester Lin]

Hi Ard,

On Mon, Nov 02, 2020 at 11:37:59PM +0100, Ard Biesheuvel wrote:
> From: Chester Lin <clin@suse.com>
> 
> 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 <clin@suse.com>
> Signed-off-by: Chester Lin <clin@suse.com>
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>  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(-)
> 
> diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
> index 7673dc833232..c98f78330b09 100644
> --- a/arch/x86/include/asm/efi.h
> +++ b/arch/x86/include/asm/efi.h
> @@ -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 */
> diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
> index 68608bd892c0..5eeb808eb024 100644
> --- a/arch/x86/kernel/Makefile
> +++ b/arch/x86/kernel/Makefile
> @@ -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
> diff --git a/security/integrity/ima/Makefile b/security/integrity/ima/Makefile
> index 67dabca670e2..2499f2485c04 100644
> --- a/security/integrity/ima/Makefile
> +++ b/security/integrity/ima/Makefile
> @@ -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
> diff --git a/arch/x86/kernel/ima_arch.c b/security/integrity/ima/ima_efi.c
> similarity index 60%
> rename from arch/x86/kernel/ima_arch.c
> rename to security/integrity/ima/ima_efi.c
> index 7dfb1e808928..233627a9d4b8 100644
> --- a/arch/x86/kernel/ima_arch.c
> +++ b/security/integrity/ima/ima_efi.c
> @@ -5,50 +5,29 @@
>  #include <linux/efi.h>
>  #include <linux/module.h>
>  #include <linux/ima.h>
> +#include <asm/efi.h>
>  
> -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.

Regards,
Chester

> +#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
> 

Re: [PATCH v4 2/3] ima: generalize x86/EFI arch glue for other EFI architectures

[Ard Biesheuvel]

On Fri, 6 Nov 2020 at 04:41, Chester Lin <clin@suse.com> wrote:
>
> Hi Ard,
>
> On Mon, Nov 02, 2020 at 11:37:59PM +0100, Ard Biesheuvel wrote:
> > From: Chester Lin <clin@suse.com>
> >
> > 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 <clin@suse.com>
> > Signed-off-by: Chester Lin <clin@suse.com>
> > Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> > ---
> >  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(-)
> >
> > diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
> > index 7673dc833232..c98f78330b09 100644
> > --- a/arch/x86/include/asm/efi.h
> > +++ b/arch/x86/include/asm/efi.h
> > @@ -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 */
> > diff --git a/arch/x86/kernel/Makefile b/arch/x86/kernel/Makefile
> > index 68608bd892c0..5eeb808eb024 100644
> > --- a/arch/x86/kernel/Makefile
> > +++ b/arch/x86/kernel/Makefile
> > @@ -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
> > diff --git a/security/integrity/ima/Makefile b/security/integrity/ima/Makefile
> > index 67dabca670e2..2499f2485c04 100644
> > --- a/security/integrity/ima/Makefile
> > +++ b/security/integrity/ima/Makefile
> > @@ -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
> > diff --git a/arch/x86/kernel/ima_arch.c b/security/integrity/ima/ima_efi.c
> > similarity index 60%
> > rename from arch/x86/kernel/ima_arch.c
> > rename to security/integrity/ima/ima_efi.c
> > index 7dfb1e808928..233627a9d4b8 100644
> > --- a/arch/x86/kernel/ima_arch.c
> > +++ b/security/integrity/ima/ima_efi.c
> > @@ -5,50 +5,29 @@
> >  #include <linux/efi.h>
> >  #include <linux/module.h>
> >  #include <linux/ima.h>
> > +#include <asm/efi.h>
> >
> > -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.
>

Thanks Chester! I will fix that up.


> > +#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
> >
>

[PATCH v4 3/3] arm64/ima: add ima_arch support

[Ard Biesheuvel]

From: Chester Lin <clin@suse.com>

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

Co-developed-by: Chester Lin <clin@suse.com>
Signed-off-by: Chester Lin <clin@suse.com>
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 arch/arm64/Kconfig | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index f858c352f72a..ea1b608a0fad 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -200,6 +200,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.
 
-- 
2.17.1

Re: [PATCH v4 3/3] arm64/ima: add ima_arch support

[Catalin Marinas]

On Mon, Nov 02, 2020 at 11:38:00PM +0100, Ard Biesheuvel wrote:
> From: Chester Lin <clin@suse.com>
> 
> Add arm64 IMA arch support. The code and arch policy is mainly inherited
> from x86.
> 
> Co-developed-by: Chester Lin <clin@suse.com>
> Signed-off-by: Chester Lin <clin@suse.com>
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>

Acked-by: Catalin Marinas <catalin.marinas@arm.com>

Re: [PATCH v4 0/3] wire up IMA secure boot for arm64

[Mimi Zohar]

Hi Ard, Chester,

On Mon, 2020-11-02 at 23:37 +0100, Ard Biesheuvel wrote:
> 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.

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

Mimi

Re: [PATCH v4 0/3] wire up IMA secure boot for arm64

[Ard Biesheuvel]

On Wed, 4 Nov 2020 at 19:20, Mimi Zohar <zohar@linux.ibm.com> wrote:
>
> Hi Ard, Chester,
>
> On Mon, 2020-11-02 at 23:37 +0100, Ard Biesheuvel wrote:
> > 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.
>
> 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).

Re: [PATCH v4 0/3] wire up IMA secure boot for arm64

[Mimi Zohar]

On Wed, 2020-11-04 at 19:50 +0100, Ard Biesheuvel wrote:
> On Wed, 4 Nov 2020 at 19:20, Mimi Zohar <zohar@linux.ibm.com> wrote:
> >
> > Hi Ard, Chester,
> >
> > On Mon, 2020-11-02 at 23:37 +0100, Ard Biesheuvel wrote:
> > > 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.
> >
> > 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).

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.

thanks,

Mimi

Re: [PATCH v4 0/3] wire up IMA secure boot for arm64

[Ard Biesheuvel]

On Wed, 4 Nov 2020 at 20:03, Mimi Zohar <zohar@linux.ibm.com> wrote:
>
> On Wed, 2020-11-04 at 19:50 +0100, Ard Biesheuvel wrote:
> > On Wed, 4 Nov 2020 at 19:20, Mimi Zohar <zohar@linux.ibm.com> wrote:
> > >
> > > Hi Ard, Chester,
> > >
> > > On Mon, 2020-11-02 at 23:37 +0100, Ard Biesheuvel wrote:
> > > > 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.
> > >
> > > 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).
>
> 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.
>

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.

Re: [PATCH v4 0/3] wire up IMA secure boot for arm64

[Mimi Zohar]

On Wed, 2020-11-04 at 20:12 +0100, Ard Biesheuvel wrote:

> > 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.

thanks,

Acked-by: Mimi Zohar <zohar@linux.ibm.com>

Re: [PATCH v4 0/3] wire up IMA secure boot for arm64

[Ard Biesheuvel]

On Wed, 4 Nov 2020 at 20:55, Mimi Zohar <zohar@linux.ibm.com> wrote:
>
> On Wed, 2020-11-04 at 20:12 +0100, Ard Biesheuvel wrote:
>
> > > 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.
>
> thanks,
>
> Acked-by: Mimi Zohar <zohar@linux.ibm.com>
>

Thanks Mimi