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 5:
- Fix i386 compilation error (rsi should've been changed to esi).
- Fix arm64 compilation error ('sys_table_arg' is a hidden macro parameter).
Ver 4:
- Use an enum to tell the kernel whether secure boot mode is enabled,
disabled, couldn't be determined or wasn't even tried due to not being
in EFI mode.
- Support the UEFI-2.6 DeployedMode flag.
- Don't clear boot_params->secure_boot in x86 sanitize_boot_params().
- Preclear the boot_params->secure_boot on x86 head_*.S entry if we may
not go through efi_main().
The patches can be found here also:
http://git.kernel.org/cgit/linux/kernel/git/dhowells/linux-fs.git/log/?h=efi-secure-boot
at tag:
efi-secure-boot-20161207-2
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 (5):
x86/efi: Allow invocation of arbitrary runtime services
arm/efi: Allow invocation of arbitrary runtime services
efi: Add SHIM and image security database GUID definitions
efi: Get the secure boot status
efi: Handle secure boot from UEFI-2.6
Josh Boyer (2):
efi: Disable secure boot if shim is in insecure mode
efi: Add EFI_SECURE_BOOT bit
Documentation/x86/zero-page.txt | 2 +
arch/arm/include/asm/efi.h | 1
arch/arm64/include/asm/efi.h | 1
arch/x86/boot/compressed/eboot.c | 3 +
arch/x86/boot/compressed/head_32.S | 7 +-
arch/x86/boot/compressed/head_64.S | 9 +--
arch/x86/include/asm/bootparam_utils.h | 5 +
arch/x86/include/asm/efi.h | 5 +
arch/x86/include/uapi/asm/bootparam.h | 3 +
arch/x86/kernel/asm-offsets.c | 1
arch/x86/kernel/setup.c | 15 ++++
drivers/firmware/efi/libstub/Makefile | 2 -
drivers/firmware/efi/libstub/arm-stub.c | 58 +---------------
drivers/firmware/efi/libstub/secureboot.c | 102 +++++++++++++++++++++++++++++
include/linux/efi.h | 52 ++++++++++-----
15 files changed, 182 insertions(+), 84 deletions(-)
create mode 100644 drivers/firmware/efi/libstub/secureboot.c
From: Ard Biesheuvel <[email protected]>
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 <[email protected]>
Signed-off-by: David Howells <[email protected]>
---
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);
/*
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 <[email protected]>
Signed-off-by: David Howells <[email protected]>
---
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
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 <[email protected]>
Signed-off-by: David Howells <[email protected]>
---
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, ...) \
Add the definitions for shim and image security database, both of which
are used widely in various Linux distros.
Signed-off-by: Josh Boyer <[email protected]>
Signed-off-by: David Howells <[email protected]>
Reviewed-by: Ard Biesheuvel <[email protected]>
---
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
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 <[email protected]>
Signed-off-by: David Howells <[email protected]>
---
Documentation/x86/zero-page.txt | 2 +
arch/x86/boot/compressed/eboot.c | 2 +
arch/x86/boot/compressed/head_32.S | 1
arch/x86/boot/compressed/head_64.S | 1
arch/x86/include/asm/bootparam_utils.h | 5 +-
arch/x86/include/uapi/asm/bootparam.h | 3 +
arch/x86/kernel/asm-offsets.c | 1
drivers/firmware/efi/libstub/Makefile | 2 -
drivers/firmware/efi/libstub/arm-stub.c | 58 +------------------------
drivers/firmware/efi/libstub/secureboot.c | 66 +++++++++++++++++++++++++++++
include/linux/efi.h | 8 ++++
11 files changed, 90 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 c8c32ebcdfdb..5b151c262ac2 100644
--- a/arch/x86/boot/compressed/eboot.c
+++ b/arch/x86/boot/compressed/eboot.c
@@ -1158,6 +1158,8 @@ struct boot_params *efi_main(struct efi_config *c,
else
setup_boot_services32(efi_early);
+ boot_params->secure_boot = efi_get_secureboot(sys_table);
+
setup_graphics(boot_params);
setup_efi_pci(boot_params);
diff --git a/arch/x86/boot/compressed/head_32.S b/arch/x86/boot/compressed/head_32.S
index d85b9625e836..c635f7e32f5c 100644
--- a/arch/x86/boot/compressed/head_32.S
+++ b/arch/x86/boot/compressed/head_32.S
@@ -61,6 +61,7 @@
__HEAD
ENTRY(startup_32)
+ movb $0, BP_secure_boot(%esi)
#ifdef CONFIG_EFI_STUB
jmp preferred_addr
diff --git a/arch/x86/boot/compressed/head_64.S b/arch/x86/boot/compressed/head_64.S
index beab8322f72a..ccd2c7461b7f 100644
--- a/arch/x86/boot/compressed/head_64.S
+++ b/arch/x86/boot/compressed/head_64.S
@@ -244,6 +244,7 @@ ENTRY(startup_64)
* that maps our entire kernel(text+data+bss+brk), zero page
* and command line.
*/
+ movb $0, BP_secure_boot(%rsi)
#ifdef CONFIG_EFI_STUB
/*
* The entry point for the PE/COFF executable is efi_pe_entry, so
diff --git a/arch/x86/include/asm/bootparam_utils.h b/arch/x86/include/asm/bootparam_utils.h
index 4a8cb8d7cbd5..7e16d53ff6a3 100644
--- a/arch/x86/include/asm/bootparam_utils.h
+++ b/arch/x86/include/asm/bootparam_utils.h
@@ -38,9 +38,10 @@ static void sanitize_boot_params(struct boot_params *boot_params)
memset(&boot_params->ext_ramdisk_image, 0,
(char *)&boot_params->efi_info -
(char *)&boot_params->ext_ramdisk_image);
- memset(&boot_params->kbd_status, 0,
+ boot_params->kbd_status = 0;
+ memset(&boot_params->_pad5, 0,
(char *)&boot_params->hdr -
- (char *)&boot_params->kbd_status);
+ (char *)&boot_params->_pad5);
memset(&boot_params->_pad7[0], 0,
(char *)&boot_params->edd_mbr_sig_buffer[0] -
(char *)&boot_params->_pad7[0]);
diff --git a/arch/x86/include/uapi/asm/bootparam.h b/arch/x86/include/uapi/asm/bootparam.h
index b10bf319ed20..5138dacf8bb8 100644
--- a/arch/x86/include/uapi/asm/bootparam.h
+++ b/arch/x86/include/uapi/asm/bootparam.h
@@ -135,7 +135,8 @@ struct boot_params {
__u8 eddbuf_entries; /* 0x1e9 */
__u8 edd_mbr_sig_buf_entries; /* 0x1ea */
__u8 kbd_status; /* 0x1eb */
- __u8 _pad5[3]; /* 0x1ec */
+ __u8 secure_boot; /* 0x1ec */
+ __u8 _pad5[2]; /* 0x1ed */
/*
* The sentinel is set to a nonzero value (0xff) in header.S.
*
diff --git a/arch/x86/kernel/asm-offsets.c b/arch/x86/kernel/asm-offsets.c
index c62e015b126c..de827d6ac8c2 100644
--- a/arch/x86/kernel/asm-offsets.c
+++ b/arch/x86/kernel/asm-offsets.c
@@ -81,6 +81,7 @@ void common(void) {
BLANK();
OFFSET(BP_scratch, boot_params, scratch);
+ OFFSET(BP_secure_boot, boot_params, secure_boot);
OFFSET(BP_loadflags, boot_params, hdr.loadflags);
OFFSET(BP_hardware_subarch, boot_params, hdr.hardware_subarch);
OFFSET(BP_version, boot_params, hdr.version);
diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile
index 6621b13c370f..9af966863612 100644
--- a/drivers/firmware/efi/libstub/Makefile
+++ b/drivers/firmware/efi/libstub/Makefile
@@ -28,7 +28,7 @@ OBJECT_FILES_NON_STANDARD := y
# Prevents link failures: __sanitizer_cov_trace_pc() is not linked in.
KCOV_INSTRUMENT := n
-lib-y := efi-stub-helper.o gop.o
+lib-y := efi-stub-helper.o gop.o secureboot.o
# include the stub's generic dependencies from lib/ when building for ARM/arm64
arm-deps := fdt_rw.c fdt_ro.c fdt_wip.c fdt.c fdt_empty_tree.c fdt_sw.c sort.c
diff --git a/drivers/firmware/efi/libstub/arm-stub.c b/drivers/firmware/efi/libstub/arm-stub.c
index b4f7d78f9e8b..06d503419071 100644
--- a/drivers/firmware/efi/libstub/arm-stub.c
+++ b/drivers/firmware/efi/libstub/arm-stub.c
@@ -20,52 +20,6 @@
bool __nokaslr;
-static int efi_get_secureboot(efi_system_table_t *sys_table_arg)
-{
- static efi_char16_t const sb_var_name[] = {
- 'S', 'e', 'c', 'u', 'r', 'e', 'B', 'o', 'o', 't', 0 };
- static efi_char16_t const sm_var_name[] = {
- 'S', 'e', 't', 'u', 'p', 'M', 'o', 'd', 'e', 0 };
-
- efi_guid_t var_guid = EFI_GLOBAL_VARIABLE_GUID;
- efi_get_variable_t *f_getvar = sys_table_arg->runtime->get_variable;
- u8 val;
- unsigned long size = sizeof(val);
- efi_status_t status;
-
- status = f_getvar((efi_char16_t *)sb_var_name, (efi_guid_t *)&var_guid,
- NULL, &size, &val);
-
- if (status != EFI_SUCCESS)
- goto out_efi_err;
-
- if (val == 0)
- return 0;
-
- status = f_getvar((efi_char16_t *)sm_var_name, (efi_guid_t *)&var_guid,
- NULL, &size, &val);
-
- if (status != EFI_SUCCESS)
- goto out_efi_err;
-
- if (val == 1)
- return 0;
-
- return 1;
-
-out_efi_err:
- switch (status) {
- case EFI_NOT_FOUND:
- return 0;
- case EFI_DEVICE_ERROR:
- return -EIO;
- case EFI_SECURITY_VIOLATION:
- return -EACCES;
- default:
- return -EINVAL;
- }
-}
-
efi_status_t efi_open_volume(efi_system_table_t *sys_table_arg,
void *__image, void **__fh)
{
@@ -226,7 +180,7 @@ unsigned long efi_entry(void *handle, efi_system_table_t *sys_table,
efi_guid_t loaded_image_proto = LOADED_IMAGE_PROTOCOL_GUID;
unsigned long reserve_addr = 0;
unsigned long reserve_size = 0;
- int secure_boot = 0;
+ enum efi_secureboot_mode secure_boot = efi_secureboot_mode_unknown;
struct screen_info *si;
/* Check if we were booted by the EFI firmware */
@@ -296,19 +250,13 @@ 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.
*/
- 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..70e2a36577d4
--- /dev/null
+++ b/drivers/firmware/efi/libstub/secureboot.c
@@ -0,0 +1,66 @@
+/*
+ * Secure boot handling.
+ *
+ * Copyright (C) 2013,2014 Linaro Limited
+ * Roy Franz <[email protected]
+ * Copyright (C) 2013 Red Hat, Inc.
+ * Mark Salter <[email protected]>
+ *
+ * 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. We return:
+ */
+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)
+ goto secure_boot_disabled;
+
+ pr_efi(sys_table_arg, "UEFI Secure Boot is enabled.\n");
+ return efi_secureboot_mode_enabled;
+
+secure_boot_disabled:
+ return efi_secureboot_mode_disabled;
+
+out_efi_err:
+ pr_efi_err(sys_table_arg, "Could not determine UEFI Secure Boot status.\n");
+ if (status == EFI_NOT_FOUND)
+ goto secure_boot_disabled;
+ return efi_secureboot_mode_unknown;
+}
diff --git a/include/linux/efi.h b/include/linux/efi.h
index c7904556d7a8..92e23f03045e 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -1477,6 +1477,14 @@ efi_status_t efi_setup_gop(efi_system_table_t *sys_table_arg,
bool efi_runtime_disabled(void);
extern void efi_call_virt_check_flags(unsigned long flags, const char *call);
+enum efi_secureboot_mode {
+ efi_secureboot_mode_unset,
+ efi_secureboot_mode_unknown,
+ efi_secureboot_mode_disabled,
+ efi_secureboot_mode_enabled,
+};
+enum efi_secureboot_mode efi_get_secureboot(efi_system_table_t *sys_table);
+
/*
* Arch code can implement the following three template macros, avoiding
* reptition for the void/non-void return cases of {__,}efi_call_virt():
From: Josh Boyer <[email protected]>
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 <[email protected]>
Signed-off-by: David Howells <[email protected]>
---
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 70e2a36577d4..ba6ef717c66f 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)
goto 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.
+ */
+ 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)
+ goto secure_boot_disabled;
+
+secure_boot_enabled:
pr_efi(sys_table_arg, "UEFI Secure Boot is enabled.\n");
return efi_secureboot_mode_enabled;
From: Josh Boyer <[email protected]>
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 <[email protected]>
Signed-off-by: David Howells <[email protected]>
---
arch/x86/kernel/setup.c | 15 +++++++++++++++
include/linux/efi.h | 1 +
2 files changed, 16 insertions(+)
diff --git a/arch/x86/kernel/setup.c b/arch/x86/kernel/setup.c
index 9c337b0e8ba7..d8972ec6257d 100644
--- a/arch/x86/kernel/setup.c
+++ b/arch/x86/kernel/setup.c
@@ -1152,6 +1152,21 @@ void __init setup_arch(char **cmdline_p)
/* Allocate bigger log buffer */
setup_log_buf(1);
+ if (IS_ENABLED(CONFIG_EFI)) {
+ switch (boot_params.secure_boot) {
+ case efi_secureboot_mode_disabled:
+ pr_info("Secure boot disabled\n");
+ break;
+ case efi_secureboot_mode_enabled:
+ set_bit(EFI_SECURE_BOOT, &efi.flags);
+ pr_info("Secure boot enabled\n");
+ break;
+ default:
+ pr_info("Secure boot could not be determined\n");
+ break;
+ }
+ }
+
reserve_initrd();
acpi_table_upgrade();
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 92e23f03045e..135ca9c0c0b5 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
/*
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 <[email protected]>
Signed-off-by: David Howells <[email protected]>
---
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 ba6ef717c66f..333b1594cce4 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)
goto secure_boot_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)
+ goto 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 135ca9c0c0b5..e1893f5002c3 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))
On Wed, Dec 07, 2016 at 01:18:39PM +0000, David Howells wrote:
> @@ -226,7 +180,7 @@ unsigned long efi_entry(void *handle, efi_system_table_t *sys_table,
> efi_guid_t loaded_image_proto = LOADED_IMAGE_PROTOCOL_GUID;
> unsigned long reserve_addr = 0;
> unsigned long reserve_size = 0;
> - int secure_boot = 0;
> + enum efi_secureboot_mode secure_boot = efi_secureboot_mode_unknown;
You're setting this variable unconditionally further down, so no need
to initialize it.
> +/*
> + * Determine whether we're in secure boot mode. We return:
> + */
We return? Looks like something's missing here.
> +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)
> + goto secure_boot_disabled;
Well, you could just return efi_secureboot_mode_disabled directly here
instead of doing a jump.
> +
> + pr_efi(sys_table_arg, "UEFI Secure Boot is enabled.\n");
> + return efi_secureboot_mode_enabled;
> +
> +secure_boot_disabled:
> + return efi_secureboot_mode_disabled;
> +
> +out_efi_err:
> + pr_efi_err(sys_table_arg, "Could not determine UEFI Secure Boot status.\n");
> + if (status == EFI_NOT_FOUND)
> + goto secure_boot_disabled;
> + return efi_secureboot_mode_unknown;
> +}
Thanks,
Lukas
How about the attached?
Thanks,
David
---
commit 6788837a26d517d10d00138aadd338cc73d69237
Author: David Howells <[email protected]>
Date: Mon Nov 21 23:55:55 2016 +0000
efi: Get the secure boot status
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 <[email protected]>
Signed-off-by: David Howells <[email protected]>
diff --git a/Documentation/x86/zero-page.txt b/Documentation/x86/zero-page.txt
index 95a4d34af3fd..b8527c6b7646 100644
--- a/Documentation/x86/zero-page.txt
+++ b/Documentation/x86/zero-page.txt
@@ -31,6 +31,8 @@ Offset Proto Name Meaning
1E9/001 ALL eddbuf_entries Number of entries in eddbuf (below)
1EA/001 ALL edd_mbr_sig_buf_entries Number of entries in edd_mbr_sig_buffer
(below)
+1EB/001 ALL kbd_status Numlock is enabled
+1EC/001 ALL secure_boot Secure boot is enabled in the firmware
1EF/001 ALL sentinel Used to detect broken bootloaders
290/040 ALL edd_mbr_sig_buffer EDD MBR signatures
2D0/A00 ALL e820_map E820 memory map table
diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
index c8c32ebcdfdb..5b151c262ac2 100644
--- a/arch/x86/boot/compressed/eboot.c
+++ b/arch/x86/boot/compressed/eboot.c
@@ -1158,6 +1158,8 @@ struct boot_params *efi_main(struct efi_config *c,
else
setup_boot_services32(efi_early);
+ boot_params->secure_boot = efi_get_secureboot(sys_table);
+
setup_graphics(boot_params);
setup_efi_pci(boot_params);
diff --git a/arch/x86/boot/compressed/head_32.S b/arch/x86/boot/compressed/head_32.S
index d85b9625e836..c635f7e32f5c 100644
--- a/arch/x86/boot/compressed/head_32.S
+++ b/arch/x86/boot/compressed/head_32.S
@@ -61,6 +61,7 @@
__HEAD
ENTRY(startup_32)
+ movb $0, BP_secure_boot(%esi)
#ifdef CONFIG_EFI_STUB
jmp preferred_addr
diff --git a/arch/x86/boot/compressed/head_64.S b/arch/x86/boot/compressed/head_64.S
index beab8322f72a..ccd2c7461b7f 100644
--- a/arch/x86/boot/compressed/head_64.S
+++ b/arch/x86/boot/compressed/head_64.S
@@ -244,6 +244,7 @@ ENTRY(startup_64)
* that maps our entire kernel(text+data+bss+brk), zero page
* and command line.
*/
+ movb $0, BP_secure_boot(%rsi)
#ifdef CONFIG_EFI_STUB
/*
* The entry point for the PE/COFF executable is efi_pe_entry, so
diff --git a/arch/x86/include/asm/bootparam_utils.h b/arch/x86/include/asm/bootparam_utils.h
index 4a8cb8d7cbd5..7e16d53ff6a3 100644
--- a/arch/x86/include/asm/bootparam_utils.h
+++ b/arch/x86/include/asm/bootparam_utils.h
@@ -38,9 +38,10 @@ static void sanitize_boot_params(struct boot_params *boot_params)
memset(&boot_params->ext_ramdisk_image, 0,
(char *)&boot_params->efi_info -
(char *)&boot_params->ext_ramdisk_image);
- memset(&boot_params->kbd_status, 0,
+ boot_params->kbd_status = 0;
+ memset(&boot_params->_pad5, 0,
(char *)&boot_params->hdr -
- (char *)&boot_params->kbd_status);
+ (char *)&boot_params->_pad5);
memset(&boot_params->_pad7[0], 0,
(char *)&boot_params->edd_mbr_sig_buffer[0] -
(char *)&boot_params->_pad7[0]);
diff --git a/arch/x86/include/uapi/asm/bootparam.h b/arch/x86/include/uapi/asm/bootparam.h
index b10bf319ed20..5138dacf8bb8 100644
--- a/arch/x86/include/uapi/asm/bootparam.h
+++ b/arch/x86/include/uapi/asm/bootparam.h
@@ -135,7 +135,8 @@ struct boot_params {
__u8 eddbuf_entries; /* 0x1e9 */
__u8 edd_mbr_sig_buf_entries; /* 0x1ea */
__u8 kbd_status; /* 0x1eb */
- __u8 _pad5[3]; /* 0x1ec */
+ __u8 secure_boot; /* 0x1ec */
+ __u8 _pad5[2]; /* 0x1ed */
/*
* The sentinel is set to a nonzero value (0xff) in header.S.
*
diff --git a/arch/x86/kernel/asm-offsets.c b/arch/x86/kernel/asm-offsets.c
index c62e015b126c..de827d6ac8c2 100644
--- a/arch/x86/kernel/asm-offsets.c
+++ b/arch/x86/kernel/asm-offsets.c
@@ -81,6 +81,7 @@ void common(void) {
BLANK();
OFFSET(BP_scratch, boot_params, scratch);
+ OFFSET(BP_secure_boot, boot_params, secure_boot);
OFFSET(BP_loadflags, boot_params, hdr.loadflags);
OFFSET(BP_hardware_subarch, boot_params, hdr.hardware_subarch);
OFFSET(BP_version, boot_params, hdr.version);
diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile
index 6621b13c370f..9af966863612 100644
--- a/drivers/firmware/efi/libstub/Makefile
+++ b/drivers/firmware/efi/libstub/Makefile
@@ -28,7 +28,7 @@ OBJECT_FILES_NON_STANDARD := y
# Prevents link failures: __sanitizer_cov_trace_pc() is not linked in.
KCOV_INSTRUMENT := n
-lib-y := efi-stub-helper.o gop.o
+lib-y := efi-stub-helper.o gop.o secureboot.o
# include the stub's generic dependencies from lib/ when building for ARM/arm64
arm-deps := fdt_rw.c fdt_ro.c fdt_wip.c fdt.c fdt_empty_tree.c fdt_sw.c sort.c
diff --git a/drivers/firmware/efi/libstub/arm-stub.c b/drivers/firmware/efi/libstub/arm-stub.c
index b4f7d78f9e8b..9984d0442442 100644
--- a/drivers/firmware/efi/libstub/arm-stub.c
+++ b/drivers/firmware/efi/libstub/arm-stub.c
@@ -20,52 +20,6 @@
bool __nokaslr;
-static int efi_get_secureboot(efi_system_table_t *sys_table_arg)
-{
- static efi_char16_t const sb_var_name[] = {
- 'S', 'e', 'c', 'u', 'r', 'e', 'B', 'o', 'o', 't', 0 };
- static efi_char16_t const sm_var_name[] = {
- 'S', 'e', 't', 'u', 'p', 'M', 'o', 'd', 'e', 0 };
-
- efi_guid_t var_guid = EFI_GLOBAL_VARIABLE_GUID;
- efi_get_variable_t *f_getvar = sys_table_arg->runtime->get_variable;
- u8 val;
- unsigned long size = sizeof(val);
- efi_status_t status;
-
- status = f_getvar((efi_char16_t *)sb_var_name, (efi_guid_t *)&var_guid,
- NULL, &size, &val);
-
- if (status != EFI_SUCCESS)
- goto out_efi_err;
-
- if (val == 0)
- return 0;
-
- status = f_getvar((efi_char16_t *)sm_var_name, (efi_guid_t *)&var_guid,
- NULL, &size, &val);
-
- if (status != EFI_SUCCESS)
- goto out_efi_err;
-
- if (val == 1)
- return 0;
-
- return 1;
-
-out_efi_err:
- switch (status) {
- case EFI_NOT_FOUND:
- return 0;
- case EFI_DEVICE_ERROR:
- return -EIO;
- case EFI_SECURITY_VIOLATION:
- return -EACCES;
- default:
- return -EINVAL;
- }
-}
-
efi_status_t efi_open_volume(efi_system_table_t *sys_table_arg,
void *__image, void **__fh)
{
@@ -226,7 +180,7 @@ unsigned long efi_entry(void *handle, efi_system_table_t *sys_table,
efi_guid_t loaded_image_proto = LOADED_IMAGE_PROTOCOL_GUID;
unsigned long reserve_addr = 0;
unsigned long reserve_size = 0;
- int secure_boot = 0;
+ enum efi_secureboot_mode secure_boot;
struct screen_info *si;
/* Check if we were booted by the EFI firmware */
@@ -296,19 +250,14 @@ unsigned long efi_entry(void *handle, efi_system_table_t *sys_table,
pr_efi_err(sys_table, "Failed to parse EFI cmdline options\n");
secure_boot = efi_get_secureboot(sys_table);
- if (secure_boot > 0)
- pr_efi(sys_table, "UEFI Secure Boot is enabled.\n");
-
- if (secure_boot < 0) {
- pr_efi_err(sys_table,
- "could not determine UEFI Secure Boot status.\n");
- }
/*
- * Unauthenticated device tree data is a security hazard, so
- * ignore 'dtb=' unless UEFI Secure Boot is disabled.
+ * Unauthenticated device tree data is a security hazard, so ignore
+ * 'dtb=' unless UEFI Secure Boot is disabled. We assume that secure
+ * boot is enabled if we can't determine its state.
*/
- if (secure_boot != 0 && strstr(cmdline_ptr, "dtb=")) {
+ if (secure_boot != efi_secureboot_mode_disabled &&
+ strstr(cmdline_ptr, "dtb=")) {
pr_efi(sys_table, "Ignoring DTB from command line.\n");
} else {
status = handle_cmdline_files(sys_table, image, cmdline_ptr,
diff --git a/drivers/firmware/efi/libstub/secureboot.c b/drivers/firmware/efi/libstub/secureboot.c
new file mode 100644
index 000000000000..62d6904da800
--- /dev/null
+++ b/drivers/firmware/efi/libstub/secureboot.c
@@ -0,0 +1,63 @@
+/*
+ * Secure boot handling.
+ *
+ * Copyright (C) 2013,2014 Linaro Limited
+ * Roy Franz <[email protected]
+ * Copyright (C) 2013 Red Hat, Inc.
+ * Mark Salter <[email protected]>
+ *
+ * This file is part of the Linux kernel, and is made available under the
+ * terms of the GNU General Public License version 2.
+ *
+ */
+
+#include <linux/efi.h>
+#include <asm/efi.h>
+
+/* BIOS variables */
+static const efi_guid_t efi_variable_guid = EFI_GLOBAL_VARIABLE_GUID;
+static const efi_char16_t const efi_SecureBoot_name[] = {
+ 'S', 'e', 'c', 'u', 'r', 'e', 'B', 'o', 'o', 't', 0
+};
+static const efi_char16_t const efi_SetupMode_name[] = {
+ 'S', 'e', 't', 'u', 'p', 'M', 'o', 'd', 'e', 0
+};
+
+#define get_efi_var(name, vendor, ...) \
+ efi_call_runtime(get_variable, \
+ (efi_char16_t *)(name), (efi_guid_t *)(vendor), \
+ __VA_ARGS__);
+
+/*
+ * Determine whether we're in secure boot mode.
+ */
+enum efi_secureboot_mode efi_get_secureboot(efi_system_table_t *sys_table_arg)
+{
+ u8 secboot, setupmode;
+ unsigned long size;
+ efi_status_t status;
+
+ size = sizeof(secboot);
+ status = get_efi_var(efi_SecureBoot_name, &efi_variable_guid,
+ NULL, &size, &secboot);
+ if (status != EFI_SUCCESS)
+ goto out_efi_err;
+
+ size = sizeof(setupmode);
+ status = get_efi_var(efi_SetupMode_name, &efi_variable_guid,
+ NULL, &size, &setupmode);
+ if (status != EFI_SUCCESS)
+ goto out_efi_err;
+
+ if (secboot == 0 || setupmode == 1)
+ return efi_secureboot_mode_disabled;
+
+ pr_efi(sys_table_arg, "UEFI Secure Boot is enabled.\n");
+ return efi_secureboot_mode_enabled;
+
+out_efi_err:
+ pr_efi_err(sys_table_arg, "Could not determine UEFI Secure Boot status.\n");
+ if (status == EFI_NOT_FOUND)
+ return efi_secureboot_mode_disabled;
+ return efi_secureboot_mode_unknown;
+}
diff --git a/include/linux/efi.h b/include/linux/efi.h
index c7904556d7a8..92e23f03045e 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -1477,6 +1477,14 @@ efi_status_t efi_setup_gop(efi_system_table_t *sys_table_arg,
bool efi_runtime_disabled(void);
extern void efi_call_virt_check_flags(unsigned long flags, const char *call);
+enum efi_secureboot_mode {
+ efi_secureboot_mode_unset,
+ efi_secureboot_mode_unknown,
+ efi_secureboot_mode_disabled,
+ efi_secureboot_mode_enabled,
+};
+enum efi_secureboot_mode efi_get_secureboot(efi_system_table_t *sys_table);
+
/*
* Arch code can implement the following three template macros, avoiding
* reptition for the void/non-void return cases of {__,}efi_call_virt():
On Thu, Dec 08, 2016 at 08:16:21AM +0000, David Howells wrote:
> +/*
> + * 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;
> +}
In the out_efi_err path, the if-statement needs to come before the
pr_efi_err() call. Otherwise it would be a change of behaviour for
ARM to what we have now.
Also, minor nit, I'd expect Matt to ask for a newline between the
if-statement and the following statements, so:
out_efi_err:
if (status == EFI_NOT_FOUND)
return efi_secureboot_mode_disabled;
pr_efi_err(sys_table_arg, "Could not determine UEFI Secure Boot status.\n");
return efi_secureboot_mode_unknown;
The error message doesn't say what the consequence is of the
failure to determine the status, but IIUC this differs between
x86 and ARM, is that correct? (If I remember the discussion
correctly, x86 defaults to disabled, ARM to enabled.)
Thanks,
Lukas
Lukas Wunner <[email protected]> wrote:
> > +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;
> > +}
>
> In the out_efi_err path, the if-statement needs to come before the
> pr_efi_err() call. Otherwise it would be a change of behaviour for
> ARM to what we have now.
As I understand it, if the BIOS is an EFI BIOS, these variables must exist -
in which case I would argue that the pr_efi_err-statement should be before the
if-statement.
David
On Thu, Dec 08, 2016 at 05:31:13PM +0000, David Howells wrote:
> Lukas Wunner <[email protected]> wrote:
> > > +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;
> > > +}
> >
> > In the out_efi_err path, the if-statement needs to come before the
> > pr_efi_err() call. Otherwise it would be a change of behaviour for
> > ARM to what we have now.
>
> As I understand it, if the BIOS is an EFI BIOS, these variables must exist -
> in which case I would argue that the pr_efi_err-statement should be before
> the if-statement.
The existing efi_get_secureboot() in arm-stub.c returns 0 in the
EFI_NOT_FOUND case and the "Could not determine ..." error is only
printed if the return value is < 0. So you're introducing a change
of behaviour.
If you feel the change is justified, fine, I won't argue against it
since I don't have a dog in this fight.
But obviously it's something that a reader of your patch will trip over,
so at least explain it in the commit message. It would also be good to
explain why you're moving the pr_efi_err() calls in the first place.
ISTR it has to do with the different interpretation of an error,
what I wrote in my previous e-mail: x86 defaults to considering secureboot
disabled on error, ARM to enabled. I'm not even sure that's correct,
I'd have to go re-read the whole thread, which again shows that there's
too little documentation in the commit message.
Thanks,
Lukas