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

Groups > linux.kernel > #1565316 > 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-23 22:30 +0100
    Re: [PATCH 5/8] efi: Get the secure boot status [ver #6] David Howells <dhowells@redhat.com> - 2017-01-23 23:20 +0100
      Re: [PATCH 5/8] efi: Get the secure boot status [ver #6] Matt Fleming <matt@codeblueprint.co.uk> - 2017-01-27 15:10 +0100
        Re: [PATCH 5/8] efi: Get the secure boot status [ver #6] David Howells <dhowells@redhat.com> - 2017-01-31 15:20 +0100
      What should the default lockdown mode be if the bootloader sentinel triggers sanitization? David Howells <dhowells@redhat.com> - 2017-01-30 13:20 +0100
        Re: What should the default lockdown mode be if the bootloader  sentinel triggers sanitization? Matt Fleming <matt@codeblueprint.co.uk> - 2017-01-30 15:00 +0100
          Re: What should the default lockdown mode be if the bootloader sentinel triggers sanitization? David Howells <dhowells@redhat.com> - 2017-01-30 15:10 +0100
            Re: What should the default lockdown mode be if the bootloader  sentinel triggers sanitization? Matt Fleming <matt@codeblueprint.co.uk> - 2017-01-31 13:00 +0100

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

On Mon, 16 Jan, at 03:39:18PM, David Howells wrote:
> 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.
 
The boot loader, not the wrapper unless there is no boot loader, such
as when the kernel image is loaded directly via EFI firmware (the
original EFI stub use case).

> > 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 exists to catch those boot loaders that don't keep to the spec,
e.g. kexec-tools.

> > 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.
 
The ->sentinel flag should be clear (because you zero'd boot_params on
alloc), so the code inside of sanitize_boot_params() should never
trigger for the secure boot case.

The comment for 'struct boot_params' explains this better than I can:

        /*
         * The sentinel is set to a nonzero value (0xff) in header.S.
         *
         * A bootloader is supposed to only take setup_header and put
         * it into a clean boot_params buffer. If it turns out that
         * it is clumsy or too generous with the buffer, it most
         * probably will pick up the sentinel variable too. The fact
         * that this variable then is still 0xff will let kernel
         * know that some variables in boot_params are invalid and
         * kernel should zero out certain portions of boot_params.
         */
        __u8  sentinel;                                 /* 0x1ef */

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

See the point above about the ->sentinel flag.

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

The thing is, we don't clear any older fields explicitly and we can't
clear not-yet-invented new fields in the future if they're being set
by the boot loader and not in the EFI stub. Let's not make this one
field unnecessarily different from everything else.

Additionally, the way you've written the code prohibits someone from
adding secure boot support to a boot loader that doesn't enter the
kernel via the EFI stub.

I have no idea whether any boot loaders exist that work that way, but
I don't see why we should actively prohibit them from working, when
allowing them to work is as simple as: Don't zero the field in the
boot stub.

[toc] | [next] | [standalone]

#1565329

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

> >  (4) extract_kernel() calls sanitize_boot_params() which would otherwise clear
> >      the secure-boot flag.
>  
> The ->sentinel flag should be clear (because you zero'd boot_params on
> alloc), so the code inside of sanitize_boot_params() should never
> trigger for the secure boot case.

But it *does* trigger, otherwise I wouldn't've noticed this.

David

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

#1568386

On Mon, 23 Jan, at 10:11:43PM, David Howells wrote:
> Matt Fleming <matt@codeblueprint.co.uk> wrote:
> 
> > >  (4) extract_kernel() calls sanitize_boot_params() which would otherwise clear
> > >      the secure-boot flag.
> >  
> > The ->sentinel flag should be clear (because you zero'd boot_params on
> > alloc), so the code inside of sanitize_boot_params() should never
> > trigger for the secure boot case.
> 
> But it *does* trigger, otherwise I wouldn't've noticed this.

This looks like it's triggered because of a grub2 bug, if I'm reading
the code correctly (big if).

grub2 memcpy()'s 1024 bytes from the start of kernel image header into
the allocated (and zeroed) boot_params object. Unfortunately, it
should only be copying the second 512-byte chunk, not the first too.

The boot loader should only fill out those fields in the first 512
bytes that it understands. Everything else should be zero, which
allows us to add fields (and give them default non-zero values in the
header) in the future without breaking old boot loaders.

Something like this might fix it (not compiled tested). Could one of
the grub2 folks take a look?

---->8----

diff --git a/grub-core/loader/i386/efi/linux.c b/grub-core/loader/i386/efi/linux.c
index 010bf98..fe5771e 100644
--- a/grub-core/loader/i386/efi/linux.c
+++ b/grub-core/loader/i386/efi/linux.c
@@ -269,7 +269,7 @@ grub_cmd_linux (grub_command_t cmd __attribute__ ((unused)),
   loaded=1;
 
   lh.code32_start = (grub_uint32_t)(grub_uint64_t) kernel_mem;
-  grub_memcpy (params, &lh, 2 * 512);
+  grub_memcpy (params, (grub_uint8_t *)&lh[512], 512);
 
   params->type_of_loader = 0x21;
 

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

#1570770

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

> -  grub_memcpy (params, &lh, 2 * 512);
> +  grub_memcpy (params, (grub_uint8_t *)&lh[512], 512);

It would appear this change is wrong and params needs to be changed to params
+ 512 or something similar.

David

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

#1569692 — What should the default lockdown mode be if the bootloader sentinel triggers sanitization?

Hi all,

There's an interesting issue with the way the x86 boot parameters are passed
into the kernel if we want to store the secure-boot mode flag in there.

My patches add boot_params->secure_boot, into which is placed the secure boot
mode as deduced by the EFI boot wrapper, if it is invoked.  This, however,
gets scrubbed by sanitize_boot_params() if the ->sentinel flag is set.  It
turns out that grub2 has a bug in it whereby it initialises boot_params by
copying the wrong stuff over it, thereby setting the ->sentinel flag.

In my patch I saw that sanitisation was happening and I stopped
sanitize_boot_params() from clobbering that particular byte and instead zeroed
it on entry to the boot wrapper.  This seemed reasonable since the boot
wrapper calculates the flag and simply overwrites whatever the boot loader had
placed there - and the value was getting clobbered by sanitisation called
during kernel decompression.

Matt argues, however, that boot_params->secure_boot should be propagated from
the bootloader and if the bootloader wants to set it, then we should skip the
check in efi_main() and go with the bootloader's opinion.  This is something
we probably want to do with kexec() so that the lockdown state is propagated
there.

However, what should happen in the core kernel if the bootloader doesn't
properly initialise ->sentinel and sanitisation is done that then clobbers
->secure_boot?  Should the kernel be locked down by default or left open by
default if lockdown was enabled in the kernel config?

But, as I mentioned, a bug in grub2 whereby it is copying the wrong
initialisation data over boot_params is causing sanitisation to be triggered.

Some questions that should clarify how we proceed:

 (1) Do we actually want to propagate the mode determination from the boot
     loader?

 (2) Do we have to determine the secure-boot status in the EFI boot wrapper
     (we don't use it there) or can we determine it in the core kernel?

 (3) What's the default mode in the case of sanitisation when lockdown is
     configured?

 (4) How do we handle the initialisation being mucked up such that ->sentinel
     ends up 0 and ->secure_boot ends up essentially random?

Any thoughts?

David

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

#1569756 — Re: What should the default lockdown mode be if the bootloader sentinel triggers sanitization?

On Mon, 30 Jan, at 12:10:29PM, David Howells wrote:
> 
> Matt argues, however, that boot_params->secure_boot should be propagated from
> the bootloader and if the bootloader wants to set it, then we should skip the
> check in efi_main() and go with the bootloader's opinion.  This is something
> we probably want to do with kexec() so that the lockdown state is propagated
> there.
 
Actually what I was arguing for was that if the boot loader wants to
set it and bypass the EFI boot stub, e.g. by going via the legacy
64-bit entry point, startup_64, then we should allow that as well as
setting the flag in the EFI boot stub.

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

#1569760 — Re: What should the default lockdown mode be if the bootloader sentinel triggers sanitization?

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

> > Matt argues, however, that boot_params->secure_boot should be propagated from
> > the bootloader and if the bootloader wants to set it, then we should skip the
> > check in efi_main() and go with the bootloader's opinion.  This is something
> > we probably want to do with kexec() so that the lockdown state is propagated
> > there.
>  
> Actually what I was arguing for was that if the boot loader wants to
> set it and bypass the EFI boot stub, e.g. by going via the legacy
> 64-bit entry point, startup_64, then we should allow that as well as
> setting the flag in the EFI boot stub.

That brings up another question:  Should the non-EFI entry points clear the
secure_boot mode flag and set a default?

David

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

#1570651 — Re: What should the default lockdown mode be if the bootloader sentinel triggers sanitization?

On Mon, 30 Jan, at 02:01:32PM, David Howells wrote:
> Matt Fleming <matt@codeblueprint.co.uk> wrote:
> 
> > > Matt argues, however, that boot_params->secure_boot should be propagated from
> > > the bootloader and if the bootloader wants to set it, then we should skip the
> > > check in efi_main() and go with the bootloader's opinion.  This is something
> > > we probably want to do with kexec() so that the lockdown state is propagated
> > > there.
> >  
> > Actually what I was arguing for was that if the boot loader wants to
> > set it and bypass the EFI boot stub, e.g. by going via the legacy
> > 64-bit entry point, startup_64, then we should allow that as well as
> > setting the flag in the EFI boot stub.
> 
> That brings up another question:  Should the non-EFI entry points clear the
> secure_boot mode flag and set a default?

There are no non-EFI boot entry points. EFI worked before we added the
EFI boot stub. The boot stub just provides new features (and allows us
to bundle firmware/boot fixes workarounds with kernel updates).

This is exactly why we should allow, or at least not actively
prohibit, the boot loader to set ->secure_boot and jump to the old
entry point if it wants to do that.

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web