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

[Gary Lin]

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