| Message ID | 20200904072905.25332-2-clin@suse.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | add ima_arch support for ARM64 | expand |
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 >
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 > > >
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 > > > > >
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.
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
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(-)