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

[Gary Lin <glin@suse.com>]

On Thu, Jun 01, 2017 at 08:46:26AM +0000, Ard Biesheuvel wrote:
> On 1 June 2017 at 08:11, Gary Lin <glin@suse.com> wrote:
> > On Fri, May 12, 2017 at 04:05:34PM +0800, Gary Lin wrote:
> >> A new section, secdata, in the setup header is introduced to store the
> >> distro-specific security version which is designed to help the
> >> bootloader to warn the user when loading a less secure or vulnerable
> >> kernel. The secdata section can be presented as the following:
> >>
> >> struct sec_hdr {
> >>       __u16 header_length;
> >>       __u32 distro_version;
> >>       __u16 security_version;
> >> } __attribute__((packed));
> >> char *signer;
> >>
> >> It consists of a fixed size structure and a null-terminated string.
> >> "header_length" is the size of "struct sec_hdr" and can be used as the
> >> offset to "signer". It also can be a kind of the "header version" to
> >> detect if any new member is introduced.
> >>
> >> The kernel packager of the distribution can put the distro name in
> >> "signer" and the distro version in "distro_version". When a severe
> >> vulnerability is fixed, the packager increases "security_version" in
> >> the kernel build afterward. The bootloader can maintain a list of the
> >> security versions of the current kernels and only allows the kernel with
> >> a higher or equal security version to boot. If the user is going to boot
> >> a kernel with a lower security version, a warning should show to prevent
> >> the user from loading a vulnerable kernel accidentally.
> >>
> >> Enabling UEFI Secure Boot is recommended when using the security version
> >> or the attacker may alter the security version stealthily.
> >>
> > Any comment?
> >
> 
> This is now entirely x86-specific. My preference would be to have a
> generic solution instead.
> 
After check the headers again, another idea came to my mind: the MS-DOS
stub. It's designed to show a warning while the image is loaded in DOS(*),
but I wonder if it still matters. In the x86 linux efi header, the stub
is just a 3-lines message, while arm64 completely ignores the stub.

Since there is a offset to the PE header at 0x3c, we can theoretically
put any thing between 0x40 and the PE header without affecting the
current settings.

HPA,

Does the MS-DOS stub raise any concern to you?

Thanks,

Gary Lin

(*) https://msdn.microsoft.com/zh-tw/library/windows/desktop/ms680547(v=vs.85).aspx#ms-dos_stub__image_only_

> -- 
> Ard.
> 
> 
> >> v2:
> >> - Decrease the size of secdata_offset to 2 bytes since the setup header
> >>   is limited to around 32KB.
> >> - Restructure the secdata section. The signer is now a null-terminated
> >>   string. The type of distro_version changes to u32 in case the distro
> >>   uses a long version.
> >> - Modify the Kconfig names and add help.
> >> - Remove the signer name hack in build.c.
> >>
> >> Cc: Ard Biesheuvel <ard.biesheuvel@linaro.org>
> >> Cc: "H. Peter Anvin" <hpa@zytor.com>
> >> Cc: Thomas Gleixner <tglx@linutronix.de>
> >> Cc: Ingo Molnar <mingo@redhat.com>
> >> Cc: Joey Lee <jlee@suse.com>
> >> Signed-off-by: Gary Lin <glin@suse.com>
> >> ---
> >>  arch/x86/Kconfig                      | 28 ++++++++++++++++++++++++++++
> >>  arch/x86/boot/header.S                | 14 +++++++++++++-
> >>  arch/x86/boot/setup.ld                |  1 +
> >>  arch/x86/boot/tools/build.c           |  1 -
> >>  arch/x86/include/uapi/asm/bootparam.h |  1 +
> >>  5 files changed, 43 insertions(+), 2 deletions(-)
> >>
> >> diff --git a/arch/x86/Kconfig b/arch/x86/Kconfig
> >> index 5bbdef151805..2c5539518ce0 100644
> >> --- a/arch/x86/Kconfig
> >> +++ b/arch/x86/Kconfig
> >> @@ -1817,6 +1817,34 @@ config EFI_MIXED
> >>
> >>          If unsure, say N.
> >>
> >> +config SIGNER_NAME
> >> +     string "Signer name"
> >> +     default ""
> >> +     ---help---
> >> +        This option specifies who signs or releases this kernel.
> >> +
> >> +config DISTRO_VERSION
> >> +     int "Distribution version"
> >> +     default 0
> >> +     range 0 4294967295
> >> +     ---help---
> >> +       This option specifies the distribution version which this
> >> +       kernel belongs to.
> >> +
> >> +config SECURITY_VERSION
> >> +     int "Security version"
> >> +     default 0
> >> +     range 0 65535
> >> +     ---help---
> >> +        The security version is the version defined by the distribution
> >> +        to indicate the severe security fixes. The bootloader can maintain
> >> +        a list of the security versions of the current kernels. After
> >> +        fixing a severe vulnerability in the kernel, the distribution can
> >> +        increase the security version to notify the bootloader to update
> >> +        the list. When booting a kernel with a lower security version,
> >> +        the bootloader warns the user to avoid loading a vulnerable kernel
> >> +        accidentally.
> >> +
> >>  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..37683caf1668 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:              .word secdata_start
> >>
> >>  # End of setup header #####################################################
> >>
> >> @@ -629,3 +630,14 @@ die:
> >>  setup_corrupt:
> >>       .byte   7
> >>       .string "No setup signature found...\n"
> >> +
> >> +     .section ".secdata", "a"
> >> +secdata_start:
> >> +header_length:
> >> +     .word   signer - secdata_start
> >> +distro_version:
> >> +     .long   CONFIG_DISTRO_VERSION
> >> +security_version:
> >> +     .word   CONFIG_SECURITY_VERSION
> >> +signer:
> >> +     .string CONFIG_SIGNER_NAME
> >> 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..a629d6b615cf 100644
> >> --- a/arch/x86/boot/tools/build.c
> >> +++ b/arch/x86/boot/tools/build.c
> >> @@ -287,7 +287,6 @@ static inline int reserve_pecoff_reloc_section(int c)
> >>  }
> >>  #endif /* CONFIG_EFI_STUB */
> >>
> >> -
> >>  /*
> >>   * Parse zoffset.h and find the entry points. We could just #include zoffset.h
> >>   * but that would mean tools/build would have to be rebuilt every time. It's
> >> diff --git a/arch/x86/include/uapi/asm/bootparam.h b/arch/x86/include/uapi/asm/bootparam.h
> >> index 07244ea16765..32ffacfaaaff 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;
> >> +     __u16   secdata_offset;
> >>  } __attribute__((packed));
> >>
> >>  struct sys_desc_table {
> >> --
> >> 2.12.2
> >>
>