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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <20170127140101.GD31613@codeblueprint.co.uk>
Date:   Fri, 27 Jan 2017 14:01:01 +0000
From:   Matt Fleming <matt@...eblueprint.co.uk>
To:     David Howells <dhowells@...hat.com>
Cc:     ard.biesheuvel@...aro.org, linux-efi@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        linux-security-module@...r.kernel.org, keyrings@...r.kernel.org,
        linux-arm-kernel@...ts.infradead.org,
        "H. Peter Anvin" <hpa@...or.com>, Peter Jones <pjones@...hat.com>,
        Michael Chang <mchang@...e.com>
Subject: Re: [PATCH 5/8] efi: Get the secure boot status [ver #6]

On Mon, 23 Jan, at 10:11:43PM, David Howells wrote:
> Matt Fleming <matt@...eblueprint.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;