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

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

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

  - Rebased on efi/next.
  - Remove the EFI_SECURE_BOOT flag bit and defer it for later.  Don't
  - Preclear boot_params->secure_boot and don't clear it in
    sanitize_boot_params()[*]
  - Don't probe for the secure-boot mode if the boot loader gives us this
    mode (ie. if boot_params->secure_boot is non-zero).
  
    [*] There's a bug in grub2 whereby it copies too much, sets the sentinel
    	byte and triggers the sanitisation.

 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-20170131

David
---
David Howells (6):
      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
      efi: Print the secure boot status in x86 setup_arch()

Josh Boyer (1):
      efi: Disable secure boot if shim is in insecure mode


 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          |    7 ++
 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/asm-offsets.c             |    1 
 arch/x86/kernel/setup.c                   |   14 ++++
 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                       |   15 ++++
 14 files changed, 161 insertions(+), 66 deletions(-)
 create mode 100644 drivers/firmware/efi/libstub/secureboot.c

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

[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 6d3aeabbce68..f99978db6b6f 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 4d85e600db78..d2ae1f821e0c 100644
--- a/arch/x86/boot/compressed/head_64.S
+++ b/arch/x86/boot/compressed/head_64.S
@@ -264,7 +264,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
@@ -284,7 +284,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
@@ -456,14 +456,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 2/7] arm/efi: Allow invocation of arbitrary runtime services [ver #7]

[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 0b6b1633017f..e7445281e534 100644
--- a/arch/arm64/include/asm/efi.h
+++ b/arch/arm64/include/asm/efi.h
@@ -50,6 +50,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 3/7] efi: Add SHIM and image security database GUID definitions [ver #7]

[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 8c6e007a5408..58c9dd48f42a 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -611,6 +611,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 4/7] efi: Get the secure boot status [ver #7]

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

For x86, it is stored in boot_params and can be overridden by the boot
loader or kexec.  This allows secure-boot mode to be passed on to a new
kernel.

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          |    6 +++
 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 ++++
 8 files changed, 89 insertions(+), 59 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 f99978db6b6f..57c2c9c71e53 100644
--- a/arch/x86/boot/compressed/eboot.c
+++ b/arch/x86/boot/compressed/eboot.c
@@ -988,6 +988,12 @@ struct boot_params *efi_main(struct efi_config *c,
 	else
 		setup_boot_services32(efi_early);
 
+	/* If the boot loader gave us a value for secure_boot then we use that,
+	 * otherwise we ask the BIOS.
+	 */
+	if (boot_params->secure_boot == efi_secureboot_mode_unset)
+		boot_params->secure_boot = efi_get_secureboot(sys_table);
+
 	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/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 33e0e2f1a730..f7425960f6a5 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 6fca48c9e054..d4056c6be1ec 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)
 {
@@ -157,7 +111,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 */
@@ -227,19 +181,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 58c9dd48f42a..1c200cdbdc05 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -1480,6 +1480,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():

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

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 3302 bytes --]

Hi David,

[auto build test ERROR on efi/next]
[also build test ERROR on v4.10-rc6 next-20170130]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/David-Howells/efi-Pass-secure-boot-mode-to-kernel-ver-7/20170131-232202
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mfleming/efi.git next
config: arm-multi_v7_defconfig (attached as .config)
compiler: arm-linux-gnueabi-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=arm 

All errors (new ones prefixed by >>):

   In file included from drivers/firmware/efi/libstub/secureboot.c:15:0:
   drivers/firmware/efi/libstub/secureboot.c: In function 'efi_get_secureboot':
>> arch/arm/include/asm/efi.h:58:34: error: called object is not a function or function pointer
    #define efi_call_runtime(f, ...) sys_table_arg->runtime->f(__VA_ARGS__)
                                     ^
   drivers/firmware/efi/libstub/secureboot.c:27:2: note: in expansion of macro 'efi_call_runtime'
     efi_call_runtime(get_variable, \
     ^~~~~~~~~~~~~~~~
   drivers/firmware/efi/libstub/secureboot.c:41:11: note: in expansion of macro 'get_efi_var'
     status = get_efi_var(efi_SecureBoot_name, &efi_variable_guid,
              ^~~~~~~~~~~
>> arch/arm/include/asm/efi.h:58:34: error: called object is not a function or function pointer
    #define efi_call_runtime(f, ...) sys_table_arg->runtime->f(__VA_ARGS__)
                                     ^
   drivers/firmware/efi/libstub/secureboot.c:27:2: note: in expansion of macro 'efi_call_runtime'
     efi_call_runtime(get_variable, \
     ^~~~~~~~~~~~~~~~
   drivers/firmware/efi/libstub/secureboot.c:47:11: note: in expansion of macro 'get_efi_var'
     status = get_efi_var(efi_SetupMode_name, &efi_variable_guid,
              ^~~~~~~~~~~

vim +58 arch/arm/include/asm/efi.h

da58fb657 Ard Biesheuvel 2015-09-24  52  #endif /* CONFIG_EFI */
da58fb657 Ard Biesheuvel 2015-09-24  53  
81a0bc39e Roy Franz      2015-09-23  54  /* arch specific definitions used by the stub code */
81a0bc39e Roy Franz      2015-09-23  55  
81a0bc39e Roy Franz      2015-09-23  56  #define efi_call_early(f, ...)		sys_table_arg->boottime->f(__VA_ARGS__)
fc3720642 Ard Biesheuvel 2016-04-25  57  #define __efi_call_early(f, ...)	f(__VA_ARGS__)
2fb88d885 David Howells  2017-01-31 @58  #define efi_call_runtime(f, ...)	sys_table_arg->runtime->f(__VA_ARGS__)
fc3720642 Ard Biesheuvel 2016-04-25  59  #define efi_is_64bit()			(false)
81a0bc39e Roy Franz      2015-09-23  60  
3552fdf29 Lukas Wunner   2016-11-12  61  #define efi_call_proto(protocol, f, instance, ...)			\

:::::: The code at line 58 was first introduced by commit
:::::: 2fb88d8858497455db948de0d2488b35ef2bd874 arm/efi: Allow invocation of arbitrary runtime services [ver #7]

:::::: TO: David Howells <dhowells@redhat.com>
:::::: CC: 0day robot <fengguang.wu@intel.com>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 39268 bytes --]

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

[kbuild test robot]

[-- Attachment #1: Type: text/plain, Size: 3343 bytes --]

Hi David,

[auto build test ERROR on efi/next]
[also build test ERROR on v4.10-rc6 next-20170130]
[if your patch is applied to the wrong git tree, please drop us a note to help improve the system]

url:    https://github.com/0day-ci/linux/commits/David-Howells/efi-Pass-secure-boot-mode-to-kernel-ver-7/20170131-232202
base:   https://git.kernel.org/pub/scm/linux/kernel/git/mfleming/efi.git next
config: arm64-defconfig (attached as .config)
compiler: aarch64-linux-gnu-gcc (Debian 6.1.1-9) 6.1.1 20160705
reproduce:
        wget https://git.kernel.org/cgit/linux/kernel/git/wfg/lkp-tests.git/plain/sbin/make.cross -O ~/bin/make.cross
        chmod +x ~/bin/make.cross
        # save the attached .config to linux build tree
        make.cross ARCH=arm64 

All error/warnings (new ones prefixed by >>):

   In file included from drivers/firmware/efi/libstub/secureboot.c:15:0:
   drivers/firmware/efi/libstub/secureboot.c: In function 'efi_get_secureboot':
>> arch/arm64/include/asm/efi.h:52:34: error: called object is not a function or function pointer
    #define efi_call_runtime(f, ...) sys_table_arg->runtime->f(__VA_ARGS__)
                                     ^
>> drivers/firmware/efi/libstub/secureboot.c:27:2: note: in expansion of macro 'efi_call_runtime'
     efi_call_runtime(get_variable, \
     ^~~~~~~~~~~~~~~~
>> drivers/firmware/efi/libstub/secureboot.c:41:11: note: in expansion of macro 'get_efi_var'
     status = get_efi_var(efi_SecureBoot_name, &efi_variable_guid,
              ^~~~~~~~~~~
>> arch/arm64/include/asm/efi.h:52:34: error: called object is not a function or function pointer
    #define efi_call_runtime(f, ...) sys_table_arg->runtime->f(__VA_ARGS__)
                                     ^
>> drivers/firmware/efi/libstub/secureboot.c:27:2: note: in expansion of macro 'efi_call_runtime'
     efi_call_runtime(get_variable, \
     ^~~~~~~~~~~~~~~~
   drivers/firmware/efi/libstub/secureboot.c:47:11: note: in expansion of macro 'get_efi_var'
     status = get_efi_var(efi_SetupMode_name, &efi_variable_guid,
              ^~~~~~~~~~~

vim +52 arch/arm64/include/asm/efi.h

a13b00778 Ard Biesheuvel 2014-07-02  46   */
a13b00778 Ard Biesheuvel 2014-07-02  47  #define EFI_FDT_ALIGN	SZ_2M   /* used by allocate_new_fdt_and_exit_boot() */
a13b00778 Ard Biesheuvel 2014-07-02  48  #define MAX_FDT_OFFSET	SZ_512M
a13b00778 Ard Biesheuvel 2014-07-02  49  
a13b00778 Ard Biesheuvel 2014-07-02  50  #define efi_call_early(f, ...)		sys_table_arg->boottime->f(__VA_ARGS__)
fc3720642 Ard Biesheuvel 2016-04-25  51  #define __efi_call_early(f, ...)	f(__VA_ARGS__)
2fb88d885 David Howells  2017-01-31 @52  #define efi_call_runtime(f, ...)	sys_table_arg->runtime->f(__VA_ARGS__)
fc3720642 Ard Biesheuvel 2016-04-25  53  #define efi_is_64bit()			(true)
a13b00778 Ard Biesheuvel 2014-07-02  54  
3552fdf29 Lukas Wunner   2016-11-12  55  #define efi_call_proto(protocol, f, instance, ...)			\

:::::: The code at line 52 was first introduced by commit
:::::: 2fb88d8858497455db948de0d2488b35ef2bd874 arm/efi: Allow invocation of arbitrary runtime services [ver #7]

:::::: TO: David Howells <dhowells@redhat.com>
:::::: CC: 0day robot <fengguang.wu@intel.com>

---
0-DAY kernel test infrastructure                Open Source Technology Center
https://lists.01.org/pipermail/kbuild-all                   Intel Corporation

[-- Attachment #2: .config.gz --]
[-- Type: application/gzip, Size: 32973 bytes --]

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

[Matt Fleming]

On Tue, 31 Jan, at 03:13:49PM, David Howells wrote:
> diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
> index f99978db6b6f..57c2c9c71e53 100644
> --- a/arch/x86/boot/compressed/eboot.c
> +++ b/arch/x86/boot/compressed/eboot.c
> @@ -988,6 +988,12 @@ struct boot_params *efi_main(struct efi_config *c,
>  	else
>  		setup_boot_services32(efi_early);
>  
> +	/* If the boot loader gave us a value for secure_boot then we use that,
> +	 * otherwise we ask the BIOS.
> +	 */
> +	if (boot_params->secure_boot == efi_secureboot_mode_unset)
> +		boot_params->secure_boot = efi_get_secureboot(sys_table);
> +
>  	setup_graphics(boot_params);
>  
>  	setup_efi_pci(boot_params);

It's not a big deal, but this multi-line comment format isn't correct.
Either Ard or I will fix it up when applying this patch. Same goes for
the comment in patch 5.

Otherwise this patch looks fine.

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

[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 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 6/7] efi: Handle secure boot from UEFI-2.6 [ver #7]

[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 1c200cdbdc05..87c1a6993f17 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -646,6 +646,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 6/7] efi: Handle secure boot from UEFI-2.6 [ver #7]

[Ard Biesheuvel]

On 31 January 2017 at 15:14, David Howells <dhowells@redhat.com> wrote:
> 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>

Interestingly, the string 'DeployedMode' appears zero times in the
EDK2 codebase, so I wonder if it makes any sense to merge this now.
The string 'AuditMode' does appear once, but in a comment

In any case, the logic is not entirely correct either: apologies if it
was me who caused any confusion here, but it seems DeployedMode could
legally be 0 or 1 while secure boot is in fact enabled. It is actually
AuditMode that should be taken into account here, i.e., if AuditMode
== 1, the firmware ignores invalid or missing signatures. If
SecureBoot == 0x1, SetupMode == 0x0 and AuditMode == 0x0 (or
non-existent), signature verification is performed regardless of the
value (or existence) of DeployedMode.

So I propose to respin this patch to treat AuditMode == 0x1 as 'secure
boot disabled', and ignore if it is missing.

> ---
>
>  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 1c200cdbdc05..87c1a6993f17 100644
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -646,6 +646,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 6/7] efi: Handle secure boot from UEFI-2.6 [ver #7]

[David Howells]

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

> > 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>
> 
> Interestingly, the string 'DeployedMode' appears zero times in the
> EDK2 codebase, so I wonder if it makes any sense to merge this now.
> The string 'AuditMode' does appear once, but in a comment

It's in the standard, so shouldn't we check for it?

> In any case, the logic is not entirely correct either: apologies if it
> was me who caused any confusion here, but it seems DeployedMode could
> legally be 0 or 1 while secure boot is in fact enabled. It is actually
> AuditMode that should be taken into account here, i.e., if AuditMode
> == 1, the firmware ignores invalid or missing signatures. If
> SecureBoot == 0x1, SetupMode == 0x0 and AuditMode == 0x0 (or
> non-existent), signature verification is performed regardless of the
> value (or existence) of DeployedMode.
> 
> So I propose to respin this patch to treat AuditMode == 0x1 as 'secure
> boot disabled', and ignore if it is missing.

Ummm...  This might conflict what said:

| 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

Looking again at that diagram, should I be checking all four variables
(SecureBoot, SetupMode, DeployedMode and AuditMode)?  And/or should I treat
audit mode differently to deployed mode?

Further, there doesn't seem to be a state in which SecureBoot is shown as
being 1.

David

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

[Ard Biesheuvel]

On 31 January 2017 at 18:59, David Howells <dhowells@redhat.com> wrote:
> Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>
>> > 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>
>>
>> Interestingly, the string 'DeployedMode' appears zero times in the
>> EDK2 codebase, so I wonder if it makes any sense to merge this now.
>> The string 'AuditMode' does appear once, but in a comment
>
> It's in the standard, so shouldn't we check for it?
>
>> In any case, the logic is not entirely correct either: apologies if it
>> was me who caused any confusion here, but it seems DeployedMode could
>> legally be 0 or 1 while secure boot is in fact enabled. It is actually
>> AuditMode that should be taken into account here, i.e., if AuditMode
>> == 1, the firmware ignores invalid or missing signatures. If
>> SecureBoot == 0x1, SetupMode == 0x0 and AuditMode == 0x0 (or
>> non-existent), signature verification is performed regardless of the
>> value (or existence) of DeployedMode.
>>
>> So I propose to respin this patch to treat AuditMode == 0x1 as 'secure
>> boot disabled', and ignore if it is missing.
>
> Ummm...  This might conflict what said:
>
> | 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
>
> Looking again at that diagram, should I be checking all four variables
> (SecureBoot, SetupMode, DeployedMode and AuditMode)?  And/or should I treat
> audit mode differently to deployed mode?
>

Well, we are trying to decide whether the system is locked down or
not. AuditMode is only writable before ExitBootServices(), and when
AuditMode == 0, signature verification occurs as usual, regardless of
the value of DeployedMode. Whether someone could turn on AuditMode on
the *next* boot does not sound that relevant to me, since someone
could also re-enter SetupMode in the same way.

So this patch should take AuditMode into account, but not DeployedMode, i.e.,

SecureBoot == 0x1
SetupMode == 0x0
AuditMode == 0x0 (or non-existent)

implies a locked down state.

> Further, there doesn't seem to be a state in which SecureBoot is shown as
> being 1.
>

Yes, that is sloppy. But the fact that EDK2, being the v2.6 reference,
does not implement any of this *at all* is much more worrying to me,
given that UDK2017 based firmware will certainly turn up in production
systems.

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

[David Howells]

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

> So this patch should take AuditMode into account, but not DeployedMode, i.e.,
> 
> SecureBoot == 0x1
> SetupMode == 0x0
> AuditMode == 0x0 (or non-existent)

If we're in audit mode or setup mode SecureBoot==0 and SetupMode==1 according
to the flowchart, so the check of AuditMode would seem redundant.

Further, the checks above don't seem to differentiate deployed mode from user
mode.  Should they?

David

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

[Ard Biesheuvel]

On 1 February 2017 at 12:33, David Howells <dhowells@redhat.com> wrote:
> Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>
>> So this patch should take AuditMode into account, but not DeployedMode, i.e.,
>>
>> SecureBoot == 0x1
>> SetupMode == 0x0
>> AuditMode == 0x0 (or non-existent)
>
> If we're in audit mode or setup mode SecureBoot==0 and SetupMode==1 according
> to the flowchart, so the check of AuditMode would seem redundant.
>
> Further, the checks above don't seem to differentiate deployed mode from user
> mode.  Should they?
>

>From the OS pov, UserMode and DeployedMode are the same, the only
difference being that AuditMode may be entered from UserMode simply by
setting the variable to 0x1 (which can only be done before
ExitBootServices()). And since AuditMode implies SetupMode (according
to the diagram), you are right that we don't need to care about
AuditMode either. AFAICT, that makes the entire patch unnecessary, so
let's drop it for now.

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

[David Howells]

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

> From the OS pov, UserMode and DeployedMode are the same, the only
> difference being that AuditMode may be entered from UserMode simply by
> setting the variable to 0x1 (which can only be done before
> ExitBootServices()). And since AuditMode implies SetupMode (according
> to the diagram), you are right that we don't need to care about
> AuditMode either. AFAICT, that makes the entire patch unnecessary, so
> let's drop it for now.

Okay, in that case, do you want me to reissue and place a signed tag on my
patchset without that patch, or can you pull the other patches individually?

David

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

[Ard Biesheuvel]

On 1 February 2017 at 15:00, David Howells <dhowells@redhat.com> wrote:
> Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>
>> From the OS pov, UserMode and DeployedMode are the same, the only
>> difference being that AuditMode may be entered from UserMode simply by
>> setting the variable to 0x1 (which can only be done before
>> ExitBootServices()). And since AuditMode implies SetupMode (according
>> to the diagram), you are right that we don't need to care about
>> AuditMode either. AFAICT, that makes the entire patch unnecessary, so
>> let's drop it for now.
>
> Okay, in that case, do you want me to reissue and place a signed tag on my
> patchset without that patch, or can you pull the other patches individually?
>

Let's wait for Matt to comment on the x86 bits before reissuing
anything, but if the subsequent patches still apply cleanly, I don't
think there is a need to resend or re-sign.

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

[Matt Fleming]

On Wed, 01 Feb, at 03:02:14PM, Ard Biesheuvel wrote:
> 
> Let's wait for Matt to comment on the x86 bits before reissuing
> anything, but if the subsequent patches still apply cleanly, I don't
> think there is a need to resend or re-sign.

This all looks fine to me now, thanks for the re-work David.

Ard, do you want to apply these patches along with a fixup for the
multi-line comments and send a second v4.11 pull to tip?

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

[Ard Biesheuvel]

> On 2 Feb 2017, at 21:36, Matt Fleming <matt@codeblueprint.co.uk> wrote:
> 
>> On Wed, 01 Feb, at 03:02:14PM, Ard Biesheuvel wrote:
>> 
>> Let's wait for Matt to comment on the x86 bits before reissuing
>> anything, but if the subsequent patches still apply cleanly, I don't
>> think there is a need to resend or re-sign.
> 
> This all looks fine to me now, thanks for the re-work David.
> 
> Ard, do you want to apply these patches along with a fixup for the
> multi-line comments and send a second v4.11 pull to tip?

Sure, I'll take care of it tomorrow

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

[David Howells]

David Howells <dhowells@redhat.com> wrote:

> Ummm...  This might conflict what said:

Might conflict with what James said, I should've said.

David

[PATCH 7/7] efi: Print the secure boot status in x86 setup_arch() [ver #7]

[David Howells]

Print the secure boot status in the x86 setup_arch() but otherwise do
nothing more for now.  More functionality will be added later, but this at
least allows for testing.

Signed-off-by: David Howells <dhowells@redhat.com>
---

 arch/x86/kernel/setup.c |   14 ++++++++++++++
 1 file changed, 14 insertions(+)

diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index 4cfba947d774..22e4b47a5c14 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -1176,6 +1176,20 @@ 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:
+			pr_info("Secure boot enabled\n");
+			break;
+		default:
+			pr_info("Secure boot could not be determined\n");
+			break;
+		}
+	}
+
 	reserve_initrd();
 
 	acpi_table_upgrade();

Re: [PATCH 7/7] efi: Print the secure boot status in x86 setup_arch() [ver #7]

[Ard Biesheuvel]

On 31 January 2017 at 15:14, David Howells <dhowells@redhat.com> wrote:
> Print the secure boot status in the x86 setup_arch() but otherwise do
> nothing more for now.  More functionality will be added later, but this at
> least allows for testing.
>
> Signed-off-by: David Howells <dhowells@redhat.com>
> ---
>
>  arch/x86/kernel/setup.c |   14 ++++++++++++++
>  1 file changed, 14 insertions(+)
>
> diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
> index 4cfba947d774..22e4b47a5c14 100644
> --- a/arch/x86/kernel/setup.c
> +++ b/arch/x86/kernel/setup.c
> @@ -1176,6 +1176,20 @@ void __init setup_arch(char **cmdline_p)
>         /* Allocate bigger log buffer */
>         setup_log_buf(1);
>
> +       if (IS_ENABLED(CONFIG_EFI)) {

Shouldn't this be a runtime check?

> +               switch (boot_params.secure_boot) {
> +               case efi_secureboot_mode_disabled:
> +                       pr_info("Secure boot disabled\n");
> +                       break;
> +               case efi_secureboot_mode_enabled:
> +                       pr_info("Secure boot enabled\n");
> +                       break;
> +               default:
> +                       pr_info("Secure boot could not be determined\n");
> +                       break;
> +               }
> +       }
> +
>         reserve_initrd();
>
>         acpi_table_upgrade();
>

Re: [PATCH 7/7] efi: Print the secure boot status in x86 setup_arch() [ver #7]

[David Howells]

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

> > +       if (IS_ENABLED(CONFIG_EFI)) {
> 
> Shouldn't this be a runtime check?

Interesting question.  The original patch I was working from had a #ifdef
here.  Possibly it doesn't need to be there at all.  We could rely entirely on
the value of boot_params.secure_boot.

David

Re: [PATCH 7/7] efi: Print the secure boot status in x86 setup_arch() [ver #7]

[Ard Biesheuvel]

On 3 February 2017 at 16:21, David Howells <dhowells@redhat.com> wrote:
> Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>
>> > +       if (IS_ENABLED(CONFIG_EFI)) {
>>
>> Shouldn't this be a runtime check?
>
> Interesting question.  The original patch I was working from had a #ifdef
> here.  Possibly it doesn't need to be there at all.  We could rely entirely on
> the value of boot_params.secure_boot.
>

Yes, but only if you are booting via UEFI, no? So perhaps use
efi_enabled(EFI_BOOT) instead?

Re: [PATCH 7/7] efi: Print the secure boot status in x86 setup_arch() [ver #7]

[David Howells]

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

> Yes, but only if you are booting via UEFI, no?

Why limit it so?  Even if you don't boot via UEFI, the bootloader/kexec can
always set the secure-boot state on.

> So perhaps use efi_enabled(EFI_BOOT) instead?

I've no objection to that, given it incorporates a test of CONFIG_EFI.

David

Re: [PATCH 7/7] efi: Print the secure boot status in x86 setup_arch() [ver #7]

[David Howells]

David Howells <dhowells@redhat.com> wrote:

> Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> 
> > Yes, but only if you are booting via UEFI, no?
> 
> Why limit it so?  Even if you don't boot via UEFI, the bootloader/kexec can
> always set the secure-boot state on.
> 
> > So perhaps use efi_enabled(EFI_BOOT) instead?
> 
> I've no objection to that, given it incorporates a test of CONFIG_EFI.

Feel free to just go ahead and change it in the patch.  We can always take the
check out later.

David

Re: [PATCH 7/7] efi: Print the secure boot status in x86 setup_arch() [ver #7]

[Ard Biesheuvel]

On 3 February 2017 at 16:29, David Howells <dhowells@redhat.com> wrote:
> David Howells <dhowells@redhat.com> wrote:
>
>> Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>>
>> > Yes, but only if you are booting via UEFI, no?
>>
>> Why limit it so?  Even if you don't boot via UEFI, the bootloader/kexec can
>> always set the secure-boot state on.
>>
>> > So perhaps use efi_enabled(EFI_BOOT) instead?
>>
>> I've no objection to that, given it incorporates a test of CONFIG_EFI.
>
> Feel free to just go ahead and change it in the patch.  We can always take the
> check out later.
>

Sure

Re: [PATCH 7/7] efi: Print the secure boot status in x86 setup_arch() [ver #7]

[Ard Biesheuvel]

On 3 February 2017 at 16:29, Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
> On 3 February 2017 at 16:29, David Howells <dhowells@redhat.com> wrote:
>> David Howells <dhowells@redhat.com> wrote:
>>
>>> Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>>>
>>> > Yes, but only if you are booting via UEFI, no?
>>>
>>> Why limit it so?  Even if you don't boot via UEFI, the bootloader/kexec can
>>> always set the secure-boot state on.
>>>
>>> > So perhaps use efi_enabled(EFI_BOOT) instead?
>>>
>>> I've no objection to that, given it incorporates a test of CONFIG_EFI.
>>
>> Feel free to just go ahead and change it in the patch.  We can always take the
>> check out later.
>>
>
> Sure

OK, I have queued these patches (minus the DeployedMode one) in the
next branch on efi.git. Please double check, I will send out a pull
request to tip shortly (once the autobuilder gives me the green light)

Thanks,
Ard.

Re: [PATCH 7/7] efi: Print the secure boot status in x86 setup_arch() [ver #7]

[David Howells]

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

> OK, I have queued these patches (minus the DeployedMode one) in the
> next branch on efi.git. Please double check, I will send out a pull
> request to tip shortly (once the autobuilder gives me the green light)

Looks okay.  Now if I can just squeeze a fixed grub2 out of Peter Jones...

David

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

[Ard Biesheuvel]

Hello David,

On 31 January 2017 at 15:13, David Howells <dhowells@redhat.com> 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.
>

Could you please resend with the [ver #7] removed from the subject
lines? It is a bit tedious to have to manually modify each patch when
applying.

Thanks,
Ard.

> Changes:
>
>  Ver 7:
>
>   - Rebased on efi/next.
>   - Remove the EFI_SECURE_BOOT flag bit and defer it for later.  Don't
>   - Preclear boot_params->secure_boot and don't clear it in
>     sanitize_boot_params()[*]
>   - Don't probe for the secure-boot mode if the boot loader gives us this
>     mode (ie. if boot_params->secure_boot is non-zero).
>
>     [*] There's a bug in grub2 whereby it copies too much, sets the sentinel
>         byte and triggers the sanitisation.
>
>  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-20170131
>
> David
> ---
> David Howells (6):
>       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
>       efi: Print the secure boot status in x86 setup_arch()
>
> Josh Boyer (1):
>       efi: Disable secure boot if shim is in insecure mode
>
>
>  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          |    7 ++
>  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/asm-offsets.c             |    1
>  arch/x86/kernel/setup.c                   |   14 ++++
>  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                       |   15 ++++
>  14 files changed, 161 insertions(+), 66 deletions(-)
>  create mode 100644 drivers/firmware/efi/libstub/secureboot.c
>

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

[David Howells]

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

> Could you please resend with the [ver #7] removed from the subject
> lines? It is a bit tedious to have to manually modify each patch when
> applying.

I presume you don't want to just pull the branch or tag?

David

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

[Ard Biesheuvel]

On 31 January 2017 at 17:04, David Howells <dhowells@redhat.com> wrote:
> Ard Biesheuvel <ard.biesheuvel@linaro.org> wrote:
>
>> Could you please resend with the [ver #7] removed from the subject
>> lines? It is a bit tedious to have to manually modify each patch when
>> applying.
>

This looks mostly fine to me (modulo the DeployedMode patch I just
replied to) but Matt needs to look at the x86 bits.