[PATCH 0/6] add ima_arch support for ARM64

[PATCH 0/6] add ima_arch support for ARM64

[Chester Lin]

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.

Chester Lin (6):
  efistub: pass uefi secureboot flag via fdt params
  efi/arm: a helper to parse secure boot param in fdt params
  efi: add secure boot flag
  efi/arm: check secure boot status in efi init
  arm64/ima: add ima arch support
  docs/arm: add the description of uefi-secure-boot param

 Documentation/arm/uefi.rst         |  2 ++
 arch/arm64/Kconfig                 |  1 +
 arch/arm64/kernel/Makefile         |  2 ++
 arch/arm64/kernel/ima_arch.c       | 37 ++++++++++++++++++++++++++++
 drivers/firmware/efi/arm-init.c    |  3 +++
 drivers/firmware/efi/fdtparams.c   | 23 ++++++++++++++++++
 drivers/firmware/efi/libstub/fdt.c | 39 +++++++++++++++++++++++++++++-
 include/linux/efi.h                |  2 ++
 8 files changed, 108 insertions(+), 1 deletion(-)
 create mode 100644 arch/arm64/kernel/ima_arch.c

-- 
2.26.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 1/6] efistub: pass uefi secureboot flag via fdt params

[Chester Lin]

Add a new UEFI parameter: "linux,uefi-secure-boot" in fdt boot params
as other architectures have done in their own boot data. For example,
the boot_params->secure_boot in x86.

Signed-off-by: Chester Lin <clin@suse.com>
---
 drivers/firmware/efi/libstub/fdt.c | 39 +++++++++++++++++++++++++++++-
 1 file changed, 38 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/efi/libstub/fdt.c b/drivers/firmware/efi/libstub/fdt.c
index 11ecf3c4640e..c9a341e4715f 100644
--- a/drivers/firmware/efi/libstub/fdt.c
+++ b/drivers/firmware/efi/libstub/fdt.c
@@ -136,6 +136,10 @@ static efi_status_t update_fdt(void *orig_fdt, unsigned long orig_fdt_size,
 	if (status)
 		goto fdt_set_fail;
 
+	status = fdt_setprop_var(fdt, node, "linux,uefi-secure-boot", fdt_val32);
+	if (status)
+		goto fdt_set_fail;
+
 	if (IS_ENABLED(CONFIG_RANDOMIZE_BASE)) {
 		efi_status_t efi_status;
 
@@ -199,6 +203,24 @@ static efi_status_t update_fdt_memmap(void *fdt, struct efi_boot_memmap *map)
 	return EFI_SUCCESS;
 }
 
+static efi_status_t update_fdt_secboot(void *fdt, u32 secboot)
+{
+	int node = fdt_path_offset(fdt, "/chosen");
+	u32 fdt_val32;
+	int err;
+
+	if (node < 0)
+		return EFI_LOAD_ERROR;
+
+	fdt_val32 = cpu_to_fdt32(secboot);
+
+	err = fdt_setprop_inplace_var(fdt, node, "linux,uefi-secure-boot", fdt_val32);
+	if (err)
+		return EFI_LOAD_ERROR;
+
+	return EFI_SUCCESS;
+}
+
 struct exit_boot_struct {
 	efi_memory_desc_t	*runtime_map;
 	int			*runtime_entry_count;
@@ -208,6 +230,9 @@ struct exit_boot_struct {
 static efi_status_t exit_boot_func(struct efi_boot_memmap *map,
 				   void *priv)
 {
+	efi_status_t status;
+	enum efi_secureboot_mode secboot_status;
+	u32 secboot_var = 0;
 	struct exit_boot_struct *p = priv;
 	/*
 	 * Update the memory map with virtual addresses. The function will also
@@ -217,7 +242,19 @@ static efi_status_t exit_boot_func(struct efi_boot_memmap *map,
 	efi_get_virtmap(*map->map, *map->map_size, *map->desc_size,
 			p->runtime_map, p->runtime_entry_count);
 
-	return update_fdt_memmap(p->new_fdt_addr, map);
+	status = update_fdt_memmap(p->new_fdt_addr, map);
+
+	if (status != EFI_SUCCESS)
+		return status;
+
+	secboot_status = efi_get_secureboot();
+
+	if (secboot_status == efi_secureboot_mode_enabled)
+		secboot_var = 1;
+
+	status = update_fdt_secboot(p->new_fdt_addr, secboot_var);
+
+	return status;
 }
 
 #ifndef MAX_FDT_SIZE
-- 
2.26.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 2/6] efi/arm: a helper to parse secure boot param in fdt params

[Chester Lin]

Add a helper to query the UEFI secureboot param from the chosen node in
FDT.

Signed-off-by: Chester Lin <clin@suse.com>
---
 drivers/firmware/efi/fdtparams.c | 23 +++++++++++++++++++++++
 include/linux/efi.h              |  1 +
 2 files changed, 24 insertions(+)

diff --git a/drivers/firmware/efi/fdtparams.c b/drivers/firmware/efi/fdtparams.c
index bb042ab7c2be..d58ec4119bcf 100644
--- a/drivers/firmware/efi/fdtparams.c
+++ b/drivers/firmware/efi/fdtparams.c
@@ -124,3 +124,26 @@ u64 __init efi_get_fdt_params(struct efi_memory_map_data *mm)
 	pr_info("UEFI not found.\n");
 	return 0;
 }
+
+bool __init efi_secureboot_enabled_in_fdt(void)
+{
+	const void *fdt = initial_boot_params;
+	int node;
+	u32 secboot;
+
+
+	node = fdt_path_offset(fdt, "/chosen");
+
+	if (node < 0) {
+		pr_err("chosen node not found.\n");
+		return false;
+	}
+
+	if (!efi_get_fdt_prop(fdt, node, "linux,uefi-secure-boot",
+			      "SECURE BOOT", &secboot, sizeof(secboot))) {
+		if (secboot)
+			return true;
+	}
+
+	return false;
+}
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 73db1ae04cef..315126b2f5e9 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -663,6 +663,7 @@ extern int efi_mem_reserve_persistent(phys_addr_t addr, u64 size);
 extern void efi_initialize_iomem_resources(struct resource *code_resource,
 		struct resource *data_resource, struct resource *bss_resource);
 extern u64 efi_get_fdt_params(struct efi_memory_map_data *data);
+extern bool __init efi_secureboot_enabled_in_fdt(void);
 extern struct kobject *efi_kobj;
 
 extern int efi_reboot_quirk_mode;
-- 
2.26.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 3/6] efi: add secure boot flag

[Chester Lin]

Add a new EFI flag to indicate whether secure boot is enabled by UEFI
firmware or not.

Signed-off-by: Chester Lin <clin@suse.com>
---
 include/linux/efi.h | 1 +
 1 file changed, 1 insertion(+)

diff --git a/include/linux/efi.h b/include/linux/efi.h
index 315126b2f5e9..82a19bb0237a 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -784,6 +784,7 @@ extern int __init efi_setup_pcdp_console(char *);
 #define EFI_MEM_ATTR		10	/* Did firmware publish an EFI_MEMORY_ATTRIBUTES table? */
 #define EFI_MEM_NO_SOFT_RESERVE	11	/* Is the kernel configured to ignore soft reservations? */
 #define EFI_PRESERVE_BS_REGIONS	12	/* Are EFI boot-services memory segments available? */
+#define EFI_SECURE_BOOT		13	/* Is EFI secure-boot enabled? */
 
 #ifdef CONFIG_EFI
 /*
-- 
2.26.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 4/6] efi/arm: check secure boot status in efi init

[Chester Lin]

set EFI_SECURE_BOOT flag when UEFI secure boot is eanbled on ARM.

Signed-off-by: Chester Lin <clin@suse.com>
---
 drivers/firmware/efi/arm-init.c | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/drivers/firmware/efi/arm-init.c b/drivers/firmware/efi/arm-init.c
index 71c445d20258..70f2eaf5fb1a 100644
--- a/drivers/firmware/efi/arm-init.c
+++ b/drivers/firmware/efi/arm-init.c
@@ -234,6 +234,9 @@ void __init efi_init(void)
 		return;
 	}
 
+	if (efi_secureboot_enabled_in_fdt())
+		set_bit(EFI_SECURE_BOOT, &efi.flags);
+
 	reserve_regions();
 	efi_esrt_init();
 
-- 
2.26.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 5/6] arm64/ima: add ima arch support

[Chester Lin]

Add arm64 IMA arch support. The arch policy is 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 | 37 ++++++++++++++++++++++++++++++++++++
 3 files changed, 40 insertions(+)
 create mode 100644 arch/arm64/kernel/ima_arch.c

diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
index 6d232837cbee..b5518e7b604d 100644
--- a/arch/arm64/Kconfig
+++ b/arch/arm64/Kconfig
@@ -196,6 +196,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 a561cbb91d4d..0300ab60785d 100644
--- a/arch/arm64/kernel/Makefile
+++ b/arch/arm64/kernel/Makefile
@@ -71,3 +71,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..46f5641c3da5
--- /dev/null
+++ b/arch/arm64/kernel/ima_arch.c
@@ -0,0 +1,37 @@
+/* SPDX-License-Identifier: GPL-2.0+ */
+/*
+ * Copyright (C) 2018 IBM Corporation
+ */
+#include <linux/efi.h>
+#include <linux/module.h>
+
+bool arch_ima_get_secureboot(void)
+{
+	if (efi_enabled(EFI_SECURE_BOOT))
+		return true;
+
+	return false;
+}
+
+/* secureboot 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


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

[PATCH 6/6] docs/arm: add the description of uefi-secure-boot param

[Chester Lin]

Add the description of "linux,uefi-secure-boot" param.

Signed-off-by: Chester Lin <clin@suse.com>
---
 Documentation/arm/uefi.rst | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/Documentation/arm/uefi.rst b/Documentation/arm/uefi.rst
index f868330df6be..7d9c6a1697af 100644
--- a/Documentation/arm/uefi.rst
+++ b/Documentation/arm/uefi.rst
@@ -64,4 +64,6 @@ linux,uefi-mmap-desc-size   32-bit   Size in bytes of each entry in the UEFI
                                      memory map.
 
 linux,uefi-mmap-desc-ver    32-bit   Version of the mmap descriptor format.
+
+linux,uefi-secure-boot      32-bit   UEFI Secure Boot [0: Disabled 1: Enabled]
 ==========================  ======   ===========================================
-- 
2.26.1


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 5/6] arm64/ima: add ima arch support

[Mimi Zohar]

On Fri, 2020-09-04 at 15:29 +0800, Chester Lin wrote:
> Add arm64 IMA arch support. The arch policy is inherited from x86.
> 
> Signed-off-by: Chester Lin <clin@suse.com>

The "secureboot arch rules" comment should be updated to reflect that
the policy is both "secure and trusted boot arch rules", both here and
in x86.

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

thanks,

Mimi

> ---
>  arch/arm64/Kconfig           |  1 +
>  arch/arm64/kernel/Makefile   |  2 ++
>  arch/arm64/kernel/ima_arch.c | 37 ++++++++++++++++++++++++++++++++++++
>  3 files changed, 40 insertions(+)
>  create mode 100644 arch/arm64/kernel/ima_arch.c
> 
> diff --git a/arch/arm64/Kconfig b/arch/arm64/Kconfig
> index 6d232837cbee..b5518e7b604d 100644
> --- a/arch/arm64/Kconfig
> +++ b/arch/arm64/Kconfig
> @@ -196,6 +196,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 a561cbb91d4d..0300ab60785d 100644
> --- a/arch/arm64/kernel/Makefile
> +++ b/arch/arm64/kernel/Makefile
> @@ -71,3 +71,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..46f5641c3da5
> --- /dev/null
> +++ b/arch/arm64/kernel/ima_arch.c
> @@ -0,0 +1,37 @@
> +/* SPDX-License-Identifier: GPL-2.0+ */
> +/*
> + * Copyright (C) 2018 IBM Corporation
> + */
> +#include <linux/efi.h>
> +#include <linux/module.h>
> +
> +bool arch_ima_get_secureboot(void)
> +{
> +	if (efi_enabled(EFI_SECURE_BOOT))
> +		return true;
> +
> +	return false;
> +}
> +
> +/* secureboot 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;
> +}



_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/6] efistub: pass uefi secureboot flag via fdt params

[Ard Biesheuvel]

On Fri, 4 Sep 2020 at 10:29, Chester Lin <clin@suse.com> wrote:
>
> Add a new UEFI parameter: "linux,uefi-secure-boot" in fdt boot params
> as other architectures have done in their own boot data. For example,
> the boot_params->secure_boot in x86.
>
> Signed-off-by: Chester Lin <clin@suse.com>

Why do we need this flag? Can't the OS simply check the variable directly?

> ---
>  drivers/firmware/efi/libstub/fdt.c | 39 +++++++++++++++++++++++++++++-
>  1 file changed, 38 insertions(+), 1 deletion(-)
>
> diff --git a/drivers/firmware/efi/libstub/fdt.c b/drivers/firmware/efi/libstub/fdt.c
> index 11ecf3c4640e..c9a341e4715f 100644
> --- a/drivers/firmware/efi/libstub/fdt.c
> +++ b/drivers/firmware/efi/libstub/fdt.c
> @@ -136,6 +136,10 @@ static efi_status_t update_fdt(void *orig_fdt, unsigned long orig_fdt_size,
>         if (status)
>                 goto fdt_set_fail;
>
> +       status = fdt_setprop_var(fdt, node, "linux,uefi-secure-boot", fdt_val32);
> +       if (status)
> +               goto fdt_set_fail;
> +
>         if (IS_ENABLED(CONFIG_RANDOMIZE_BASE)) {
>                 efi_status_t efi_status;
>
> @@ -199,6 +203,24 @@ static efi_status_t update_fdt_memmap(void *fdt, struct efi_boot_memmap *map)
>         return EFI_SUCCESS;
>  }
>
> +static efi_status_t update_fdt_secboot(void *fdt, u32 secboot)
> +{
> +       int node = fdt_path_offset(fdt, "/chosen");
> +       u32 fdt_val32;
> +       int err;
> +
> +       if (node < 0)
> +               return EFI_LOAD_ERROR;
> +
> +       fdt_val32 = cpu_to_fdt32(secboot);
> +
> +       err = fdt_setprop_inplace_var(fdt, node, "linux,uefi-secure-boot", fdt_val32);
> +       if (err)
> +               return EFI_LOAD_ERROR;
> +
> +       return EFI_SUCCESS;
> +}
> +
>  struct exit_boot_struct {
>         efi_memory_desc_t       *runtime_map;
>         int                     *runtime_entry_count;
> @@ -208,6 +230,9 @@ struct exit_boot_struct {
>  static efi_status_t exit_boot_func(struct efi_boot_memmap *map,
>                                    void *priv)
>  {
> +       efi_status_t status;
> +       enum efi_secureboot_mode secboot_status;
> +       u32 secboot_var = 0;
>         struct exit_boot_struct *p = priv;
>         /*
>          * Update the memory map with virtual addresses. The function will also
> @@ -217,7 +242,19 @@ static efi_status_t exit_boot_func(struct efi_boot_memmap *map,
>         efi_get_virtmap(*map->map, *map->map_size, *map->desc_size,
>                         p->runtime_map, p->runtime_entry_count);
>
> -       return update_fdt_memmap(p->new_fdt_addr, map);
> +       status = update_fdt_memmap(p->new_fdt_addr, map);
> +
> +       if (status != EFI_SUCCESS)
> +               return status;
> +
> +       secboot_status = efi_get_secureboot();
> +
> +       if (secboot_status == efi_secureboot_mode_enabled)
> +               secboot_var = 1;
> +
> +       status = update_fdt_secboot(p->new_fdt_addr, secboot_var);
> +
> +       return status;
>  }
>
>  #ifndef MAX_FDT_SIZE
> --
> 2.26.1
>

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/6] efistub: pass uefi secureboot flag via fdt params

[Chester Lin]

Hi Ard,

On Fri, Sep 11, 2020 at 06:01:09PM +0300, Ard Biesheuvel wrote:
> On Fri, 4 Sep 2020 at 10:29, Chester Lin <clin@suse.com> wrote:
> >
> > Add a new UEFI parameter: "linux,uefi-secure-boot" in fdt boot params
> > as other architectures have done in their own boot data. For example,
> > the boot_params->secure_boot in x86.
> >
> > Signed-off-by: Chester Lin <clin@suse.com>
> 
> Why do we need this flag? Can't the OS simply check the variable directly?
> 

In fact, there's a difficulty to achieve this.

When linux kernel is booting on ARM, the runtime services are enabled later on.
It's done by arm_enable_runtime_services(), which is registered as an early_initcall.
Before it calls efi_native_runtime_setup(), all EFI runtime callbacks are still
NULL so calling efi.get_variable() will cause NULL pointer dereference.

There's a case that arch_ima_get_secureboot() can be called in early boot stage.
For example, when you try to set "ima_appraise=off" in kernel command line, it's
actually handled early:

[    0.000000] Kernel command line: BOOT_IMAGE=/boot/Image-5.9.0-rc3-9.gdd61cda-
vanilla root=UUID=a88bfb80-8abb-425c-a0f3-ad317465c28b splash=silent mitigations
=auto ignore_loglevel earlycon=pl011,mmio,0x9000000 console=ttyAMA0 ima_appraise=off
[    0.000000] ima: Secure boot enabled: ignoring ima_appraise=off boot parameter option
[    0.000000] Dentry cache hash table entries: 1048576 (order: 11, 8388608 bytes, linear)

However EFI services are remapped and enabled afterwards.

[    0.082286] rcu: Hierarchical SRCU implementation.
[    0.089592] Remapping and enabling EFI services.
[    0.097509] smp: Bringing up secondary CPUs ...

Another problem is that efi_rts_wq is created in subsys_initcall so we have to
wait for both EFI services mapping and the workqueue get initiated before calling
efi.get_variable() on ARM.

The only way I can think of is to put a flag via fdt params. May I have your
suggestions? I will appreciate if there's any better approach.

Thanks,
Chester

> > ---
> >  drivers/firmware/efi/libstub/fdt.c | 39 +++++++++++++++++++++++++++++-
> >  1 file changed, 38 insertions(+), 1 deletion(-)
> >
> > diff --git a/drivers/firmware/efi/libstub/fdt.c b/drivers/firmware/efi/libstub/fdt.c
> > index 11ecf3c4640e..c9a341e4715f 100644
> > --- a/drivers/firmware/efi/libstub/fdt.c
> > +++ b/drivers/firmware/efi/libstub/fdt.c
> > @@ -136,6 +136,10 @@ static efi_status_t update_fdt(void *orig_fdt, unsigned long orig_fdt_size,
> >         if (status)
> >                 goto fdt_set_fail;
> >
> > +       status = fdt_setprop_var(fdt, node, "linux,uefi-secure-boot", fdt_val32);
> > +       if (status)
> > +               goto fdt_set_fail;
> > +
> >         if (IS_ENABLED(CONFIG_RANDOMIZE_BASE)) {
> >                 efi_status_t efi_status;
> >
> > @@ -199,6 +203,24 @@ static efi_status_t update_fdt_memmap(void *fdt, struct efi_boot_memmap *map)
> >         return EFI_SUCCESS;
> >  }
> >
> > +static efi_status_t update_fdt_secboot(void *fdt, u32 secboot)
> > +{
> > +       int node = fdt_path_offset(fdt, "/chosen");
> > +       u32 fdt_val32;
> > +       int err;
> > +
> > +       if (node < 0)
> > +               return EFI_LOAD_ERROR;
> > +
> > +       fdt_val32 = cpu_to_fdt32(secboot);
> > +
> > +       err = fdt_setprop_inplace_var(fdt, node, "linux,uefi-secure-boot", fdt_val32);
> > +       if (err)
> > +               return EFI_LOAD_ERROR;
> > +
> > +       return EFI_SUCCESS;
> > +}
> > +
> >  struct exit_boot_struct {
> >         efi_memory_desc_t       *runtime_map;
> >         int                     *runtime_entry_count;
> > @@ -208,6 +230,9 @@ struct exit_boot_struct {
> >  static efi_status_t exit_boot_func(struct efi_boot_memmap *map,
> >                                    void *priv)
> >  {
> > +       efi_status_t status;
> > +       enum efi_secureboot_mode secboot_status;
> > +       u32 secboot_var = 0;
> >         struct exit_boot_struct *p = priv;
> >         /*
> >          * Update the memory map with virtual addresses. The function will also
> > @@ -217,7 +242,19 @@ static efi_status_t exit_boot_func(struct efi_boot_memmap *map,
> >         efi_get_virtmap(*map->map, *map->map_size, *map->desc_size,
> >                         p->runtime_map, p->runtime_entry_count);
> >
> > -       return update_fdt_memmap(p->new_fdt_addr, map);
> > +       status = update_fdt_memmap(p->new_fdt_addr, map);
> > +
> > +       if (status != EFI_SUCCESS)
> > +               return status;
> > +
> > +       secboot_status = efi_get_secureboot();
> > +
> > +       if (secboot_status == efi_secureboot_mode_enabled)
> > +               secboot_var = 1;
> > +
> > +       status = update_fdt_secboot(p->new_fdt_addr, secboot_var);
> > +
> > +       return status;
> >  }
> >
> >  #ifndef MAX_FDT_SIZE
> > --
> > 2.26.1
> >
> 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/6] efistub: pass uefi secureboot flag via fdt params

[Chester Lin]

On Mon, Sep 14, 2020 at 04:05:22PM +0800, Chester Lin wrote:
> Hi Ard,
> 
> On Fri, Sep 11, 2020 at 06:01:09PM +0300, Ard Biesheuvel wrote:
> > On Fri, 4 Sep 2020 at 10:29, Chester Lin <clin@suse.com> wrote:
> > >
> > > Add a new UEFI parameter: "linux,uefi-secure-boot" in fdt boot params
> > > as other architectures have done in their own boot data. For example,
> > > the boot_params->secure_boot in x86.
> > >
> > > Signed-off-by: Chester Lin <clin@suse.com>
> > 
> > Why do we need this flag? Can't the OS simply check the variable directly?
> > 
> 
> In fact, there's a difficulty to achieve this.
> 
> When linux kernel is booting on ARM, the runtime services are enabled later on.
> It's done by arm_enable_runtime_services(), which is registered as an early_initcall.
> Before it calls efi_native_runtime_setup(), all EFI runtime callbacks are still
> NULL so calling efi.get_variable() will cause NULL pointer dereference.
> 
> There's a case that arch_ima_get_secureboot() can be called in early boot stage.
> For example, when you try to set "ima_appraise=off" in kernel command line, it's
> actually handled early:
> 
> [    0.000000] Kernel command line: BOOT_IMAGE=/boot/Image-5.9.0-rc3-9.gdd61cda-
> vanilla root=UUID=a88bfb80-8abb-425c-a0f3-ad317465c28b splash=silent mitigations
> =auto ignore_loglevel earlycon=pl011,mmio,0x9000000 console=ttyAMA0 ima_appraise=off
> [    0.000000] ima: Secure boot enabled: ignoring ima_appraise=off boot parameter option
> [    0.000000] Dentry cache hash table entries: 1048576 (order: 11, 8388608 bytes, linear)
> 
> However EFI services are remapped and enabled afterwards.
> 
> [    0.082286] rcu: Hierarchical SRCU implementation.
> [    0.089592] Remapping and enabling EFI services.
> [    0.097509] smp: Bringing up secondary CPUs ...
> 
> Another problem is that efi_rts_wq is created in subsys_initcall so we have to
> wait for both EFI services mapping and the workqueue get initiated before calling
> efi.get_variable() on ARM.
> 
> The only way I can think of is to put a flag via fdt params. May I have your
> suggestions? I will appreciate if there's any better approach.
> 
> Thanks,
> Chester

Ping. May I have some suggestions here?

Thanks,
Chester

> 
> > > ---
> > >  drivers/firmware/efi/libstub/fdt.c | 39 +++++++++++++++++++++++++++++-
> > >  1 file changed, 38 insertions(+), 1 deletion(-)
> > >
> > > diff --git a/drivers/firmware/efi/libstub/fdt.c b/drivers/firmware/efi/libstub/fdt.c
> > > index 11ecf3c4640e..c9a341e4715f 100644
> > > --- a/drivers/firmware/efi/libstub/fdt.c
> > > +++ b/drivers/firmware/efi/libstub/fdt.c
> > > @@ -136,6 +136,10 @@ static efi_status_t update_fdt(void *orig_fdt, unsigned long orig_fdt_size,
> > >         if (status)
> > >                 goto fdt_set_fail;
> > >
> > > +       status = fdt_setprop_var(fdt, node, "linux,uefi-secure-boot", fdt_val32);
> > > +       if (status)
> > > +               goto fdt_set_fail;
> > > +
> > >         if (IS_ENABLED(CONFIG_RANDOMIZE_BASE)) {
> > >                 efi_status_t efi_status;
> > >
> > > @@ -199,6 +203,24 @@ static efi_status_t update_fdt_memmap(void *fdt, struct efi_boot_memmap *map)
> > >         return EFI_SUCCESS;
> > >  }
> > >
> > > +static efi_status_t update_fdt_secboot(void *fdt, u32 secboot)
> > > +{
> > > +       int node = fdt_path_offset(fdt, "/chosen");
> > > +       u32 fdt_val32;
> > > +       int err;
> > > +
> > > +       if (node < 0)
> > > +               return EFI_LOAD_ERROR;
> > > +
> > > +       fdt_val32 = cpu_to_fdt32(secboot);
> > > +
> > > +       err = fdt_setprop_inplace_var(fdt, node, "linux,uefi-secure-boot", fdt_val32);
> > > +       if (err)
> > > +               return EFI_LOAD_ERROR;
> > > +
> > > +       return EFI_SUCCESS;
> > > +}
> > > +
> > >  struct exit_boot_struct {
> > >         efi_memory_desc_t       *runtime_map;
> > >         int                     *runtime_entry_count;
> > > @@ -208,6 +230,9 @@ struct exit_boot_struct {
> > >  static efi_status_t exit_boot_func(struct efi_boot_memmap *map,
> > >                                    void *priv)
> > >  {
> > > +       efi_status_t status;
> > > +       enum efi_secureboot_mode secboot_status;
> > > +       u32 secboot_var = 0;
> > >         struct exit_boot_struct *p = priv;
> > >         /*
> > >          * Update the memory map with virtual addresses. The function will also
> > > @@ -217,7 +242,19 @@ static efi_status_t exit_boot_func(struct efi_boot_memmap *map,
> > >         efi_get_virtmap(*map->map, *map->map_size, *map->desc_size,
> > >                         p->runtime_map, p->runtime_entry_count);
> > >
> > > -       return update_fdt_memmap(p->new_fdt_addr, map);
> > > +       status = update_fdt_memmap(p->new_fdt_addr, map);
> > > +
> > > +       if (status != EFI_SUCCESS)
> > > +               return status;
> > > +
> > > +       secboot_status = efi_get_secureboot();
> > > +
> > > +       if (secboot_status == efi_secureboot_mode_enabled)
> > > +               secboot_var = 1;
> > > +
> > > +       status = update_fdt_secboot(p->new_fdt_addr, secboot_var);
> > > +
> > > +       return status;
> > >  }
> > >
> > >  #ifndef MAX_FDT_SIZE
> > > --
> > > 2.26.1
> > >
> > 


_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel

Re: [PATCH 1/6] efistub: pass uefi secureboot flag via fdt params

[Ard Biesheuvel]

On Mon, 5 Oct 2020 at 04:20, Chester Lin <clin@suse.com> wrote:
>
> On Mon, Sep 14, 2020 at 04:05:22PM +0800, Chester Lin wrote:
> > Hi Ard,
> >
> > On Fri, Sep 11, 2020 at 06:01:09PM +0300, Ard Biesheuvel wrote:
> > > On Fri, 4 Sep 2020 at 10:29, Chester Lin <clin@suse.com> wrote:
> > > >
> > > > Add a new UEFI parameter: "linux,uefi-secure-boot" in fdt boot params
> > > > as other architectures have done in their own boot data. For example,
> > > > the boot_params->secure_boot in x86.
> > > >
> > > > Signed-off-by: Chester Lin <clin@suse.com>
> > >
> > > Why do we need this flag? Can't the OS simply check the variable directly?
> > >
> >
> > In fact, there's a difficulty to achieve this.
> >
> > When linux kernel is booting on ARM, the runtime services are enabled later on.
> > It's done by arm_enable_runtime_services(), which is registered as an early_initcall.
> > Before it calls efi_native_runtime_setup(), all EFI runtime callbacks are still
> > NULL so calling efi.get_variable() will cause NULL pointer dereference.
> >
> > There's a case that arch_ima_get_secureboot() can be called in early boot stage.
> > For example, when you try to set "ima_appraise=off" in kernel command line, it's
> > actually handled early:
> >
> > [    0.000000] Kernel command line: BOOT_IMAGE=/boot/Image-5.9.0-rc3-9.gdd61cda-
> > vanilla root=UUID=a88bfb80-8abb-425c-a0f3-ad317465c28b splash=silent mitigations
> > =auto ignore_loglevel earlycon=pl011,mmio,0x9000000 console=ttyAMA0 ima_appraise=off
> > [    0.000000] ima: Secure boot enabled: ignoring ima_appraise=off boot parameter option
> > [    0.000000] Dentry cache hash table entries: 1048576 (order: 11, 8388608 bytes, linear)
> >
> > However EFI services are remapped and enabled afterwards.
> >
> > [    0.082286] rcu: Hierarchical SRCU implementation.
> > [    0.089592] Remapping and enabling EFI services.
> > [    0.097509] smp: Bringing up secondary CPUs ...
> >
> > Another problem is that efi_rts_wq is created in subsys_initcall so we have to
> > wait for both EFI services mapping and the workqueue get initiated before calling
> > efi.get_variable() on ARM.
> >
> > The only way I can think of is to put a flag via fdt params. May I have your
> > suggestions? I will appreciate if there's any better approach.
> >
> > Thanks,
> > Chester
>
> Ping. May I have some suggestions here?
>

IMA itself is initialized as a late initcall. The only reason you see
this message early is because this is where the parsing of the command
line parameter happens.

I'll send out a patch with a proposed solution for this issue.

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel