Re: [RFC PATCH] x86/boot: Add the secdata section to the setup header

From: Gary Lin
Date: Mon May 08 2017 - 00:53:23 EST


On Sat, May 06, 2017 at 01:12:10AM -0700, hpa@xxxxxxxxx wrote:
> On May 5, 2017 2:26:39 AM PDT, Gary Lin <glin@xxxxxxxx> wrote:
> >This is a different approach to replace my previous implementation of
> >Security Version(*). Instead of using the fields in the PE/COFF header,
> >this commit adds secdata_offset in the setup header for the file offset
> >of secdata. Currently, the secdata section contains the signer's name,
> >the distro version, and the security version as defined in the wiki
> >page.
> >Since we don't rely on the PE/COFF header anymore, the size of signer
> >is
> >increased to 8 bytes to store more characters.
> >
> >Since this is just a tentative patch, I haven't started the other parts
> >(shim and mokutil) yet, and it's flexible to change. Any comment and
> >suggestion are welcome.
> >
> >(*) https://github.com/lcp/shim/wiki/Security-Version
> >
> >Cc: Ard Biesheuvel <ard.biesheuvel@xxxxxxxxxx>
> >Cc: "H. Peter Anvin" <hpa@xxxxxxxxx>
> >Cc: Thomas Gleixner <tglx@xxxxxxxxxxxxx>
> >Cc: Ingo Molnar <mingo@xxxxxxxxxx>
> >Cc: Joey Lee <jlee@xxxxxxxx>
> >Signed-off-by: Gary Lin <glin@xxxxxxxx>
> >---
> > arch/x86/Kconfig | 14 ++++++++++++++
> > arch/x86/boot/header.S | 15 ++++++++++++++-
> > arch/x86/boot/setup.ld | 1 +
> > arch/x86/boot/tools/build.c | 11 +++++++++++
> > arch/x86/include/uapi/asm/bootparam.h | 1 +
> > 5 files changed, 41 insertions(+), 1 deletion(-)
> >
> >diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> >index 5bbdef151805..09f99cd1e699 100644
> >--- a/arch/x86/Kconfig
> >+++ b/arch/x86/Kconfig
> >@@ -1817,6 +1817,20 @@ config EFI_MIXED
> >
> > If unsure, say N.
> >
> >+config SEC_SIGNER
> >+ string "The signer name"
> >+ default "none"
> >+
> >+config SEC_DISTRO
> >+ int "The distro version"
> >+ default 0
> >+ range 0 65535
> >+
> >+config SEC_VERSION
> >+ int "The security version"
> >+ default 0
> >+ range 0 65535
> >+
> > config SECCOMP
> > def_bool y
> > prompt "Enable seccomp to safely compute untrusted bytecode"
> >diff --git a/arch/x86/boot/header.S b/arch/x86/boot/header.S
> >index 3dd5be33aaa7..f751790f1f44 100644
> >--- a/arch/x86/boot/header.S
> >+++ b/arch/x86/boot/header.S
> >@@ -301,7 +301,7 @@ _start:
> > # Part 2 of the header, from the old setup.S
> >
> > .ascii "HdrS" # header signature
> >- .word 0x020d # header version number (>= 0x0105)
> >+ .word 0x020e # header version number (>= 0x0105)
> > # or else old loadlin-1.5 will fail)
> > .globl realmode_swtch
> > realmode_swtch: .word 0, 0 # default_switch, SETUPSEG
> >@@ -552,6 +552,7 @@ pref_address: .quad LOAD_PHYSICAL_ADDR # preferred
> >load addr
> >
> > init_size: .long INIT_SIZE # kernel initialization size
> > handover_offset: .long 0 # Filled in by build.c
> >+secdata_offset: .long secdata_start
> >
> ># End of setup header
> >#####################################################
> >
> >@@ -629,3 +630,15 @@ die:
> > setup_corrupt:
> > .byte 7
> > .string "No setup signature found...\n"
> >+
> >+ .section ".secdata", "a"
> >+secdata_start:
> >+sec_length:
> >+ .long secdata_end - secdata_start
> >+sec_signer:
> >+ .quad 0 # Filled by build.c
> >+sec_distro:
> >+ .word CONFIG_SEC_DISTRO
> >+sec_version:
> >+ .word CONFIG_SEC_VERSION
> >+secdata_end:
> >diff --git a/arch/x86/boot/setup.ld b/arch/x86/boot/setup.ld
> >index 96a6c7563538..43ddbaabaf7a 100644
> >--- a/arch/x86/boot/setup.ld
> >+++ b/arch/x86/boot/setup.ld
> >@@ -18,6 +18,7 @@ SECTIONS
> > .entrytext : { *(.entrytext) }
> > .inittext : { *(.inittext) }
> > .initdata : { *(.initdata) }
> >+ .secdata : { *(.secdata) }
> > __end_init = .;
> >
> > .text : { *(.text) }
> >diff --git a/arch/x86/boot/tools/build.c b/arch/x86/boot/tools/build.c
> >index 0702d2531bc7..ec4b311d7b2d 100644
> >--- a/arch/x86/boot/tools/build.c
> >+++ b/arch/x86/boot/tools/build.c
> >@@ -287,6 +287,15 @@ static inline int reserve_pecoff_reloc_section(int
> >c)
> > }
> > #endif /* CONFIG_EFI_STUB */
> >
> >+static void security_fields_update(void)
> >+{
> >+ unsigned int security_offset;
> >+ char *dest;
> >+
> >+ security_offset = get_unaligned_le32(&buf[0x268]);
> >+ dest = (char *)&buf[security_offset + 4];
> >+ strncpy(dest, CONFIG_SEC_SIGNER, 8);
> >+}
> >
> > /*
> >* Parse zoffset.h and find the entry points. We could just #include
> >zoffset.h
> >@@ -401,6 +410,8 @@ int main(int argc, char ** argv)
> >
> > efi_stub_entry_update();
> >
> >+ security_fields_update();
> >+
> > crc = partial_crc32(buf, i, crc);
> > if (fwrite(buf, 1, i, dest) != i)
> > die("Writing setup failed");
> >diff --git a/arch/x86/include/uapi/asm/bootparam.h
> >b/arch/x86/include/uapi/asm/bootparam.h
> >index 07244ea16765..713ae5d933a4 100644
> >--- a/arch/x86/include/uapi/asm/bootparam.h
> >+++ b/arch/x86/include/uapi/asm/bootparam.h
> >@@ -85,6 +85,7 @@ struct setup_header {
> > __u64 pref_address;
> > __u32 init_size;
> > __u32 handover_offset;
> >+ __u32 secdata_offset;
> > } __attribute__((packed));
> >
> > struct sys_desc_table {
>
> There is no need to burn a 4-byte field if this is inside the setup area, as that is limited to ~32K anyway.

Right, 2-byte is enough for secdata_offset.

> Since this structure is a static ABI, it should have a way to grow it (a length field is usually a good choice.)
>

sec_length serves for the purpose. It's the length of the secdata
section, so we can use it to determine if there is a new field.

> What are the semantics of these fields? If they are supposed to be distribution-independent, they need semantics defined, otherwise they might better be a distro-specific use field.
>
They are all distro-specific.

sec_signer is basically the distro name. sec_distro is the distro
version. sec_version is the "Security Version" defined by the distro.
If grub2 is booting a "less-secure" kernel, i.e. a kernel with a lower
Security Version, a warning will show to notify the user.

Cheers,

Gary Lin