[PATCH 0/7] efi: Pass secure boot mode to kernel [ver #3]

[PATCH 0/7] efi: Pass secure boot mode to kernel [ver #3]

[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.

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-20161123-2

Note that the patches are not terminal on the branch.

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

David Howells (4):
      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

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        |    6 +-
 arch/x86/boot/compressed/head_64.S        |    8 +-
 arch/x86/include/asm/efi.h                |    5 ++
 arch/x86/include/uapi/asm/bootparam.h     |    3 +
 arch/x86/kernel/setup.c                   |    5 ++
 drivers/firmware/efi/libstub/Makefile     |    2 -
 drivers/firmware/efi/libstub/arm-stub.c   |   46 --------------
 drivers/firmware/efi/libstub/secureboot.c |   93 +++++++++++++++++++++++++++++
 include/linux/efi.h                       |   42 +++++++------
 13 files changed, 144 insertions(+), 73 deletions(-)
 create mode 100644 drivers/firmware/efi/libstub/secureboot.c

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

[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/7] arm/efi: Allow invocation of arbitrary runtime services [ver #3]

[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 1/7] efi: use typed function pointers for runtime services table [ver #3]

[David Howells]

From: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.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-QSEj5FYQhm4dnm+yROfE0A@public.gmane.org>
Signed-off-by: David Howells <dhowells-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
---

 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 4/7] efi: Add SHIM and image security database GUID definitions [ver #3]

[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-rxtnV0ftBwyoClj4AeEUq9i2O/JbrIOy@public.gmane.org>
Signed-off-by: David Howells <dhowells-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Reviewed-by: Ard Biesheuvel <ard.biesheuvel-QSEj5FYQhm4dnm+yROfE0A@public.gmane.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/7] efi: Get the secure boot status [ver #3]

[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/include/uapi/asm/bootparam.h     |    3 +
 drivers/firmware/efi/libstub/Makefile     |    2 -
 drivers/firmware/efi/libstub/arm-stub.c   |   46 -------------------
 drivers/firmware/efi/libstub/secureboot.c |   71 +++++++++++++++++++++++++++++
 include/linux/efi.h                       |    2 +
 7 files changed, 80 insertions(+), 48 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..6023b0e6f2af 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) == 1);
+
 	setup_graphics(boot_params);
 
 	setup_efi_pci(boot_params);
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/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..552ee61ddbed 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)
 {
diff --git a/drivers/firmware/efi/libstub/secureboot.c b/drivers/firmware/efi/libstub/secureboot.c
new file mode 100644
index 000000000000..466fe24f5866
--- /dev/null
+++ b/drivers/firmware/efi/libstub/secureboot.c
@@ -0,0 +1,71 @@
+/*
+ * 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.
+ */
+int efi_get_secureboot(efi_system_table_t *sys_table_arg)
+{
+	u8 val;
+	unsigned long size = sizeof(val);
+	efi_status_t status;
+
+	status = get_efi_var(efi_SecureBoot_name, &efi_variable_guid,
+			     NULL, &size, &val);
+
+	if (status != EFI_SUCCESS)
+		goto out_efi_err;
+
+	if (val == 0)
+		return 0;
+
+	status = get_efi_var(efi_SetupMode_name, &efi_variable_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;
+	}
+}
diff --git a/include/linux/efi.h b/include/linux/efi.h
index c7904556d7a8..5d6c60a9caf8 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -1477,6 +1477,8 @@ 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);
 
+int efi_get_secureboot(efi_system_table_t *sys_table_arg);
+
 /*
  * Arch code can implement the following three template macros, avoiding
  * reptition for the void/non-void return cases of {__,}efi_call_virt():

Re: [PATCH 5/7] efi: Get the secure boot status [ver #3]

[James Bottomley]

On Wed, 2016-11-23 at 12:54 +0000, 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>

Since you seem to be using this to mean "is the platform locked down?",
this looks to be no longer complete in the UEFI 2.6 world.  If
DeployedMode == 0, even if SecureBoot == 1 and SetupMode == 0, you can
remove the platform key by writing 1 to AuditMode and gain control of
the secure variables.  The lock down state becomes DeployedMode == 1,
SecureBoot == 1 and SetupMode == 0

See the diagram on page 1817

http://www.uefi.org/sites/default/files/resources/UEFI%20Spec%202_6.pdf

James

Re: [PATCH 5/7] efi: Get the secure boot status [ver #3]

[David Howells]

James Bottomley <James.Bottomley@HansenPartnership.com> wrote:

> Since you seem to be using this to mean "is the platform locked down?",
> this looks to be no longer complete in the UEFI 2.6 world.  If
> DeployedMode == 0, even if SecureBoot == 1 and SetupMode == 0, you can
> remove the platform key by writing 1 to AuditMode and gain control of
> the secure variables.  The lock down state becomes DeployedMode == 1,
> SecureBoot == 1 and SetupMode == 0
> 
> See the diagram on page 1817
> 
> http://www.uefi.org/sites/default/files/resources/UEFI%20Spec%202_6.pdf

How many pages?!

Does the DeployedMode variable not exist in older versions of the UEFI spec?

David

Re: [PATCH 5/7] efi: Get the secure boot status [ver #3]

[Ard Biesheuvel]

On 25 November 2016 at 09:30, David Howells <dhowells-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> James Bottomley <James.Bottomley-d9PhHud1JfjCXq6kfMZ53/egYHeGw8Jk@public.gmane.org> wrote:
>
>> Since you seem to be using this to mean "is the platform locked down?",
>> this looks to be no longer complete in the UEFI 2.6 world.  If
>> DeployedMode == 0, even if SecureBoot == 1 and SetupMode == 0, you can
>> remove the platform key by writing 1 to AuditMode and gain control of
>> the secure variables.  The lock down state becomes DeployedMode == 1,
>> SecureBoot == 1 and SetupMode == 0
>>
>> See the diagram on page 1817
>>
>> http://www.uefi.org/sites/default/files/resources/UEFI%20Spec%202_6.pdf
>
> How many pages?!
>
> Does the DeployedMode variable not exist in older versions of the UEFI spec?
>

No, it was added in 2.6

Re: [PATCH 5/7] efi: Get the secure boot status [ver #3]

[David Howells]

How about the attached additional patch?  Should I be checking the UEFI
version number if such is available?

David
---
commit 981110f45ba73798875af7639d0328dc2d6f9919
Author: David Howells <dhowells-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Date:   Fri Nov 25 11:52:05 2016 +0000

    efi: Handle secure boot from UEFI-2.6
    
    UEFI-2.6 adds a new variable, DeployedMode.  If it exists, this must be 1
    to engage lockdown mode.
    
    Reported-by: James Bottomley <James.Bottomley-d9PhHud1JfjCXq6kfMZ53/egYHeGw8Jk@public.gmane.org>
    Signed-off-by: David Howells <dhowells-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

diff --git a/drivers/firmware/efi/libstub/secureboot.c b/drivers/firmware/efi/libstub/secureboot.c
index ca643eba5a4b..4c3bddef4fb3 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;
@@ -62,6 +65,16 @@ int efi_get_secureboot(efi_system_table_t *sys_table_arg)
 	if (val == 1)
 		return 0;
 
+	status = get_efi_var(efi_DeployedMode_name, &efi_variable_guid,
+			     NULL, &size, &val);
+	if (status != EFI_NOT_FOUND) {
+		if (status != EFI_SUCCESS)
+			goto out_efi_err;
+
+		if (val == 1)
+			return 0;
+	}
+
 	/* 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.

Re: [PATCH 5/7] efi: Get the secure boot status [ver #3]

[Ard Biesheuvel]

On 25 November 2016 at 12:03, David Howells <dhowells-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> How about the attached additional patch?  Should I be checking the UEFI
> version number if such is available?
>

Yes. In pre-2.6, DeployedMode is not a reserved name, and so it may be
possible for someone to slip in a DeployedMode=0 on a secure boot
enabled system to trick the kernel into thinking lockdown should be
disabled.


> David
> ---
> commit 981110f45ba73798875af7639d0328dc2d6f9919
> Author: David Howells <dhowells-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Date:   Fri Nov 25 11:52:05 2016 +0000
>
>     efi: Handle secure boot from UEFI-2.6
>
>     UEFI-2.6 adds a new variable, DeployedMode.  If it exists, this must be 1
>     to engage lockdown mode.
>
>     Reported-by: James Bottomley <James.Bottomley-d9PhHud1JfjCXq6kfMZ53/egYHeGw8Jk@public.gmane.org>
>     Signed-off-by: David Howells <dhowells-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>
> diff --git a/drivers/firmware/efi/libstub/secureboot.c b/drivers/firmware/efi/libstub/secureboot.c
> index ca643eba5a4b..4c3bddef4fb3 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;
> @@ -62,6 +65,16 @@ int efi_get_secureboot(efi_system_table_t *sys_table_arg)
>         if (val == 1)
>                 return 0;
>
> +       status = get_efi_var(efi_DeployedMode_name, &efi_variable_guid,
> +                            NULL, &size, &val);
> +       if (status != EFI_NOT_FOUND) {
> +               if (status != EFI_SUCCESS)
> +                       goto out_efi_err;
> +
> +               if (val == 1)
> +                       return 0;

I think the logic is the wrong way around here. Secure Boot is enabled
if SecureBoot=1 and SetupMode=0, unless DeployedMode=0. So you should
return 0 here if val == 0, but only when running on 2.6 or later.

-- 
Ard.

Re: [PATCH 5/7] efi: Get the secure boot status [ver #3]

[David Howells]

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

> Yes. In pre-2.6, DeployedMode is not a reserved name, and so it may be
> possible for someone to slip in a DeployedMode=0 on a secure boot
> enabled system to trick the kernel into thinking lockdown should be
> disabled.

How does one get the version number?  Unfortunately, searching the document
for 'version' doesn't help as every page has that in the footer:-/

> > +               if (val == 1)
> > +                       return 0;
> 
> I think the logic is the wrong way around here. Secure Boot is enabled
> if SecureBoot=1 and SetupMode=0, unless DeployedMode=0. So you should
> return 0 here if val == 0, but only when running on 2.6 or later.

Good point.

David

Re: [PATCH 5/7] efi: Get the secure boot status [ver #3]

[Ard Biesheuvel]

On 25 November 2016 at 12:35, David Howells <dhowells@redhat.com> wrote:
> Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>
>> Yes. In pre-2.6, DeployedMode is not a reserved name, and so it may be
>> possible for someone to slip in a DeployedMode=0 on a secure boot
>> enabled system to trick the kernel into thinking lockdown should be
>> disabled.
>
> How does one get the version number?  Unfortunately, searching the document
> for 'version' doesn't help as every page has that in the footer:-/
>

There is a 'revision' field in the header ('hdr') of the EFI system
table, so something like

(sys_table_arg->hdr.revision >> 16) > 2 ||
((sys_table_arg->hdr.revision >> 16) == 2 &&
(sys_table_arg->hdr.revision & 0xffff) >= 6)

should do the trick I think


>> > +               if (val == 1)
>> > +                       return 0;
>>
>> I think the logic is the wrong way around here. Secure Boot is enabled
>> if SecureBoot=1 and SetupMode=0, unless DeployedMode=0. So you should
>> return 0 here if val == 0, but only when running on 2.6 or later.
>
> Good point.
>
> David

Re: [PATCH 5/7] efi: Get the secure boot status [ver #3]

[David Howells]

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

> There is a 'revision' field in the header ('hdr') of the EFI system
> table, so something like

Is this the same as the fw_revision in the system table?

	#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))
	...

David

Re: [PATCH 5/7] efi: Get the secure boot status [ver #3]

[Ard Biesheuvel]

On 25 November 2016 at 12:51, David Howells <dhowells@redhat.com> wrote:
> Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>
>> There is a 'revision' field in the header ('hdr') of the EFI system
>> table, so something like
>
> Is this the same as the fw_revision in the system table?
>
>         #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))
>         ...
>

Yes. And in fact, that means my example is incorrect (60 not 6 in the
minor field)

Re: [PATCH 5/7] efi: Get the secure boot status [ver #3]

[David Howells]

Okay, how about the attached?

Can these variables every be anything other than 1 or 0?  E.g. should the
check on SetupMode be that it isn't 0 rather than it is 1?

David
---
commit 6d4bb08e376045e27706c2740c0fdff0a6ec43f7
Author: David Howells <dhowells-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
Date:   Fri Nov 25 11:52:05 2016 +0000

    efi: Handle secure boot from UEFI-2.6
    
    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-d9PhHud1JfjCXq6kfMZ53/egYHeGw8Jk@public.gmane.org>
    Signed-off-by: David Howells <dhowells-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>

diff --git a/drivers/firmware/efi/libstub/secureboot.c b/drivers/firmware/efi/libstub/secureboot.c
index ca643eba5a4b..157782d1c552 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;
@@ -62,6 +65,17 @@ int efi_get_secureboot(efi_system_table_t *sys_table_arg)
 	if (val == 1)
 		return 0;
 
+	/* UEFI-2.6 requires DeployedMode to be 1. */
+	if (sys_table_arg->hdr.revision == EFI_2_60_SYSTEM_TABLE_REVISION) {
+		status = get_efi_var(efi_DeployedMode_name, &efi_variable_guid,
+				     NULL, &size, &val);
+		if (status != EFI_SUCCESS)
+			goto out_efi_err;
+
+		if (val != 1)
+			return 0;
+	}
+
 	/* 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 333d31bf55bf..563abb37f03f 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))

Re: [PATCH 5/7] efi: Get the secure boot status [ver #3]

[Ard Biesheuvel]

On 25 November 2016 at 13:00, David Howells <dhowells-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org> wrote:
> Okay, how about the attached?
>
> Can these variables every be anything other than 1 or 0?  E.g. should the
> check on SetupMode be that it isn't 0 rather than it is 1?
>

The firmware will ensure that these variables only ever assume the
documented values.

> David
> ---
> commit 6d4bb08e376045e27706c2740c0fdff0a6ec43f7
> Author: David Howells <dhowells-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> Date:   Fri Nov 25 11:52:05 2016 +0000
>
>     efi: Handle secure boot from UEFI-2.6
>
>     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-d9PhHud1JfjCXq6kfMZ53/egYHeGw8Jk@public.gmane.org>
>     Signed-off-by: David Howells <dhowells-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>
> diff --git a/drivers/firmware/efi/libstub/secureboot.c b/drivers/firmware/efi/libstub/secureboot.c
> index ca643eba5a4b..157782d1c552 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;
> @@ -62,6 +65,17 @@ int efi_get_secureboot(efi_system_table_t *sys_table_arg)
>         if (val == 1)
>                 return 0;
>
> +       /* UEFI-2.6 requires DeployedMode to be 1. */
> +       if (sys_table_arg->hdr.revision == EFI_2_60_SYSTEM_TABLE_REVISION) {

>=

> +               status = get_efi_var(efi_DeployedMode_name, &efi_variable_guid,
> +                                    NULL, &size, &val);
> +               if (status != EFI_SUCCESS)
> +                       goto out_efi_err;
> +
> +               if (val != 1)
> +                       return 0;

val == 0 is better imo, since that will prevent unexpected values from
being interpreted as 'secure boot 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 333d31bf55bf..563abb37f03f 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))

Re: [PATCH 5/7] efi: Get the secure boot status [ver #3]

[David Howells]

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

> > +               if (val != 1)
> > +                       return 0;
> 
> val == 0 is better imo, since that will prevent unexpected values from
> being interpreted as 'secure boot disabled'

I've made that change.

David

Re: [PATCH 5/7] efi: Get the secure boot status [ver #3]

[David Howells]

David Howells <dhowells@redhat.com> wrote:

> +	/* UEFI-2.6 requires DeployedMode to be 1. */
> +	if (sys_table_arg->hdr.revision == EFI_2_60_SYSTEM_TABLE_REVISION) {

Actually, I suspect that this should be '>='.

David

Re: [PATCH 5/7] efi: Get the secure boot status [ver #3]

[Ard Biesheuvel]

On 25 November 2016 at 16:50, David Howells <dhowells@redhat.com> wrote:
> David Howells <dhowells@redhat.com> wrote:
>
>> +     /* UEFI-2.6 requires DeployedMode to be 1. */
>> +     if (sys_table_arg->hdr.revision == EFI_2_60_SYSTEM_TABLE_REVISION) {
>
> Actually, I suspect that this should be '>='.
>

yes, I actually pointed that out in my reply (although somewhat tersely)

Re: [PATCH 5/7] efi: Get the secure boot status [ver #3]

[David Howells]

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

> yes, I actually pointed that out in my reply (although somewhat tersely)

Ah!  Sorry, it went unnoticed amongst all the other lines beginning with '>'.

David

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

[David Howells]

From: Josh Boyer <jwboyer@fedoraproject.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@fedoraproject.org>
Signed-off-by: David Howells <dhowells@redhat.com>
---

 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 466fe24f5866..ca643eba5a4b 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[] = {
  */
 int efi_get_secureboot(efi_system_table_t *sys_table_arg)
 {
-	u8 val;
+	u32 attr;
+	u8 val, moksbstate;
 	unsigned long size = sizeof(val);
 	efi_status_t status;
 
@@ -55,6 +62,21 @@ int efi_get_secureboot(efi_system_table_t *sys_table_arg)
 	if (val == 1)
 		return 0;
 
+	/* 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)
+		return 1;
+
+	if (!(attr & EFI_VARIABLE_RUNTIME_ACCESS) && moksbstate == 1)
+		return 0;
+
 	return 1;
 
 out_efi_err:

[PATCH 7/7] efi: Add EFI_SECURE_BOOT bit [ver #3]

[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 |    5 +++++
 include/linux/efi.h     |    1 +
 2 files changed, 6 insertions(+)

diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index 9c337b0e8ba7..aa972b4cb680 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -1160,6 +1160,11 @@ void __init setup_arch(char **cmdline_p)
 
 	io_delay_init();
 
+	if (IS_ENABLED(CONFIG_EFI) && boot_params.secure_boot) {
+		set_bit(EFI_SECURE_BOOT, &efi.flags);
+		pr_info("Secure boot enabled\n");
+	}
+
 	/*
 	 * Parse the ACPI tables for possible boot-time SMP configuration.
 	 */
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 5d6c60a9caf8..333d31bf55bf 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -1066,6 +1066,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
 /*