Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1556533 > unrolled thread

Re: [PATCH 5/8] efi: Get the secure boot status [ver #6]

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  Re: [PATCH 5/8] efi: Get the secure boot status [ver #6] Matt Fleming <matt@codeblueprint.co.uk> - 2017-01-11 15:40 +0100
    Re: [PATCH 5/8] efi: Get the secure boot status [ver #6] David Howells <dhowells@redhat.com> - 2017-01-11 16:40 +0100
      Re: [PATCH 5/8] efi: Get the secure boot status [ver #6] Matt Fleming <matt@codeblueprint.co.uk> - 2017-01-16 16:00 +0100
        Re: [PATCH 5/8] efi: Get the secure boot status [ver #6] David Howells <dhowells@redhat.com> - 2017-01-16 16:40 +0100

#1556533 — Re: [PATCH 5/8] efi: Get the secure boot status [ver #6]

On Thu, 08 Dec, at 12:30:45PM, David Howells wrote:
> Get the firmware's secure-boot status in the kernel boot wrapper and stash
> it somewhere that the main kernel image can find.
> 
> The efi_get_secureboot() function is extracted from the arm stub and (a)
> generalised so that it can be called from x86 and (b) made to use
> efi_call_runtime() so that it can be run in mixed-mode.
> 
> Suggested-by: Lukas Wunner <lukas@wunner.de>
> Signed-off-by: David Howells <dhowells@redhat.com>
> ---
> 
>  Documentation/x86/zero-page.txt           |    2 +
>  arch/x86/boot/compressed/eboot.c          |    2 +
>  arch/x86/boot/compressed/head_32.S        |    1 
>  arch/x86/boot/compressed/head_64.S        |    1 
>  arch/x86/include/asm/bootparam_utils.h    |    5 +-
>  arch/x86/include/uapi/asm/bootparam.h     |    3 +
>  arch/x86/kernel/asm-offsets.c             |    1 
>  drivers/firmware/efi/libstub/Makefile     |    2 -
>  drivers/firmware/efi/libstub/arm-stub.c   |   63 +++--------------------------
>  drivers/firmware/efi/libstub/secureboot.c |   63 +++++++++++++++++++++++++++++
>  include/linux/efi.h                       |    8 ++++
>  11 files changed, 90 insertions(+), 61 deletions(-)
>  create mode 100644 drivers/firmware/efi/libstub/secureboot.c
 
[...]

> diff --git a/arch/x86/boot/compressed/head_32.S b/arch/x86/boot/compressed/head_32.S
> index d85b9625e836..c635f7e32f5c 100644
> --- a/arch/x86/boot/compressed/head_32.S
> +++ b/arch/x86/boot/compressed/head_32.S
> @@ -61,6 +61,7 @@
>  
>  	__HEAD
>  ENTRY(startup_32)
> +	movb	$0, BP_secure_boot(%esi)
>  #ifdef CONFIG_EFI_STUB
>  	jmp	preferred_addr
>  
> diff --git a/arch/x86/boot/compressed/head_64.S b/arch/x86/boot/compressed/head_64.S
> index beab8322f72a..ccd2c7461b7f 100644
> --- a/arch/x86/boot/compressed/head_64.S
> +++ b/arch/x86/boot/compressed/head_64.S
> @@ -244,6 +244,7 @@ ENTRY(startup_64)
>  	 * that maps our entire kernel(text+data+bss+brk), zero page
>  	 * and command line.
>  	 */
> +	movb	$0, BP_secure_boot(%rsi)
>  #ifdef CONFIG_EFI_STUB
>  	/*
>  	 * The entry point for the PE/COFF executable is efi_pe_entry, so

Is clearing ::secure_boot really necessary? Any code path that goes
via efi_main() will set it correctly and all other code paths should
get it cleared in sanitize_boot_params(), no?

> diff --git a/include/linux/efi.h b/include/linux/efi.h
> index c7904556d7a8..92e23f03045e 100644
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -1477,6 +1477,14 @@ efi_status_t efi_setup_gop(efi_system_table_t *sys_table_arg,
>  bool efi_runtime_disabled(void);
>  extern void efi_call_virt_check_flags(unsigned long flags, const char *call);
>  
> +enum efi_secureboot_mode {
> +	efi_secureboot_mode_unset,
> +	efi_secureboot_mode_unknown,
> +	efi_secureboot_mode_disabled,
> +	efi_secureboot_mode_enabled,
> +};
> +enum efi_secureboot_mode efi_get_secureboot(efi_system_table_t *sys_table);
> +
>  /*
>   * Arch code can implement the following three template macros, avoiding
>   * reptition for the void/non-void return cases of {__,}efi_call_virt():
> 

What's the distinction between the unset and unknown enums?

[toc] | [next] | [standalone]

#1556634

Matt Fleming <matt@codeblueprint.co.uk> wrote:

> > +	movb	$0, BP_secure_boot(%rsi)
> >  #ifdef CONFIG_EFI_STUB
> >  	/*
> >  	 * The entry point for the PE/COFF executable is efi_pe_entry, so
> 
> Is clearing ::secure_boot really necessary? Any code path that goes
> via efi_main() will set it correctly and all other code paths should
> get it cleared in sanitize_boot_params(), no?

No.

The boot_params->secure_boot parameter exists whether or not efi_main() is
traversed (ie. if EFI isn't enabled or CONFIG_EFI_STUB=n) and, if not cleared,
is of uncertain value.

Further, sanitize_boot_params() has to be modified by this patch so as not to
clobber the secure_boot flag.

> What's the distinction between the unset and unknown enums?

unset -> The flag was cleared by head.S and efi_get_secureboot() was never
called.

unknown -> efi_get_secureboot() tried and failed to access the EFI variables
that should give the state.

David

[toc] | [prev] | [next] | [standalone]

#1559809

(Cc'ing Peter A. and Peter J. for boot params discussion)

On Wed, 11 Jan, at 03:27:23PM, David Howells wrote:
> Matt Fleming <matt@codeblueprint.co.uk> wrote:
> 
> > > +	movb	$0, BP_secure_boot(%rsi)
> > >  #ifdef CONFIG_EFI_STUB
> > >  	/*
> > >  	 * The entry point for the PE/COFF executable is efi_pe_entry, so
> > 
> > Is clearing ::secure_boot really necessary? Any code path that goes
> > via efi_main() will set it correctly and all other code paths should
> > get it cleared in sanitize_boot_params(), no?
> 
> No.
> 
> The boot_params->secure_boot parameter exists whether or not efi_main() is
> traversed (ie. if EFI isn't enabled or CONFIG_EFI_STUB=n) and, if not cleared,
> is of uncertain value.
>
> Further, sanitize_boot_params() has to be modified by this patch so as not to
> clobber the secure_boot flag.

Any new parameters that boot loaders do not know about should be
cleared to zero by default in the boot loader because boot_params
itself should be zero'd when allocated.

There are two cases to consider:

 1) boot_params is not zero'd
 2) boot_params is zero'd

1) This is a broken boot loader implementation that violates the x86
boot specification and I would never expect ->secure_boot to have a
valid value. It should not be special-cased in sanitize_boot_params(),
it should be zero'd.

2) In this case ->secure_boot should be zero unless modified inside of
efi_main().

Did you hit the scenario where ->secure_boot has a garbage value while
developing these patches? I wouldn't expect to see it in practice.

[toc] | [prev] | [next] | [standalone]

#1559842

Matt Fleming <matt@codeblueprint.co.uk> wrote:

> On Wed, 11 Jan, at 03:27:23PM, David Howells wrote:
> > Matt Fleming <matt@codeblueprint.co.uk> wrote:
> > 
> > > > +	movb	$0, BP_secure_boot(%rsi)
> > > >  #ifdef CONFIG_EFI_STUB
> > > >  	/*
> > > >  	 * The entry point for the PE/COFF executable is efi_pe_entry, so
> > > 
> > > Is clearing ::secure_boot really necessary? Any code path that goes
> > > via efi_main() will set it correctly and all other code paths should
> > > get it cleared in sanitize_boot_params(), no?
> > 
> > No.
> > 
> > The boot_params->secure_boot parameter exists whether or not efi_main() is
> > traversed (ie. if EFI isn't enabled or CONFIG_EFI_STUB=n) and, if not cleared,
> > is of uncertain value.
> >
> > Further, sanitize_boot_params() has to be modified by this patch so as not to
> > clobber the secure_boot flag.
> 
> Any new parameters that boot loaders do not know about should be
> cleared to zero by default in the boot loader because boot_params
> itself should be zero'd when allocated.

Do you mean the boot loader or the boot wrapper?  If the loader, that is
outside my control - and given the purpose of the value, I'm not sure I
want to rely on that.

> There are two cases to consider:
> 
>  1) boot_params is not zero'd
>  2) boot_params is zero'd
> 
> 1) This is a broken boot loader implementation that violates the x86
> boot specification and I would never expect ->secure_boot to have a
> valid value.

If there's a boot specification that must be complied with, why does
sanitize_boot_params() even exist?  Why does the comment on it say:

 * Deal with bootloaders which fail to initialize unknown fields in
 * boot_params to zero.  The list fields in this list are taken from
 * analysis of kexec-tools; if other broken bootloaders initialize a
 * different set of fields we will need to figure out how to disambiguate.

> It should not be special-cased in sanitize_boot_params(), it should be
> zero'd.

Sigh.  sanitize_boot_params() is part of the problem.  The startup sequence
goes something like this:

 (0) We enter the boot wrapper.

 (1) We clear the secure-boot status value [my patch adds this].

 (2) The boot wrapper *may* invoke efi_main() - which will determine the
     secure-boot status.

 (3) The boot wrapper calls extract_kernel() to decompress the kernel.

 (4) extract_kernel() calls sanitize_boot_params() which would otherwise clear
     the secure-boot flag.

 (5) The boot wrapper jumps into the main kernel image, which now does not see
     the secure boot status value we calculated.

So, no, sanitize_boot_params() must *not* zero the value unless we change the
call point for s_b_p().

> 2) In this case ->secure_boot should be zero unless modified inside of
> efi_main().

I have no idea whether this is guaranteed or not.

> Did you hit the scenario where ->secure_boot has a garbage value while
> developing these patches? I wouldn't expect to see it in practice.

I haven't actually checked what the value was before I cleared it.  But, I've
found that security people get seriously paranoid about assuming things to be
implicitly so;-).

David

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web