Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1756115AbdIGVVe convert rfc822-to-8bit (ORCPT ); Thu, 7 Sep 2017 17:21:34 -0400 Received: from terminus.zytor.com ([65.50.211.136]:47535 "EHLO mail.zytor.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1755547AbdIGVVc (ORCPT ); Thu, 7 Sep 2017 17:21:32 -0400 Date: Thu, 07 Sep 2017 14:16:21 -0700 User-Agent: K-9 Mail for Android In-Reply-To: <20170907094451.2h2cbxpfmtga7buf@localhost> References: <20170512080534.4085-1-glin@suse.com> <20170601081136.ruiao3w2wfc3hftg@GaryWorkstation> <20170907094451.2h2cbxpfmtga7buf@localhost> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: 8BIT Subject: Re: [RFC v2 PATCH] x86/boot: Add the secdata section to the setup header To: Gary Lin , Ard Biesheuvel CC: "x86@kernel.org" , "linux-kernel@vger.kernel.org" , Thomas Gleixner , Ingo Molnar , Joey Lee From: hpa@zytor.com Message-ID: <2683B4EE-9BC5-4FCB-B880-C1A97163B24E@zytor.com> Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 8471 Lines: 241 On September 7, 2017 2:44:51 AM PDT, Gary Lin wrote: >On Thu, Jun 01, 2017 at 08:46:26AM +0000, Ard Biesheuvel wrote: >> On 1 June 2017 at 08:11, Gary Lin 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 >> >> Cc: "H. Peter Anvin" >> >> Cc: Thomas Gleixner >> >> Cc: Ingo Molnar >> >> Cc: Joey Lee >> >> Signed-off-by: Gary Lin >> >> --- >> >> 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 >> >> >> I really don't think that is a good idea. I would much rather keep this in a space we fully own. -- Sent from my Android device with K-9 Mail. Please excuse my brevity.