[PATCH 0/8] efi: Pass secure boot mode to kernel [ver #6]

[PATCH 0/8] efi: Pass secure boot mode to kernel [ver #6]

[David Howells]

Here's a set of patches that can determine the secure boot state of the
UEFI BIOS and pass that along to the main kernel image.  This involves
generalising ARM's efi_get_secureboot() function and making it mixed-mode
safe.

Changes:

 Ver 6:

  - Removed unnecessary variable init and trimmed comment.
  - Return efi_secureboot_mode_disabled directly rather than going to a
    place that just returns it.
  - Switched the last two patches.

 Ver 5:

  - Fix i386 compilation error (rsi should've been changed to esi).
  - Fix arm64 compilation error ('sys_table_arg' is a hidden macro parameter).

 Ver 4:

  - Use an enum to tell the kernel whether secure boot mode is enabled,
    disabled, couldn't be determined or wasn't even tried due to not being
    in EFI mode.
  - Support the UEFI-2.6 DeployedMode flag.
  - Don't clear boot_params->secure_boot in x86 sanitize_boot_params().
  - Preclear the boot_params->secure_boot on x86 head_*.S entry if we may
    not go through efi_main().

The patches can be found here also:

	http://git.kernel.org/cgit/linux/kernel/git/dhowells/linux-fs.git/log/?h=efi-secure-boot

at tag:

	efi-secure-boot-20161207-2

David
---
Ard Biesheuvel (1):
      efi: use typed function pointers for runtime services table

David Howells (5):
      x86/efi: Allow invocation of arbitrary runtime services
      arm/efi: Allow invocation of arbitrary runtime services
      efi: Add SHIM and image security database GUID definitions
      efi: Get the secure boot status
      efi: Handle secure boot from UEFI-2.6

Josh Boyer (2):
      efi: Disable secure boot if shim is in insecure mode
      efi: Add EFI_SECURE_BOOT bit


 Documentation/x86/zero-page.txt           |    2 +
 arch/arm/include/asm/efi.h                |    1 
 arch/arm64/include/asm/efi.h              |    1 
 arch/x86/boot/compressed/eboot.c          |    3 +
 arch/x86/boot/compressed/head_32.S        |    7 +-
 arch/x86/boot/compressed/head_64.S        |    9 +--
 arch/x86/include/asm/bootparam_utils.h    |    5 +
 arch/x86/include/asm/efi.h                |    5 +
 arch/x86/include/uapi/asm/bootparam.h     |    3 +
 arch/x86/kernel/asm-offsets.c             |    1 
 arch/x86/kernel/setup.c                   |   15 ++++
 drivers/firmware/efi/libstub/Makefile     |    2 -
 drivers/firmware/efi/libstub/arm-stub.c   |   63 ++----------------
 drivers/firmware/efi/libstub/secureboot.c |   99 +++++++++++++++++++++++++++++
 include/linux/efi.h                       |   52 ++++++++++-----
 15 files changed, 182 insertions(+), 86 deletions(-)
 create mode 100644 drivers/firmware/efi/libstub/secureboot.c

[PATCH 1/8] efi: use typed function pointers for runtime services table [ver #6]

[David Howells]

From: Ard Biesheuvel <ard.biesheuvel@linaro.org>

Instead of using void pointers, and casting them to correctly typed
function pointers upon use, declare the runtime services pointers
as function pointers using their respective prototypes, for which
typedefs are already available.

Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
Signed-off-by: David Howells <dhowells@redhat.com>
---

 include/linux/efi.h |   36 ++++++++++++++++++------------------
 1 file changed, 18 insertions(+), 18 deletions(-)

diff --git a/include/linux/efi.h b/include/linux/efi.h
index a07a476178cd..93a82de167eb 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -508,24 +508,6 @@ typedef struct {
 	u64 query_variable_info;
 } efi_runtime_services_64_t;
 
-typedef struct {
-	efi_table_hdr_t hdr;
-	void *get_time;
-	void *set_time;
-	void *get_wakeup_time;
-	void *set_wakeup_time;
-	void *set_virtual_address_map;
-	void *convert_pointer;
-	void *get_variable;
-	void *get_next_variable;
-	void *set_variable;
-	void *get_next_high_mono_count;
-	void *reset_system;
-	void *update_capsule;
-	void *query_capsule_caps;
-	void *query_variable_info;
-} efi_runtime_services_t;
-
 typedef efi_status_t efi_get_time_t (efi_time_t *tm, efi_time_cap_t *tc);
 typedef efi_status_t efi_set_time_t (efi_time_t *tm);
 typedef efi_status_t efi_get_wakeup_time_t (efi_bool_t *enabled, efi_bool_t *pending,
@@ -560,6 +542,24 @@ typedef efi_status_t efi_query_variable_store_t(u32 attributes,
 						unsigned long size,
 						bool nonblocking);
 
+typedef struct {
+	efi_table_hdr_t			hdr;
+	efi_get_time_t			*get_time;
+	efi_set_time_t			*set_time;
+	efi_get_wakeup_time_t		*get_wakeup_time;
+	efi_set_wakeup_time_t		*set_wakeup_time;
+	efi_set_virtual_address_map_t	*set_virtual_address_map;
+	void				*convert_pointer;
+	efi_get_variable_t		*get_variable;
+	efi_get_next_variable_t		*get_next_variable;
+	efi_set_variable_t		*set_variable;
+	efi_get_next_high_mono_count_t	*get_next_high_mono_count;
+	efi_reset_system_t		*reset_system;
+	efi_update_capsule_t		*update_capsule;
+	efi_query_capsule_caps_t	*query_capsule_caps;
+	efi_query_variable_info_t	*query_variable_info;
+} efi_runtime_services_t;
+
 void efi_native_runtime_setup(void);
 
 /*

[PATCH 2/8] x86/efi: Allow invocation of arbitrary runtime services [ver #6]

[David Howells]

Provide the ability to perform mixed-mode runtime service calls for x86 in
the same way that commit 0a637ee61247bd4bed9b2a07568ef7a1cfc76187
("x86/efi: Allow invocation of arbitrary boot services") provides the
ability to invoke arbitrary boot services.

Suggested-by: Lukas Wunner <lukas@wunner.de>
Signed-off-by: David Howells <dhowells@redhat.com>
---

 arch/x86/boot/compressed/eboot.c   |    1 +
 arch/x86/boot/compressed/head_32.S |    6 +++---
 arch/x86/boot/compressed/head_64.S |    8 ++++----
 arch/x86/include/asm/efi.h         |    5 +++++
 4 files changed, 13 insertions(+), 7 deletions(-)

diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
index ff01c8fc76f7..c8c32ebcdfdb 100644
--- a/arch/x86/boot/compressed/eboot.c
+++ b/arch/x86/boot/compressed/eboot.c
@@ -32,6 +32,7 @@ static void setup_boot_services##bits(struct efi_config *c)		\
 									\
 	table = (typeof(table))sys_table;				\
 									\
+	c->runtime_services = table->runtime;				\
 	c->boot_services = table->boottime;				\
 	c->text_output = table->con_out;				\
 }
diff --git a/arch/x86/boot/compressed/head_32.S b/arch/x86/boot/compressed/head_32.S
index fd0b6a272dd5..d85b9625e836 100644
--- a/arch/x86/boot/compressed/head_32.S
+++ b/arch/x86/boot/compressed/head_32.S
@@ -82,7 +82,7 @@ ENTRY(efi_pe_entry)
 
 	/* Relocate efi_config->call() */
 	leal	efi32_config(%esi), %eax
-	add	%esi, 32(%eax)
+	add	%esi, 40(%eax)
 	pushl	%eax
 
 	call	make_boot_params
@@ -108,7 +108,7 @@ ENTRY(efi32_stub_entry)
 
 	/* Relocate efi_config->call() */
 	leal	efi32_config(%esi), %eax
-	add	%esi, 32(%eax)
+	add	%esi, 40(%eax)
 	pushl	%eax
 2:
 	call	efi_main
@@ -264,7 +264,7 @@ relocated:
 #ifdef CONFIG_EFI_STUB
 	.data
 efi32_config:
-	.fill 4,8,0
+	.fill 5,8,0
 	.long efi_call_phys
 	.long 0
 	.byte 0
diff --git a/arch/x86/boot/compressed/head_64.S b/arch/x86/boot/compressed/head_64.S
index efdfba21a5b2..beab8322f72a 100644
--- a/arch/x86/boot/compressed/head_64.S
+++ b/arch/x86/boot/compressed/head_64.S
@@ -265,7 +265,7 @@ ENTRY(efi_pe_entry)
 	/*
 	 * Relocate efi_config->call().
 	 */
-	addq	%rbp, efi64_config+32(%rip)
+	addq	%rbp, efi64_config+40(%rip)
 
 	movq	%rax, %rdi
 	call	make_boot_params
@@ -285,7 +285,7 @@ handover_entry:
 	 * Relocate efi_config->call().
 	 */
 	movq	efi_config(%rip), %rax
-	addq	%rbp, 32(%rax)
+	addq	%rbp, 40(%rax)
 2:
 	movq	efi_config(%rip), %rdi
 	call	efi_main
@@ -457,14 +457,14 @@ efi_config:
 #ifdef CONFIG_EFI_MIXED
 	.global efi32_config
 efi32_config:
-	.fill	4,8,0
+	.fill	5,8,0
 	.quad	efi64_thunk
 	.byte	0
 #endif
 
 	.global efi64_config
 efi64_config:
-	.fill	4,8,0
+	.fill	5,8,0
 	.quad	efi_call
 	.byte	1
 #endif /* CONFIG_EFI_STUB */
diff --git a/arch/x86/include/asm/efi.h b/arch/x86/include/asm/efi.h
index e99675b9c861..2f77bcefe6b4 100644
--- a/arch/x86/include/asm/efi.h
+++ b/arch/x86/include/asm/efi.h
@@ -191,6 +191,7 @@ static inline efi_status_t efi_thunk_set_virtual_address_map(
 struct efi_config {
 	u64 image_handle;
 	u64 table;
+	u64 runtime_services;
 	u64 boot_services;
 	u64 text_output;
 	efi_status_t (*call)(unsigned long, ...);
@@ -226,6 +227,10 @@ static inline bool efi_is_64bit(void)
 #define __efi_call_early(f, ...)					\
 	__efi_early()->call((unsigned long)f, __VA_ARGS__);
 
+#define efi_call_runtime(f, ...)					\
+	__efi_early()->call(efi_table_attr(efi_runtime_services, f,	\
+		__efi_early()->runtime_services), __VA_ARGS__)
+
 extern bool efi_reboot_required(void);
 
 #else

[PATCH 3/8] arm/efi: Allow invocation of arbitrary runtime services [ver #6]

[David Howells]

efi_call_runtime() is provided for x86 to be able abstract mixed mode
support.  Provide this for ARM also so that common code work in mixed mode
also.

Suggested-by: Lukas Wunner <lukas@wunner.de>
Signed-off-by: David Howells <dhowells@redhat.com>
---

 arch/arm/include/asm/efi.h   |    1 +
 arch/arm64/include/asm/efi.h |    1 +
 2 files changed, 2 insertions(+)

diff --git a/arch/arm/include/asm/efi.h b/arch/arm/include/asm/efi.h
index 0b06f5341b45..e4e6a9d6a825 100644
--- a/arch/arm/include/asm/efi.h
+++ b/arch/arm/include/asm/efi.h
@@ -55,6 +55,7 @@ void efi_virtmap_unload(void);
 
 #define efi_call_early(f, ...)		sys_table_arg->boottime->f(__VA_ARGS__)
 #define __efi_call_early(f, ...)	f(__VA_ARGS__)
+#define efi_call_runtime(f, ...)	sys_table_arg->runtime->f(__VA_ARGS__)
 #define efi_is_64bit()			(false)
 
 #define efi_call_proto(protocol, f, instance, ...)			\
diff --git a/arch/arm64/include/asm/efi.h b/arch/arm64/include/asm/efi.h
index 771b3f0bc757..d74ae223d89f 100644
--- a/arch/arm64/include/asm/efi.h
+++ b/arch/arm64/include/asm/efi.h
@@ -49,6 +49,7 @@ int efi_set_mapping_permissions(struct mm_struct *mm, efi_memory_desc_t *md);
 
 #define efi_call_early(f, ...)		sys_table_arg->boottime->f(__VA_ARGS__)
 #define __efi_call_early(f, ...)	f(__VA_ARGS__)
+#define efi_call_runtime(f, ...)	sys_table_arg->runtime->f(__VA_ARGS__)
 #define efi_is_64bit()			(true)
 
 #define efi_call_proto(protocol, f, instance, ...)			\

[PATCH 4/8] efi: Add SHIM and image security database GUID definitions [ver #6]

[David Howells]

Add the definitions for shim and image security database, both of which
are used widely in various Linux distros.

Signed-off-by: Josh Boyer <jwboyer@fedoraproject.org>
Signed-off-by: David Howells <dhowells@redhat.com>
Reviewed-by: Ard Biesheuvel <ard.biesheuvel@linaro.org>
---

 include/linux/efi.h |    3 +++
 1 file changed, 3 insertions(+)

diff --git a/include/linux/efi.h b/include/linux/efi.h
index 93a82de167eb..c7904556d7a8 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -610,6 +610,9 @@ void efi_native_runtime_setup(void);
 #define EFI_CONSOLE_OUT_DEVICE_GUID		EFI_GUID(0xd3b36f2c, 0xd551, 0x11d4,  0x9a, 0x46, 0x00, 0x90, 0x27, 0x3f, 0xc1, 0x4d)
 #define APPLE_PROPERTIES_PROTOCOL_GUID		EFI_GUID(0x91bd12fe, 0xf6c3, 0x44fb,  0xa5, 0xb7, 0x51, 0x22, 0xab, 0x30, 0x3a, 0xe0)
 
+#define EFI_IMAGE_SECURITY_DATABASE_GUID	EFI_GUID(0xd719b2cb, 0x3d3a, 0x4596, 0xa3, 0xbc, 0xda, 0xd0, 0x0e, 0x67, 0x65, 0x6f)
+#define EFI_SHIM_LOCK_GUID			EFI_GUID(0x605dab50, 0xe046, 0x4300, 0xab, 0xb6, 0x3d, 0xd8, 0x10, 0xdd, 0x8b, 0x23)
+
 /*
  * This GUID is used to pass to the kernel proper the struct screen_info
  * structure that was populated by the stub based on the GOP protocol instance

[PATCH 5/8] efi: Get the secure boot status [ver #6]

[David Howells]

Get the firmware's secure-boot status in the kernel boot wrapper and stash
it somewhere that the main kernel image can find.

The efi_get_secureboot() function is extracted from the arm stub and (a)
generalised so that it can be called from x86 and (b) made to use
efi_call_runtime() so that it can be run in mixed-mode.

Suggested-by: Lukas Wunner <lukas@wunner.de>
Signed-off-by: David Howells <dhowells@redhat.com>
---

 Documentation/x86/zero-page.txt           |    2 +
 arch/x86/boot/compressed/eboot.c          |    2 +
 arch/x86/boot/compressed/head_32.S        |    1 
 arch/x86/boot/compressed/head_64.S        |    1 
 arch/x86/include/asm/bootparam_utils.h    |    5 +-
 arch/x86/include/uapi/asm/bootparam.h     |    3 +
 arch/x86/kernel/asm-offsets.c             |    1 
 drivers/firmware/efi/libstub/Makefile     |    2 -
 drivers/firmware/efi/libstub/arm-stub.c   |   63 +++--------------------------
 drivers/firmware/efi/libstub/secureboot.c |   63 +++++++++++++++++++++++++++++
 include/linux/efi.h                       |    8 ++++
 11 files changed, 90 insertions(+), 61 deletions(-)
 create mode 100644 drivers/firmware/efi/libstub/secureboot.c

diff --git a/Documentation/x86/zero-page.txt b/Documentation/x86/zero-page.txt
index 95a4d34af3fd..b8527c6b7646 100644
--- a/Documentation/x86/zero-page.txt
+++ b/Documentation/x86/zero-page.txt
@@ -31,6 +31,8 @@ Offset	Proto	Name		Meaning
 1E9/001	ALL	eddbuf_entries	Number of entries in eddbuf (below)
 1EA/001	ALL	edd_mbr_sig_buf_entries	Number of entries in edd_mbr_sig_buffer
 				(below)
+1EB/001	ALL     kbd_status      Numlock is enabled
+1EC/001	ALL     secure_boot	Secure boot is enabled in the firmware
 1EF/001	ALL	sentinel	Used to detect broken bootloaders
 290/040	ALL	edd_mbr_sig_buffer EDD MBR signatures
 2D0/A00	ALL	e820_map	E820 memory map table
diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
index c8c32ebcdfdb..5b151c262ac2 100644
--- a/arch/x86/boot/compressed/eboot.c
+++ b/arch/x86/boot/compressed/eboot.c
@@ -1158,6 +1158,8 @@ struct boot_params *efi_main(struct efi_config *c,
 	else
 		setup_boot_services32(efi_early);
 
+	boot_params->secure_boot = efi_get_secureboot(sys_table);
+
 	setup_graphics(boot_params);
 
 	setup_efi_pci(boot_params);
diff --git a/arch/x86/boot/compressed/head_32.S b/arch/x86/boot/compressed/head_32.S
index d85b9625e836..c635f7e32f5c 100644
--- a/arch/x86/boot/compressed/head_32.S
+++ b/arch/x86/boot/compressed/head_32.S
@@ -61,6 +61,7 @@
 
 	__HEAD
 ENTRY(startup_32)
+	movb	$0, BP_secure_boot(%esi)
 #ifdef CONFIG_EFI_STUB
 	jmp	preferred_addr
 
diff --git a/arch/x86/boot/compressed/head_64.S b/arch/x86/boot/compressed/head_64.S
index beab8322f72a..ccd2c7461b7f 100644
--- a/arch/x86/boot/compressed/head_64.S
+++ b/arch/x86/boot/compressed/head_64.S
@@ -244,6 +244,7 @@ ENTRY(startup_64)
 	 * that maps our entire kernel(text+data+bss+brk), zero page
 	 * and command line.
 	 */
+	movb	$0, BP_secure_boot(%rsi)
 #ifdef CONFIG_EFI_STUB
 	/*
 	 * The entry point for the PE/COFF executable is efi_pe_entry, so
diff --git a/arch/x86/include/asm/bootparam_utils.h b/arch/x86/include/asm/bootparam_utils.h
index 4a8cb8d7cbd5..7e16d53ff6a3 100644
--- a/arch/x86/include/asm/bootparam_utils.h
+++ b/arch/x86/include/asm/bootparam_utils.h
@@ -38,9 +38,10 @@ static void sanitize_boot_params(struct boot_params *boot_params)
 		memset(&boot_params->ext_ramdisk_image, 0,
 		       (char *)&boot_params->efi_info -
 			(char *)&boot_params->ext_ramdisk_image);
-		memset(&boot_params->kbd_status, 0,
+		boot_params->kbd_status = 0;
+		memset(&boot_params->_pad5, 0,
 		       (char *)&boot_params->hdr -
-		       (char *)&boot_params->kbd_status);
+		       (char *)&boot_params->_pad5);
 		memset(&boot_params->_pad7[0], 0,
 		       (char *)&boot_params->edd_mbr_sig_buffer[0] -
 			(char *)&boot_params->_pad7[0]);
diff --git a/arch/x86/include/uapi/asm/bootparam.h b/arch/x86/include/uapi/asm/bootparam.h
index b10bf319ed20..5138dacf8bb8 100644
--- a/arch/x86/include/uapi/asm/bootparam.h
+++ b/arch/x86/include/uapi/asm/bootparam.h
@@ -135,7 +135,8 @@ struct boot_params {
 	__u8  eddbuf_entries;				/* 0x1e9 */
 	__u8  edd_mbr_sig_buf_entries;			/* 0x1ea */
 	__u8  kbd_status;				/* 0x1eb */
-	__u8  _pad5[3];					/* 0x1ec */
+	__u8  secure_boot;				/* 0x1ec */
+	__u8  _pad5[2];					/* 0x1ed */
 	/*
 	 * The sentinel is set to a nonzero value (0xff) in header.S.
 	 *
diff --git a/arch/x86/kernel/asm-offsets.c b/arch/x86/kernel/asm-offsets.c
index c62e015b126c..de827d6ac8c2 100644
--- a/arch/x86/kernel/asm-offsets.c
+++ b/arch/x86/kernel/asm-offsets.c
@@ -81,6 +81,7 @@ void common(void) {
 
 	BLANK();
 	OFFSET(BP_scratch, boot_params, scratch);
+	OFFSET(BP_secure_boot, boot_params, secure_boot);
 	OFFSET(BP_loadflags, boot_params, hdr.loadflags);
 	OFFSET(BP_hardware_subarch, boot_params, hdr.hardware_subarch);
 	OFFSET(BP_version, boot_params, hdr.version);
diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile
index 6621b13c370f..9af966863612 100644
--- a/drivers/firmware/efi/libstub/Makefile
+++ b/drivers/firmware/efi/libstub/Makefile
@@ -28,7 +28,7 @@ OBJECT_FILES_NON_STANDARD	:= y
 # Prevents link failures: __sanitizer_cov_trace_pc() is not linked in.
 KCOV_INSTRUMENT			:= n
 
-lib-y				:= efi-stub-helper.o gop.o
+lib-y				:= efi-stub-helper.o gop.o secureboot.o
 
 # include the stub's generic dependencies from lib/ when building for ARM/arm64
 arm-deps := fdt_rw.c fdt_ro.c fdt_wip.c fdt.c fdt_empty_tree.c fdt_sw.c sort.c
diff --git a/drivers/firmware/efi/libstub/arm-stub.c b/drivers/firmware/efi/libstub/arm-stub.c
index b4f7d78f9e8b..9984d0442442 100644
--- a/drivers/firmware/efi/libstub/arm-stub.c
+++ b/drivers/firmware/efi/libstub/arm-stub.c
@@ -20,52 +20,6 @@
 
 bool __nokaslr;
 
-static int efi_get_secureboot(efi_system_table_t *sys_table_arg)
-{
-	static efi_char16_t const sb_var_name[] = {
-		'S', 'e', 'c', 'u', 'r', 'e', 'B', 'o', 'o', 't', 0 };
-	static efi_char16_t const sm_var_name[] = {
-		'S', 'e', 't', 'u', 'p', 'M', 'o', 'd', 'e', 0 };
-
-	efi_guid_t var_guid = EFI_GLOBAL_VARIABLE_GUID;
-	efi_get_variable_t *f_getvar = sys_table_arg->runtime->get_variable;
-	u8 val;
-	unsigned long size = sizeof(val);
-	efi_status_t status;
-
-	status = f_getvar((efi_char16_t *)sb_var_name, (efi_guid_t *)&var_guid,
-			  NULL, &size, &val);
-
-	if (status != EFI_SUCCESS)
-		goto out_efi_err;
-
-	if (val == 0)
-		return 0;
-
-	status = f_getvar((efi_char16_t *)sm_var_name, (efi_guid_t *)&var_guid,
-			  NULL, &size, &val);
-
-	if (status != EFI_SUCCESS)
-		goto out_efi_err;
-
-	if (val == 1)
-		return 0;
-
-	return 1;
-
-out_efi_err:
-	switch (status) {
-	case EFI_NOT_FOUND:
-		return 0;
-	case EFI_DEVICE_ERROR:
-		return -EIO;
-	case EFI_SECURITY_VIOLATION:
-		return -EACCES;
-	default:
-		return -EINVAL;
-	}
-}
-
 efi_status_t efi_open_volume(efi_system_table_t *sys_table_arg,
 			     void *__image, void **__fh)
 {
@@ -226,7 +180,7 @@ unsigned long efi_entry(void *handle, efi_system_table_t *sys_table,
 	efi_guid_t loaded_image_proto = LOADED_IMAGE_PROTOCOL_GUID;
 	unsigned long reserve_addr = 0;
 	unsigned long reserve_size = 0;
-	int secure_boot = 0;
+	enum efi_secureboot_mode secure_boot;
 	struct screen_info *si;
 
 	/* Check if we were booted by the EFI firmware */
@@ -296,19 +250,14 @@ unsigned long efi_entry(void *handle, efi_system_table_t *sys_table,
 		pr_efi_err(sys_table, "Failed to parse EFI cmdline options\n");
 
 	secure_boot = efi_get_secureboot(sys_table);
-	if (secure_boot > 0)
-		pr_efi(sys_table, "UEFI Secure Boot is enabled.\n");
-
-	if (secure_boot < 0) {
-		pr_efi_err(sys_table,
-			"could not determine UEFI Secure Boot status.\n");
-	}
 
 	/*
-	 * Unauthenticated device tree data is a security hazard, so
-	 * ignore 'dtb=' unless UEFI Secure Boot is disabled.
+	 * Unauthenticated device tree data is a security hazard, so ignore
+	 * 'dtb=' unless UEFI Secure Boot is disabled.  We assume that secure
+	 * boot is enabled if we can't determine its state.
 	 */
-	if (secure_boot != 0 && strstr(cmdline_ptr, "dtb=")) {
+	if (secure_boot != efi_secureboot_mode_disabled &&
+	    strstr(cmdline_ptr, "dtb=")) {
 		pr_efi(sys_table, "Ignoring DTB from command line.\n");
 	} else {
 		status = handle_cmdline_files(sys_table, image, cmdline_ptr,
diff --git a/drivers/firmware/efi/libstub/secureboot.c b/drivers/firmware/efi/libstub/secureboot.c
new file mode 100644
index 000000000000..62d6904da800
--- /dev/null
+++ b/drivers/firmware/efi/libstub/secureboot.c
@@ -0,0 +1,63 @@
+/*
+ * Secure boot handling.
+ *
+ * Copyright (C) 2013,2014 Linaro Limited
+ *     Roy Franz <roy.franz@linaro.org
+ * Copyright (C) 2013 Red Hat, Inc.
+ *     Mark Salter <msalter@redhat.com>
+ *
+ * This file is part of the Linux kernel, and is made available under the
+ * terms of the GNU General Public License version 2.
+ *
+ */
+
+#include <linux/efi.h>
+#include <asm/efi.h>
+
+/* BIOS variables */
+static const efi_guid_t efi_variable_guid = EFI_GLOBAL_VARIABLE_GUID;
+static const efi_char16_t const efi_SecureBoot_name[] = {
+	'S', 'e', 'c', 'u', 'r', 'e', 'B', 'o', 'o', 't', 0
+};
+static const efi_char16_t const efi_SetupMode_name[] = {
+	'S', 'e', 't', 'u', 'p', 'M', 'o', 'd', 'e', 0
+};
+
+#define get_efi_var(name, vendor, ...) \
+	efi_call_runtime(get_variable, \
+			 (efi_char16_t *)(name), (efi_guid_t *)(vendor), \
+			 __VA_ARGS__);
+
+/*
+ * Determine whether we're in secure boot mode.
+ */
+enum efi_secureboot_mode efi_get_secureboot(efi_system_table_t *sys_table_arg)
+{
+	u8 secboot, setupmode;
+	unsigned long size;
+	efi_status_t status;
+
+	size = sizeof(secboot);
+	status = get_efi_var(efi_SecureBoot_name, &efi_variable_guid,
+			     NULL, &size, &secboot);
+	if (status != EFI_SUCCESS)
+		goto out_efi_err;
+
+	size = sizeof(setupmode);
+	status = get_efi_var(efi_SetupMode_name, &efi_variable_guid,
+			     NULL, &size, &setupmode);
+	if (status != EFI_SUCCESS)
+		goto out_efi_err;
+
+	if (secboot == 0 || setupmode == 1)
+		return efi_secureboot_mode_disabled;
+
+	pr_efi(sys_table_arg, "UEFI Secure Boot is enabled.\n");
+	return efi_secureboot_mode_enabled;
+
+out_efi_err:
+	pr_efi_err(sys_table_arg, "Could not determine UEFI Secure Boot status.\n");
+	if (status == EFI_NOT_FOUND)
+		return efi_secureboot_mode_disabled;
+	return efi_secureboot_mode_unknown;
+}
diff --git a/include/linux/efi.h b/include/linux/efi.h
index c7904556d7a8..92e23f03045e 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -1477,6 +1477,14 @@ efi_status_t efi_setup_gop(efi_system_table_t *sys_table_arg,
 bool efi_runtime_disabled(void);
 extern void efi_call_virt_check_flags(unsigned long flags, const char *call);
 
+enum efi_secureboot_mode {
+	efi_secureboot_mode_unset,
+	efi_secureboot_mode_unknown,
+	efi_secureboot_mode_disabled,
+	efi_secureboot_mode_enabled,
+};
+enum efi_secureboot_mode efi_get_secureboot(efi_system_table_t *sys_table);
+
 /*
  * Arch code can implement the following three template macros, avoiding
  * reptition for the void/non-void return cases of {__,}efi_call_virt():

[PATCH 6/8] efi: Disable secure boot if shim is in insecure mode [ver #6]

[David Howells]

From: Josh Boyer <jwboyer-rxtnV0ftBwyoClj4AeEUq9i2O/JbrIOy@public.gmane.org>

A user can manually tell the shim boot loader to disable validation of
images it loads.  When a user does this, it creates a UEFI variable called
MokSBState that does not have the runtime attribute set.  Given that the
user explicitly disabled validation, we can honor that and not enable
secure boot mode if that variable is set.

Signed-off-by: Josh Boyer <jwboyer-rxtnV0ftBwyoClj4AeEUq9i2O/JbrIOy@public.gmane.org>
Signed-off-by: David Howells <dhowells-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---

 drivers/firmware/efi/libstub/secureboot.c |   24 +++++++++++++++++++++++-
 1 file changed, 23 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/efi/libstub/secureboot.c b/drivers/firmware/efi/libstub/secureboot.c
index 62d6904da800..39c91e091f6a 100644
--- a/drivers/firmware/efi/libstub/secureboot.c
+++ b/drivers/firmware/efi/libstub/secureboot.c
@@ -23,6 +23,12 @@ static const efi_char16_t const efi_SetupMode_name[] = {
 	'S', 'e', 't', 'u', 'p', 'M', 'o', 'd', 'e', 0
 };
 
+/* SHIM variables */
+static const efi_guid_t shim_guid = EFI_SHIM_LOCK_GUID;
+static efi_char16_t const shim_MokSBState_name[] = {
+	'M', 'o', 'k', 'S', 'B', 'S', 't', 'a', 't', 'e', 0
+};
+
 #define get_efi_var(name, vendor, ...) \
 	efi_call_runtime(get_variable, \
 			 (efi_char16_t *)(name), (efi_guid_t *)(vendor), \
@@ -33,7 +39,8 @@ static const efi_char16_t const efi_SetupMode_name[] = {
  */
 enum efi_secureboot_mode efi_get_secureboot(efi_system_table_t *sys_table_arg)
 {
-	u8 secboot, setupmode;
+	u32 attr;
+	u8 secboot, setupmode, moksbstate;
 	unsigned long size;
 	efi_status_t status;
 
@@ -52,6 +59,21 @@ enum efi_secureboot_mode efi_get_secureboot(efi_system_table_t *sys_table_arg)
 	if (secboot == 0 || setupmode == 1)
 		return efi_secureboot_mode_disabled;
 
+	/* See if a user has put shim into insecure mode.  If so, and if the
+	 * variable doesn't have the runtime attribute set, we might as well
+	 * honor that.
+	 */
+	size = sizeof(moksbstate);
+	status = get_efi_var(shim_MokSBState_name, &shim_guid,
+			     &attr, &size, &moksbstate);
+
+	/* If it fails, we don't care why.  Default to secure */
+	if (status != EFI_SUCCESS)
+		goto secure_boot_enabled;
+	if (!(attr & EFI_VARIABLE_RUNTIME_ACCESS) && moksbstate == 1)
+		return efi_secureboot_mode_disabled;
+
+secure_boot_enabled:
 	pr_efi(sys_table_arg, "UEFI Secure Boot is enabled.\n");
 	return efi_secureboot_mode_enabled;
 

[PATCH 7/8] efi: Handle secure boot from UEFI-2.6 [ver #6]

[David Howells]

UEFI-2.6 adds a new variable, DeployedMode.  If it exists, this must be 1
if we're to engage lockdown mode.

Reported-by: James Bottomley <James.Bottomley@HansenPartnership.com>
Signed-off-by: David Howells <dhowells@redhat.com>
---

 drivers/firmware/efi/libstub/secureboot.c |   16 +++++++++++++++-
 include/linux/efi.h                       |    4 ++++
 2 files changed, 19 insertions(+), 1 deletion(-)

diff --git a/drivers/firmware/efi/libstub/secureboot.c b/drivers/firmware/efi/libstub/secureboot.c
index 39c91e091f6a..d653f76b9725 100644
--- a/drivers/firmware/efi/libstub/secureboot.c
+++ b/drivers/firmware/efi/libstub/secureboot.c
@@ -22,6 +22,9 @@ static const efi_char16_t const efi_SecureBoot_name[] = {
 static const efi_char16_t const efi_SetupMode_name[] = {
 	'S', 'e', 't', 'u', 'p', 'M', 'o', 'd', 'e', 0
 };
+static const efi_char16_t const efi_DeployedMode_name[] = {
+	'D', 'e', 'p', 'l', 'o', 'y', 'e', 'd', 'M', 'o', 'd', 'e', 0
+};
 
 /* SHIM variables */
 static const efi_guid_t shim_guid = EFI_SHIM_LOCK_GUID;
@@ -40,7 +43,7 @@ static efi_char16_t const shim_MokSBState_name[] = {
 enum efi_secureboot_mode efi_get_secureboot(efi_system_table_t *sys_table_arg)
 {
 	u32 attr;
-	u8 secboot, setupmode, moksbstate;
+	u8 secboot, setupmode, deployedmode, moksbstate;
 	unsigned long size;
 	efi_status_t status;
 
@@ -59,6 +62,17 @@ enum efi_secureboot_mode efi_get_secureboot(efi_system_table_t *sys_table_arg)
 	if (secboot == 0 || setupmode == 1)
 		return efi_secureboot_mode_disabled;
 
+	/* UEFI-2.6 requires DeployedMode to be 1. */
+	if (sys_table_arg->hdr.revision >= EFI_2_60_SYSTEM_TABLE_REVISION) {
+		size = sizeof(deployedmode);
+		status = get_efi_var(efi_DeployedMode_name, &efi_variable_guid,
+				     NULL, &size, &deployedmode);
+		if (status != EFI_SUCCESS)
+			goto out_efi_err;
+		if (deployedmode == 0)
+			return efi_secureboot_mode_disabled;
+	}
+
 	/* See if a user has put shim into insecure mode.  If so, and if the
 	 * variable doesn't have the runtime attribute set, we might as well
 	 * honor that.
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 92e23f03045e..c894ed5bfa1c 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -645,6 +645,10 @@ typedef struct {
 
 #define EFI_SYSTEM_TABLE_SIGNATURE ((u64)0x5453595320494249ULL)
 
+#define EFI_2_60_SYSTEM_TABLE_REVISION  ((2 << 16) | (60))
+#define EFI_2_50_SYSTEM_TABLE_REVISION  ((2 << 16) | (50))
+#define EFI_2_40_SYSTEM_TABLE_REVISION  ((2 << 16) | (40))
+#define EFI_2_31_SYSTEM_TABLE_REVISION  ((2 << 16) | (31))
 #define EFI_2_30_SYSTEM_TABLE_REVISION  ((2 << 16) | (30))
 #define EFI_2_20_SYSTEM_TABLE_REVISION  ((2 << 16) | (20))
 #define EFI_2_10_SYSTEM_TABLE_REVISION  ((2 << 16) | (10))

[PATCH 8/8] efi: Add EFI_SECURE_BOOT bit [ver #6]

[David Howells]

From: Josh Boyer <jwboyer@fedoraproject.org>

UEFI machines can be booted in Secure Boot mode.  Add a EFI_SECURE_BOOT bit
that can be passed to efi_enabled() to find out whether secure boot is
enabled.

This will be used by the SysRq+x handler, registered by the x86 arch, to find
out whether secure boot mode is enabled so that it can be disabled.

Signed-off-by: Josh Boyer <jwboyer@fedoraproject.org>
Signed-off-by: David Howells <dhowells@redhat.com>
---

 arch/x86/kernel/setup.c |   15 +++++++++++++++
 include/linux/efi.h     |    1 +
 2 files changed, 16 insertions(+)

diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index 9c337b0e8ba7..d8972ec6257d 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -1152,6 +1152,21 @@ void __init setup_arch(char **cmdline_p)
 	/* Allocate bigger log buffer */
 	setup_log_buf(1);
 
+	if (IS_ENABLED(CONFIG_EFI)) {
+		switch (boot_params.secure_boot) {
+		case efi_secureboot_mode_disabled:
+			pr_info("Secure boot disabled\n");
+			break;
+		case efi_secureboot_mode_enabled:
+			set_bit(EFI_SECURE_BOOT, &efi.flags);
+			pr_info("Secure boot enabled\n");
+			break;
+		default:
+			pr_info("Secure boot could not be determined\n");
+			break;
+		}
+	}
+
 	reserve_initrd();
 
 	acpi_table_upgrade();
diff --git a/include/linux/efi.h b/include/linux/efi.h
index c894ed5bfa1c..e1893f5002c3 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -1070,6 +1070,7 @@ extern int __init efi_setup_pcdp_console(char *);
 #define EFI_ARCH_1		7	/* First arch-specific bit */
 #define EFI_DBG			8	/* Print additional debug info at runtime */
 #define EFI_NX_PE_DATA		9	/* Can runtime data regions be mapped non-executable? */
+#define EFI_SECURE_BOOT		10	/* Are we in Secure Boot mode? */
 
 #ifdef CONFIG_EFI
 /*

Re: [PATCH 5/8] efi: Get the secure boot status [ver #6]

[Matt Fleming]

On Thu, 08 Dec, at 12:30:45PM, David Howells wrote:
> Get the firmware's secure-boot status in the kernel boot wrapper and stash
> it somewhere that the main kernel image can find.
> 
> The efi_get_secureboot() function is extracted from the arm stub and (a)
> generalised so that it can be called from x86 and (b) made to use
> efi_call_runtime() so that it can be run in mixed-mode.
> 
> Suggested-by: Lukas Wunner <lukas-JFq808J9C/izQB+pC5nmwQ@public.gmane.org>
> Signed-off-by: David Howells <dhowells-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> ---
> 
>  Documentation/x86/zero-page.txt           |    2 +
>  arch/x86/boot/compressed/eboot.c          |    2 +
>  arch/x86/boot/compressed/head_32.S        |    1 
>  arch/x86/boot/compressed/head_64.S        |    1 
>  arch/x86/include/asm/bootparam_utils.h    |    5 +-
>  arch/x86/include/uapi/asm/bootparam.h     |    3 +
>  arch/x86/kernel/asm-offsets.c             |    1 
>  drivers/firmware/efi/libstub/Makefile     |    2 -
>  drivers/firmware/efi/libstub/arm-stub.c   |   63 +++--------------------------
>  drivers/firmware/efi/libstub/secureboot.c |   63 +++++++++++++++++++++++++++++
>  include/linux/efi.h                       |    8 ++++
>  11 files changed, 90 insertions(+), 61 deletions(-)
>  create mode 100644 drivers/firmware/efi/libstub/secureboot.c
 
[...]

> diff --git a/arch/x86/boot/compressed/head_32.S b/arch/x86/boot/compressed/head_32.S
> index d85b9625e836..c635f7e32f5c 100644
> --- a/arch/x86/boot/compressed/head_32.S
> +++ b/arch/x86/boot/compressed/head_32.S
> @@ -61,6 +61,7 @@
>  
>  	__HEAD
>  ENTRY(startup_32)
> +	movb	$0, BP_secure_boot(%esi)
>  #ifdef CONFIG_EFI_STUB
>  	jmp	preferred_addr
>  
> diff --git a/arch/x86/boot/compressed/head_64.S b/arch/x86/boot/compressed/head_64.S
> index beab8322f72a..ccd2c7461b7f 100644
> --- a/arch/x86/boot/compressed/head_64.S
> +++ b/arch/x86/boot/compressed/head_64.S
> @@ -244,6 +244,7 @@ ENTRY(startup_64)
>  	 * that maps our entire kernel(text+data+bss+brk), zero page
>  	 * and command line.
>  	 */
> +	movb	$0, BP_secure_boot(%rsi)
>  #ifdef CONFIG_EFI_STUB
>  	/*
>  	 * The entry point for the PE/COFF executable is efi_pe_entry, so

Is clearing ::secure_boot really necessary? Any code path that goes
via efi_main() will set it correctly and all other code paths should
get it cleared in sanitize_boot_params(), no?

> diff --git a/include/linux/efi.h b/include/linux/efi.h
> index c7904556d7a8..92e23f03045e 100644
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -1477,6 +1477,14 @@ efi_status_t efi_setup_gop(efi_system_table_t *sys_table_arg,
>  bool efi_runtime_disabled(void);
>  extern void efi_call_virt_check_flags(unsigned long flags, const char *call);
>  
> +enum efi_secureboot_mode {
> +	efi_secureboot_mode_unset,
> +	efi_secureboot_mode_unknown,
> +	efi_secureboot_mode_disabled,
> +	efi_secureboot_mode_enabled,
> +};
> +enum efi_secureboot_mode efi_get_secureboot(efi_system_table_t *sys_table);
> +
>  /*
>   * Arch code can implement the following three template macros, avoiding
>   * reptition for the void/non-void return cases of {__,}efi_call_virt():
> 

What's the distinction between the unset and unknown enums?

Re: [PATCH 8/8] efi: Add EFI_SECURE_BOOT bit [ver #6]

[Matt Fleming]

On Thu, 08 Dec, at 12:31:08PM, David Howells wrote:
> From: Josh Boyer <jwboyer@fedoraproject.org>
> 
> UEFI machines can be booted in Secure Boot mode.  Add a EFI_SECURE_BOOT bit
> that can be passed to efi_enabled() to find out whether secure boot is
> enabled.
> 
> This will be used by the SysRq+x handler, registered by the x86 arch, to find
> out whether secure boot mode is enabled so that it can be disabled.
> 
> Signed-off-by: Josh Boyer <jwboyer@fedoraproject.org>
> Signed-off-by: David Howells <dhowells@redhat.com>
> ---
> 
>  arch/x86/kernel/setup.c |   15 +++++++++++++++
>  include/linux/efi.h     |    1 +
>  2 files changed, 16 insertions(+)

Before we add more efi.flags bits I'd like this series to include the
patch that makes use of EFI_SECURE_BOOT. Alternatively, you move this
last patch to a new series.

Re: [PATCH 0/8] efi: Pass secure boot mode to kernel [ver #6]

[Matt Fleming]

On Thu, 08 Dec, at 12:30:08PM, David Howells wrote:
> 
> Here's a set of patches that can determine the secure boot state of the
> UEFI BIOS and pass that along to the main kernel image.  This involves
> generalising ARM's efi_get_secureboot() function and making it mixed-mode
> safe.

This version looks OK to me apart from the couple of comments I made.

Ard, did you take a look? In particular some boot testing on ARM/arm64
would be useful. x86 boots fine in both regular and mixed mode but
I've only tested without Secure Boot enabled.

Re: [PATCH 0/8] efi: Pass secure boot mode to kernel [ver #6]

[Ard Biesheuvel]

On 11 January 2017 at 15:01, Matt Fleming <matt@codeblueprint.co.uk> wrote:
> On Thu, 08 Dec, at 12:30:08PM, David Howells wrote:
>>
>> Here's a set of patches that can determine the secure boot state of the
>> UEFI BIOS and pass that along to the main kernel image.  This involves
>> generalising ARM's efi_get_secureboot() function and making it mixed-mode
>> safe.
>
> This version looks OK to me apart from the couple of comments I made.
>
> Ard, did you take a look? In particular some boot testing on ARM/arm64
> would be useful. x86 boots fine in both regular and mixed mode but
> I've only tested without Secure Boot enabled.

I did take a look at these patches (and commented on them) as they
were coming in, but I haven't yet gone through them as thoroughly as I
should. I will test them on ARM/arm64 as well.

Re: [PATCH 5/8] efi: Get the secure boot status [ver #6]

[David Howells]

Matt Fleming <matt@codeblueprint.co.uk> wrote:

> > +	movb	$0, BP_secure_boot(%rsi)
> >  #ifdef CONFIG_EFI_STUB
> >  	/*
> >  	 * The entry point for the PE/COFF executable is efi_pe_entry, so
> 
> Is clearing ::secure_boot really necessary? Any code path that goes
> via efi_main() will set it correctly and all other code paths should
> get it cleared in sanitize_boot_params(), no?

No.

The boot_params->secure_boot parameter exists whether or not efi_main() is
traversed (ie. if EFI isn't enabled or CONFIG_EFI_STUB=n) and, if not cleared,
is of uncertain value.

Further, sanitize_boot_params() has to be modified by this patch so as not to
clobber the secure_boot flag.

> What's the distinction between the unset and unknown enums?

unset -> The flag was cleared by head.S and efi_get_secureboot() was never
called.

unknown -> efi_get_secureboot() tried and failed to access the EFI variables
that should give the state.

David

Re: [PATCH 8/8] efi: Add EFI_SECURE_BOOT bit [ver #6]

[David Howells]

Matt Fleming <matt@codeblueprint.co.uk> wrote:

> Before we add more efi.flags bits I'd like this series to include the
> patch that makes use of EFI_SECURE_BOOT. Alternatively, you move this
> last patch to a new series.

Are you willing to take the kernel lock-down patches also?

David

Re: [PATCH 8/8] efi: Add EFI_SECURE_BOOT bit [ver #6]

[Matt Fleming]

On Wed, 11 Jan, at 03:29:24PM, David Howells wrote:
> Matt Fleming <matt@codeblueprint.co.uk> wrote:
> 
> > Before we add more efi.flags bits I'd like this series to include the
> > patch that makes use of EFI_SECURE_BOOT. Alternatively, you move this
> > last patch to a new series.
> 
> Are you willing to take the kernel lock-down patches also?

I'm happy to take them through the EFI tree provided that they've been
Reviewed/Ack-ed by the security folks.

Re: [PATCH 5/8] efi: Get the secure boot status [ver #6]

[Matt Fleming]

(Cc'ing Peter A. and Peter J. for boot params discussion)

On Wed, 11 Jan, at 03:27:23PM, David Howells wrote:
> Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org> wrote:
> 
> > > +	movb	$0, BP_secure_boot(%rsi)
> > >  #ifdef CONFIG_EFI_STUB
> > >  	/*
> > >  	 * The entry point for the PE/COFF executable is efi_pe_entry, so
> > 
> > Is clearing ::secure_boot really necessary? Any code path that goes
> > via efi_main() will set it correctly and all other code paths should
> > get it cleared in sanitize_boot_params(), no?
> 
> No.
> 
> The boot_params->secure_boot parameter exists whether or not efi_main() is
> traversed (ie. if EFI isn't enabled or CONFIG_EFI_STUB=n) and, if not cleared,
> is of uncertain value.
>
> Further, sanitize_boot_params() has to be modified by this patch so as not to
> clobber the secure_boot flag.

Any new parameters that boot loaders do not know about should be
cleared to zero by default in the boot loader because boot_params
itself should be zero'd when allocated.

There are two cases to consider:

 1) boot_params is not zero'd
 2) boot_params is zero'd

1) This is a broken boot loader implementation that violates the x86
boot specification and I would never expect ->secure_boot to have a
valid value. It should not be special-cased in sanitize_boot_params(),
it should be zero'd.

2) In this case ->secure_boot should be zero unless modified inside of
efi_main().

Did you hit the scenario where ->secure_boot has a garbage value while
developing these patches? I wouldn't expect to see it in practice.

Re: [PATCH 5/8] efi: Get the secure boot status [ver #6]

[David Howells]

Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org> wrote:

> On Wed, 11 Jan, at 03:27:23PM, David Howells wrote:
> > Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org> wrote:
> > 
> > > > +	movb	$0, BP_secure_boot(%rsi)
> > > >  #ifdef CONFIG_EFI_STUB
> > > >  	/*
> > > >  	 * The entry point for the PE/COFF executable is efi_pe_entry, so
> > > 
> > > Is clearing ::secure_boot really necessary? Any code path that goes
> > > via efi_main() will set it correctly and all other code paths should
> > > get it cleared in sanitize_boot_params(), no?
> > 
> > No.
> > 
> > The boot_params->secure_boot parameter exists whether or not efi_main() is
> > traversed (ie. if EFI isn't enabled or CONFIG_EFI_STUB=n) and, if not cleared,
> > is of uncertain value.
> >
> > Further, sanitize_boot_params() has to be modified by this patch so as not to
> > clobber the secure_boot flag.
> 
> Any new parameters that boot loaders do not know about should be
> cleared to zero by default in the boot loader because boot_params
> itself should be zero'd when allocated.

Do you mean the boot loader or the boot wrapper?  If the loader, that is
outside my control - and given the purpose of the value, I'm not sure I
want to rely on that.

> There are two cases to consider:
> 
>  1) boot_params is not zero'd
>  2) boot_params is zero'd
> 
> 1) This is a broken boot loader implementation that violates the x86
> boot specification and I would never expect ->secure_boot to have a
> valid value.

If there's a boot specification that must be complied with, why does
sanitize_boot_params() even exist?  Why does the comment on it say:

 * Deal with bootloaders which fail to initialize unknown fields in
 * boot_params to zero.  The list fields in this list are taken from
 * analysis of kexec-tools; if other broken bootloaders initialize a
 * different set of fields we will need to figure out how to disambiguate.

> It should not be special-cased in sanitize_boot_params(), it should be
> zero'd.

Sigh.  sanitize_boot_params() is part of the problem.  The startup sequence
goes something like this:

 (0) We enter the boot wrapper.

 (1) We clear the secure-boot status value [my patch adds this].

 (2) The boot wrapper *may* invoke efi_main() - which will determine the
     secure-boot status.

 (3) The boot wrapper calls extract_kernel() to decompress the kernel.

 (4) extract_kernel() calls sanitize_boot_params() which would otherwise clear
     the secure-boot flag.

 (5) The boot wrapper jumps into the main kernel image, which now does not see
     the secure boot status value we calculated.

So, no, sanitize_boot_params() must *not* zero the value unless we change the
call point for s_b_p().

> 2) In this case ->secure_boot should be zero unless modified inside of
> efi_main().

I have no idea whether this is guaranteed or not.

> Did you hit the scenario where ->secure_boot has a garbage value while
> developing these patches? I wouldn't expect to see it in practice.

I haven't actually checked what the value was before I cleared it.  But, I've
found that security people get seriously paranoid about assuming things to be
implicitly so;-).

David

Re: [PATCH 8/8] efi: Add EFI_SECURE_BOOT bit [ver #6]

[David Howells]

Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org> wrote:

> > > Before we add more efi.flags bits I'd like this series to include the
> > > patch that makes use of EFI_SECURE_BOOT. Alternatively, you move this
> > > last patch to a new series.
> > 
> > Are you willing to take the kernel lock-down patches also?
> 
> I'm happy to take them through the EFI tree provided that they've been
> Reviewed/Ack-ed by the security folks.

Thinking further about it, it might be best to push the EFI_SECURE_BOOT flag
into a later series and exclude that patch from this one.

David

Re: [PATCH 5/8] efi: Get the secure boot status [ver #6]

[David Howells]

Hi Matt, Ard,

Any further thoughts?

Thanks,
David

Re: [PATCH 5/8] efi: Get the secure boot status [ver #6]

[Matt Fleming]

On Mon, 16 Jan, at 03:39:18PM, David Howells wrote:
> Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org> wrote:
> 
> > On Wed, 11 Jan, at 03:27:23PM, David Howells wrote:
> > > Matt Fleming <matt-mF/unelCI9GS6iBeEJttW/XRex20P6io@public.gmane.org> wrote:
> > > 
> > > > > +	movb	$0, BP_secure_boot(%rsi)
> > > > >  #ifdef CONFIG_EFI_STUB
> > > > >  	/*
> > > > >  	 * The entry point for the PE/COFF executable is efi_pe_entry, so
> > > > 
> > > > Is clearing ::secure_boot really necessary? Any code path that goes
> > > > via efi_main() will set it correctly and all other code paths should
> > > > get it cleared in sanitize_boot_params(), no?
> > > 
> > > No.
> > > 
> > > The boot_params->secure_boot parameter exists whether or not efi_main() is
> > > traversed (ie. if EFI isn't enabled or CONFIG_EFI_STUB=n) and, if not cleared,
> > > is of uncertain value.
> > >
> > > Further, sanitize_boot_params() has to be modified by this patch so as not to
> > > clobber the secure_boot flag.
> > 
> > Any new parameters that boot loaders do not know about should be
> > cleared to zero by default in the boot loader because boot_params
> > itself should be zero'd when allocated.
> 
> Do you mean the boot loader or the boot wrapper?  If the loader, that is
> outside my control - and given the purpose of the value, I'm not sure I
> want to rely on that.
 
The boot loader, not the wrapper unless there is no boot loader, such
as when the kernel image is loaded directly via EFI firmware (the
original EFI stub use case).

> > There are two cases to consider:
> > 
> >  1) boot_params is not zero'd
> >  2) boot_params is zero'd
> > 
> > 1) This is a broken boot loader implementation that violates the x86
> > boot specification and I would never expect ->secure_boot to have a
> > valid value.
> 
> If there's a boot specification that must be complied with, why does
> sanitize_boot_params() even exist?  Why does the comment on it say:
> 
>  * Deal with bootloaders which fail to initialize unknown fields in
>  * boot_params to zero.  The list fields in this list are taken from
>  * analysis of kexec-tools; if other broken bootloaders initialize a
>  * different set of fields we will need to figure out how to disambiguate.
 
It exists to catch those boot loaders that don't keep to the spec,
e.g. kexec-tools.

> > It should not be special-cased in sanitize_boot_params(), it should be
> > zero'd.
> 
> Sigh.  sanitize_boot_params() is part of the problem.  The startup sequence
> goes something like this:
> 
>  (0) We enter the boot wrapper.
> 
>  (1) We clear the secure-boot status value [my patch adds this].
> 
>  (2) The boot wrapper *may* invoke efi_main() - which will determine the
>      secure-boot status.
> 
>  (3) The boot wrapper calls extract_kernel() to decompress the kernel.
> 
>  (4) extract_kernel() calls sanitize_boot_params() which would otherwise clear
>      the secure-boot flag.
 
The ->sentinel flag should be clear (because you zero'd boot_params on
alloc), so the code inside of sanitize_boot_params() should never
trigger for the secure boot case.

The comment for 'struct boot_params' explains this better than I can:

        /*
         * The sentinel is set to a nonzero value (0xff) in header.S.
         *
         * A bootloader is supposed to only take setup_header and put
         * it into a clean boot_params buffer. If it turns out that
         * it is clumsy or too generous with the buffer, it most
         * probably will pick up the sentinel variable too. The fact
         * that this variable then is still 0xff will let kernel
         * know that some variables in boot_params are invalid and
         * kernel should zero out certain portions of boot_params.
         */
        __u8  sentinel;                                 /* 0x1ef */

>  (5) The boot wrapper jumps into the main kernel image, which now does not see
>      the secure boot status value we calculated.
> 
> So, no, sanitize_boot_params() must *not* zero the value unless we change the
> call point for s_b_p().

See the point above about the ->sentinel flag.

> > 2) In this case ->secure_boot should be zero unless modified inside of
> > efi_main().
> 
> I have no idea whether this is guaranteed or not.
> 
> > Did you hit the scenario where ->secure_boot has a garbage value while
> > developing these patches? I wouldn't expect to see it in practice.
> 
> I haven't actually checked what the value was before I cleared it.  But, I've
> found that security people get seriously paranoid about assuming things to be
> implicitly so;-).

The thing is, we don't clear any older fields explicitly and we can't
clear not-yet-invented new fields in the future if they're being set
by the boot loader and not in the EFI stub. Let's not make this one
field unnecessarily different from everything else.

Additionally, the way you've written the code prohibits someone from
adding secure boot support to a boot loader that doesn't enter the
kernel via the EFI stub.

I have no idea whether any boot loaders exist that work that way, but
I don't see why we should actively prohibit them from working, when
allowing them to work is as simple as: Don't zero the field in the
boot stub.

Re: [PATCH 5/8] efi: Get the secure boot status [ver #6]

[David Howells]

Matt Fleming <matt@codeblueprint.co.uk> wrote:

> >  (4) extract_kernel() calls sanitize_boot_params() which would otherwise clear
> >      the secure-boot flag.
>  
> The ->sentinel flag should be clear (because you zero'd boot_params on
> alloc), so the code inside of sanitize_boot_params() should never
> trigger for the secure boot case.

But it *does* trigger, otherwise I wouldn't've noticed this.

David

Re: [PATCH 0/8] efi: Pass secure boot mode to kernel [ver #6]

[Ard Biesheuvel]

On 11 January 2017 at 15:05, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> On 11 January 2017 at 15:01, Matt Fleming <matt@codeblueprint.co.uk> wrote:
>> On Thu, 08 Dec, at 12:30:08PM, David Howells wrote:
>>>
>>> Here's a set of patches that can determine the secure boot state of the
>>> UEFI BIOS and pass that along to the main kernel image.  This involves
>>> generalising ARM's efi_get_secureboot() function and making it mixed-mode
>>> safe.
>>
>> This version looks OK to me apart from the couple of comments I made.
>>
>> Ard, did you take a look? In particular some boot testing on ARM/arm64
>> would be useful. x86 boots fine in both regular and mixed mode but
>> I've only tested without Secure Boot enabled.
>
> I did take a look at these patches (and commented on them) as they
> were coming in, but I haven't yet gone through them as thoroughly as I
> should. I will test them on ARM/arm64 as well.

Apologies for the tardiness. I intend to look into these tomorrow.

Re: [PATCH 5/8] efi: Get the secure boot status [ver #6]

[Matt Fleming]

On Mon, 23 Jan, at 10:11:43PM, David Howells wrote:
> Matt Fleming <matt@codeblueprint.co.uk> wrote:
> 
> > >  (4) extract_kernel() calls sanitize_boot_params() which would otherwise clear
> > >      the secure-boot flag.
> >  
> > The ->sentinel flag should be clear (because you zero'd boot_params on
> > alloc), so the code inside of sanitize_boot_params() should never
> > trigger for the secure boot case.
> 
> But it *does* trigger, otherwise I wouldn't've noticed this.

This looks like it's triggered because of a grub2 bug, if I'm reading
the code correctly (big if).

grub2 memcpy()'s 1024 bytes from the start of kernel image header into
the allocated (and zeroed) boot_params object. Unfortunately, it
should only be copying the second 512-byte chunk, not the first too.

The boot loader should only fill out those fields in the first 512
bytes that it understands. Everything else should be zero, which
allows us to add fields (and give them default non-zero values in the
header) in the future without breaking old boot loaders.

Something like this might fix it (not compiled tested). Could one of
the grub2 folks take a look?

---->8----

diff --git a/grub-core/loader/i386/efi/linux.c b/grub-core/loader/i386/efi/linux.c
index 010bf98..fe5771e 100644
--- a/grub-core/loader/i386/efi/linux.c
+++ b/grub-core/loader/i386/efi/linux.c
@@ -269,7 +269,7 @@ grub_cmd_linux (grub_command_t cmd __attribute__ ((unused)),
   loaded=1;
 
   lh.code32_start = (grub_uint32_t)(grub_uint64_t) kernel_mem;
-  grub_memcpy (params, &lh, 2 * 512);
+  grub_memcpy (params, (grub_uint8_t *)&lh[512], 512);
 
   params->type_of_loader = 0x21;
 

Re: [PATCH 0/8] efi: Pass secure boot mode to kernel [ver #6]

[Ard Biesheuvel]

On 24 January 2017 at 17:15, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> On 11 January 2017 at 15:05, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>> On 11 January 2017 at 15:01, Matt Fleming <matt@codeblueprint.co.uk> wrote:
>>> On Thu, 08 Dec, at 12:30:08PM, David Howells wrote:
>>>>
>>>> Here's a set of patches that can determine the secure boot state of the
>>>> UEFI BIOS and pass that along to the main kernel image.  This involves
>>>> generalising ARM's efi_get_secureboot() function and making it mixed-mode
>>>> safe.
>>>
>>> This version looks OK to me apart from the couple of comments I made.
>>>
>>> Ard, did you take a look? In particular some boot testing on ARM/arm64
>>> would be useful. x86 boots fine in both regular and mixed mode but
>>> I've only tested without Secure Boot enabled.
>>
>> I did take a look at these patches (and commented on them) as they
>> were coming in, but I haven't yet gone through them as thoroughly as I
>> should. I will test them on ARM/arm64 as well.
>
> Apologies for the tardiness. I intend to look into these tomorrow.

These patches build and run fine on arm, including on secure boot
systems, so I don't have any objections.

Once the open discussion points re x86 are resolved, I can proceed and
merge them if desired. Matt?

What should the default lockdown mode be if the bootloader sentinel triggers sanitization?

[David Howells]

Hi all,

There's an interesting issue with the way the x86 boot parameters are passed
into the kernel if we want to store the secure-boot mode flag in there.

My patches add boot_params->secure_boot, into which is placed the secure boot
mode as deduced by the EFI boot wrapper, if it is invoked.  This, however,
gets scrubbed by sanitize_boot_params() if the ->sentinel flag is set.  It
turns out that grub2 has a bug in it whereby it initialises boot_params by
copying the wrong stuff over it, thereby setting the ->sentinel flag.

In my patch I saw that sanitisation was happening and I stopped
sanitize_boot_params() from clobbering that particular byte and instead zeroed
it on entry to the boot wrapper.  This seemed reasonable since the boot
wrapper calculates the flag and simply overwrites whatever the boot loader had
placed there - and the value was getting clobbered by sanitisation called
during kernel decompression.

Matt argues, however, that boot_params->secure_boot should be propagated from
the bootloader and if the bootloader wants to set it, then we should skip the
check in efi_main() and go with the bootloader's opinion.  This is something
we probably want to do with kexec() so that the lockdown state is propagated
there.

However, what should happen in the core kernel if the bootloader doesn't
properly initialise ->sentinel and sanitisation is done that then clobbers
->secure_boot?  Should the kernel be locked down by default or left open by
default if lockdown was enabled in the kernel config?

But, as I mentioned, a bug in grub2 whereby it is copying the wrong
initialisation data over boot_params is causing sanitisation to be triggered.

Some questions that should clarify how we proceed:

 (1) Do we actually want to propagate the mode determination from the boot
     loader?

 (2) Do we have to determine the secure-boot status in the EFI boot wrapper
     (we don't use it there) or can we determine it in the core kernel?

 (3) What's the default mode in the case of sanitisation when lockdown is
     configured?

 (4) How do we handle the initialisation being mucked up such that ->sentinel
     ends up 0 and ->secure_boot ends up essentially random?

Any thoughts?

David

Re: What should the default lockdown mode be if the bootloader sentinel triggers sanitization?

[Matt Fleming]

On Mon, 30 Jan, at 12:10:29PM, David Howells wrote:
> 
> Matt argues, however, that boot_params->secure_boot should be propagated from
> the bootloader and if the bootloader wants to set it, then we should skip the
> check in efi_main() and go with the bootloader's opinion.  This is something
> we probably want to do with kexec() so that the lockdown state is propagated
> there.
 
Actually what I was arguing for was that if the boot loader wants to
set it and bypass the EFI boot stub, e.g. by going via the legacy
64-bit entry point, startup_64, then we should allow that as well as
setting the flag in the EFI boot stub.

Re: What should the default lockdown mode be if the bootloader sentinel triggers sanitization?

[David Howells]

Matt Fleming <matt@codeblueprint.co.uk> wrote:

> > Matt argues, however, that boot_params->secure_boot should be propagated from
> > the bootloader and if the bootloader wants to set it, then we should skip the
> > check in efi_main() and go with the bootloader's opinion.  This is something
> > we probably want to do with kexec() so that the lockdown state is propagated
> > there.
>  
> Actually what I was arguing for was that if the boot loader wants to
> set it and bypass the EFI boot stub, e.g. by going via the legacy
> 64-bit entry point, startup_64, then we should allow that as well as
> setting the flag in the EFI boot stub.

That brings up another question:  Should the non-EFI entry points clear the
secure_boot mode flag and set a default?

David

Re: What should the default lockdown mode be if the bootloader sentinel triggers sanitization?

[Matt Fleming]

On Mon, 30 Jan, at 02:01:32PM, David Howells wrote:
> Matt Fleming <matt@codeblueprint.co.uk> wrote:
> 
> > > Matt argues, however, that boot_params->secure_boot should be propagated from
> > > the bootloader and if the bootloader wants to set it, then we should skip the
> > > check in efi_main() and go with the bootloader's opinion.  This is something
> > > we probably want to do with kexec() so that the lockdown state is propagated
> > > there.
> >  
> > Actually what I was arguing for was that if the boot loader wants to
> > set it and bypass the EFI boot stub, e.g. by going via the legacy
> > 64-bit entry point, startup_64, then we should allow that as well as
> > setting the flag in the EFI boot stub.
> 
> That brings up another question:  Should the non-EFI entry points clear the
> secure_boot mode flag and set a default?

There are no non-EFI boot entry points. EFI worked before we added the
EFI boot stub. The boot stub just provides new features (and allows us
to bundle firmware/boot fixes workarounds with kernel updates).

This is exactly why we should allow, or at least not actively
prohibit, the boot loader to set ->secure_boot and jump to the old
entry point if it wants to do that.

Re: [PATCH 5/8] efi: Get the secure boot status [ver #6]

[David Howells]

Matt Fleming <matt@codeblueprint.co.uk> wrote:

> -  grub_memcpy (params, &lh, 2 * 512);
> +  grub_memcpy (params, (grub_uint8_t *)&lh[512], 512);

It would appear this change is wrong and params needs to be changed to params
+ 512 or something similar.

David