Message-ID: <20171003032210.phfajmpzxmp45dq6@GaryWorkstation>
Date:   Tue, 3 Oct 2017 11:22:10 +0800
From:   Gary Lin <glin@...e.com>
To:     hpa@...or.com
Cc:     Ard Biesheuvel <ard.biesheuvel@...aro.org>,
        "x86@...nel.org" <x86@...nel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Joey Lee <jlee@...e.com>
Subject: Re: [RFC v2 PATCH] x86/boot: Add the secdata section to the setup
 header

On Fri, Sep 08, 2017 at 01:59:00PM -0700, hpa@...or.com wrote:
> On September 8, 2017 2:45:10 AM PDT, Gary Lin <glin@...e.com> wrote:
> >On Thu, Sep 07, 2017 at 02:16:21PM -0700, hpa@...or.com wrote:
> >> On September 7, 2017 2:44:51 AM PDT, Gary Lin <glin@...e.com> wrote:
> >> >On Thu, Jun 01, 2017 at 08:46:26AM +0000, Ard Biesheuvel wrote:
> >> >> On 1 June 2017 at 08:11, Gary Lin <glin@...e.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@...aro.org>
> >> >> >> Cc: "H. Peter Anvin" <hpa@...or.com>
> >> >> >> Cc: Thomas Gleixner <tglx@...utronix.de>
> >> >> >> Cc: Ingo Molnar <mingo@...hat.com>
> >> >> >> Cc: Joey Lee <jlee@...e.com>
> >> >> >> Signed-off-by: Gary Lin <glin@...e.com>
> >> >> >> ---
> >[snip]
> >> >> >> --
> >> >> >> 2.12.2
> >> >> >>
> >> >> 
> >> 
> >> I really don't think that is a good idea.  I would much rather keep
> >this in a space we fully own.
> >Fine. I'll find another place for ARM64 (probably append the structure
> >right after the PE-header and denote the 2-byte offset in the reserved
> >fields in the first 64 bytes header).
> >
> >Thanks,
> >
> >Gary Lin
> 
> Another "safe" option would be to put it in a COFF segment; then it would be system-independent.
Hi HPA,

Sorry for the late reply, I was travelling last two weeks.

In the beginning, I thought a new coff section was feasible. However, it
seems not possible for x86-64.

Although the section itself can be anywhere, we have to register an
entry in the section table in the coff optional header. For the x86-64
kernel image, the section table starts at 0x13b while every entry takes
40 bytes. Currently, there are 4 sections: .setup, .reloc, .text, and
.bss, so the new entry should be added at (0x13b + 0x24 * 4) = 0x1db and
ended at 0x1ff. Unfortunately, the x86 boot protocol requires the setup
header starts at 0x1f1, so there is no room for a new entry in the coff
section table :-(

Gary Lin

Powered by blists - more mailing lists