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

[hpa]

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.  Since this structure is a static ABI, it should have a way to grow it (a length field is usually a good choice.)

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.


-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.