Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1528038 > unrolled thread

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

Back to article view | Back to linux.kernel

Contents

  [PATCH 0/6] efi: Pass secure boot mode to kernel [ver #2] David Howells <dhowells@redhat.com> - 2016-11-23 01:30 +0100
    [PATCH 3/6] efi: Add SHIM and image security database GUID  definitions [ver #2] David Howells <dhowells@redhat.com> - 2016-11-23 01:30 +0100
    [PATCH 1/6] x86/efi: Allow invocation of arbitrary runtime services  [ver #2] David Howells <dhowells@redhat.com> - 2016-11-23 01:30 +0100
    [PATCH 2/6] arm/efi: Allow invocation of arbitrary runtime services  [ver #2] David Howells <dhowells@redhat.com> - 2016-11-23 01:40 +0100
      Re: [PATCH 2/6] arm/efi: Allow invocation of arbitrary runtime services [ver #2] David Howells <dhowells@redhat.com> - 2016-11-23 10:40 +0100
        Re: [PATCH 2/6] arm/efi: Allow invocation of arbitrary runtime  services [ver #2] Ard Biesheuvel <ard.biesheuvel@linaro.org> - 2016-11-23 11:40 +0100
          Re: [PATCH 2/6] arm/efi: Allow invocation of arbitrary runtime services [ver #2] David Howells <dhowells@redhat.com> - 2016-11-23 13:00 +0100
      Re: [PATCH 2/6] arm/efi: Allow invocation of arbitrary runtime  services [ver #2] Mark Rutland <mark.rutland@arm.com> - 2016-11-23 11:40 +0100
        Re: [PATCH 2/6] arm/efi: Allow invocation of arbitrary runtime services [ver #2] David Howells <dhowells@redhat.com> - 2016-11-23 12:50 +0100
          Re: [PATCH 2/6] arm/efi: Allow invocation of arbitrary runtime  services [ver #2] Mark Rutland <mark.rutland@arm.com> - 2016-11-23 14:40 +0100
    [PATCH 5/6] efi: Disable secure boot if shim is in insecure mode  [ver #2] David Howells <dhowells@redhat.com> - 2016-11-23 01:40 +0100
      Re: [PATCH 5/6] efi: Disable secure boot if shim is in insecure mode  [ver #2] Mark Rutland <mark.rutland@arm.com> - 2016-11-23 14:40 +0100
    [PATCH 4/6] efi: Get the secure boot status [ver #2] David Howells <dhowells@redhat.com> - 2016-11-23 01:40 +0100
      Re: [PATCH 4/6] efi: Get the secure boot status [ver #2] Lukas Wunner <lukas@wunner.de> - 2016-11-23 10:40 +0100
        Re: [PATCH 4/6] efi: Get the secure boot status [ver #2] David Howells <dhowells@redhat.com> - 2016-11-23 11:00 +0100
          Re: [PATCH 4/6] efi: Get the secure boot status [ver #2] Lukas Wunner <lukas@wunner.de> - 2016-11-23 11:20 +0100
      Re: [PATCH 4/6] efi: Get the secure boot status [ver #2] Mark Rutland <mark.rutland@arm.com> - 2016-11-23 11:50 +0100
        Re: [PATCH 4/6] efi: Get the secure boot status [ver #2] David Howells <dhowells@redhat.com> - 2016-11-23 12:30 +0100
          Re: [PATCH 4/6] efi: Get the secure boot status [ver #2] Mark Rutland <mark.rutland@arm.com> - 2016-11-23 14:50 +0100
            Re: [PATCH 4/6] efi: Get the secure boot status [ver #2] David Howells <dhowells@redhat.com> - 2016-11-23 15:20 +0100
              Re: [PATCH 4/6] efi: Get the secure boot status [ver #2] David Howells <dhowells@redhat.com> - 2016-11-23 16:00 +0100
              Re: [PATCH 4/6] efi: Get the secure boot status [ver #2] Mark Rutland <mark.rutland@arm.com> - 2016-11-23 16:10 +0100

Page 1 of 2  [1] 2  Next page →

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

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

The patches can be found here also:

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

at tag:

	efi-secure-boot-20161123

Note that the patches are not terminal on the branch.

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

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


 Documentation/x86/zero-page.txt           |    2 +
 arch/arm/include/asm/efi.h                |    1 
 arch/arm64/include/asm/efi.h              |    1 
 arch/x86/boot/compressed/eboot.c          |    3 +
 arch/x86/boot/compressed/head_32.S        |    6 +-
 arch/x86/boot/compressed/head_64.S        |    8 +-
 arch/x86/include/asm/efi.h                |    5 ++
 arch/x86/include/uapi/asm/bootparam.h     |    3 +
 arch/x86/kernel/setup.c                   |    7 ++
 drivers/firmware/efi/libstub/Makefile     |    2 -
 drivers/firmware/efi/libstub/arm-stub.c   |   46 --------------
 drivers/firmware/efi/libstub/secureboot.c |   93 +++++++++++++++++++++++++++++
 include/linux/efi.h                       |    6 ++
 13 files changed, 128 insertions(+), 55 deletions(-)
 create mode 100644 drivers/firmware/efi/libstub/secureboot.c

[toc] | [next] | [standalone]

#1528039 — [PATCH 3/6] efi: Add SHIM and image security database GUID definitions [ver #2]

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 a07a476178cd..24db4e5ec817 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

[toc] | [prev] | [next] | [standalone]

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

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

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

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

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

[toc] | [prev] | [next] | [standalone]

#1528041 — [PATCH 2/6] arm/efi: Allow invocation of arbitrary runtime services [ver #2]

Provide the ability to perform mixed-mode runtime service calls for arm 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/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, ...)			\

[toc] | [prev] | [next] | [standalone]

#1528249 — Re: [PATCH 2/6] arm/efi: Allow invocation of arbitrary runtime services [ver #2]

David Howells <dhowells@redhat.com> wrote:

> +#define efi_call_runtime(f, ...)	sys_table_arg->runtime->f(__VA_ARGS__)

Turns out it's not that simple - of course.  runtime->get_variable is just a
void pointer.  The old arm stub was casting it by virtue of assignment to a
function pointer variable.

The x86_64 appears to be doing bypassing all the compile-time type checking by
passing the arguments through an ellipsis and then fixing up the argument list
in the ->call() function.

What I've changed the ARM and ARM64 things to is:

	#define efi_call_runtime(f, ...)	((efi_##f##_t *)sys_table_arg->runtime->f)(__VA_ARGS__)

David

[toc] | [prev] | [next] | [standalone]

#1528296 — Re: [PATCH 2/6] arm/efi: Allow invocation of arbitrary runtime services [ver #2]

On 23 November 2016 at 09:34, David Howells <dhowells@redhat.com> wrote:
> David Howells <dhowells@redhat.com> wrote:
>
>> +#define efi_call_runtime(f, ...)     sys_table_arg->runtime->f(__VA_ARGS__)
>
> Turns out it's not that simple - of course.  runtime->get_variable is just a
> void pointer.  The old arm stub was casting it by virtue of assignment to a
> function pointer variable.
>
> The x86_64 appears to be doing bypassing all the compile-time type checking by
> passing the arguments through an ellipsis and then fixing up the argument list
> in the ->call() function.
>
> What I've changed the ARM and ARM64 things to is:
>
>         #define efi_call_runtime(f, ...)        ((efi_##f##_t *)sys_table_arg->runtime->f)(__VA_ARGS__)
>

Could we please instead fix the definition of efi_runtime_services_t,
given that we have typedefs already for all its members?

[toc] | [prev] | [next] | [standalone]

#1528352 — Re: [PATCH 2/6] arm/efi: Allow invocation of arbitrary runtime services [ver #2]

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

> > What I've changed the ARM and ARM64 things to is:
> >
> >         #define efi_call_runtime(f, ...)        ((efi_##f##_t *)sys_table_arg->runtime->f)(__VA_ARGS__)
> >
> 
> Could we please instead fix the definition of efi_runtime_services_t,
> given that we have typedefs already for all its members?

Okay, I've pulled in your patch and removed the cast.

I would like to provide wrapper static inlines for things like
efi_get_variable() to get the parameter checking, but the sys_table_arg
behind-the-scenes parameter is tricky to deal with in that case.

David

[toc] | [prev] | [next] | [standalone]

#1528299 — Re: [PATCH 2/6] arm/efi: Allow invocation of arbitrary runtime services [ver #2]

Hi,

Any reason to not Cc LAKML?

On Wed, Nov 23, 2016 at 12:22:43AM +0000, David Howells wrote:
> Provide the ability to perform mixed-mode runtime service calls for arm in
> the same way that commit 0a637ee61247bd4bed9b2a07568ef7a1cfc76187
> ("x86/efi: Allow invocation of arbitrary boot services") provides the
> ability to invoke arbitrary boot services.

I'm not sure I understand. On arm/arm64, "mixed-mode" simply isn't possible.

I see we already call runtime services directly in efi_get_secureboot()
in drivers/firmware/efi/libstub/arm-stub.c.

If this is just to provide a consistent API for the stub, please note
that.

> Suggested-by: Lukas Wunner <lukas@wunner.de>
> Signed-off-by: David Howells <dhowells@redhat.com>
> ---
> 
>  arch/arm/include/asm/efi.h   |    1 +
>  arch/arm64/include/asm/efi.h |    1 +
>  2 files changed, 2 insertions(+)
> 
> diff --git a/arch/arm/include/asm/efi.h b/arch/arm/include/asm/efi.h
> index 0b06f5341b45..e4e6a9d6a825 100644
> --- a/arch/arm/include/asm/efi.h
> +++ b/arch/arm/include/asm/efi.h
> @@ -55,6 +55,7 @@ void efi_virtmap_unload(void);
>  
>  #define efi_call_early(f, ...)		sys_table_arg->boottime->f(__VA_ARGS__)
>  #define __efi_call_early(f, ...)	f(__VA_ARGS__)
> +#define efi_call_runtime(f, ...)	sys_table_arg->runtime->f(__VA_ARGS__)
>  #define efi_is_64bit()			(false)
>  
>  #define efi_call_proto(protocol, f, instance, ...)			\
> diff --git a/arch/arm64/include/asm/efi.h b/arch/arm64/include/asm/efi.h
> index 771b3f0bc757..d74ae223d89f 100644
> --- a/arch/arm64/include/asm/efi.h
> +++ b/arch/arm64/include/asm/efi.h
> @@ -49,6 +49,7 @@ int efi_set_mapping_permissions(struct mm_struct *mm, efi_memory_desc_t *md);
>  
>  #define efi_call_early(f, ...)		sys_table_arg->boottime->f(__VA_ARGS__)
>  #define __efi_call_early(f, ...)	f(__VA_ARGS__)
> +#define efi_call_runtime(f, ...)	sys_table_arg->runtime->f(__VA_ARGS__)

Given this can only work in the stub, the name is somewhat unfortunate,
as it sounds like it's expected to be used at runtime (i.e. in the
kernel). But I guess that's not a big problem.

Other than the casting issue you noted, I think this should work,
though.

Thanks,
Mark.

[toc] | [prev] | [next] | [standalone]

#1528344 — Re: [PATCH 2/6] arm/efi: Allow invocation of arbitrary runtime services [ver #2]

Mark Rutland <mark.rutland@arm.com> wrote:

> Any reason to not Cc LAKML?

Probably not.

> On Wed, Nov 23, 2016 at 12:22:43AM +0000, David Howells wrote:
> > Provide the ability to perform mixed-mode runtime service calls for arm in
> > the same way that commit 0a637ee61247bd4bed9b2a07568ef7a1cfc76187
> > ("x86/efi: Allow invocation of arbitrary boot services") provides the
> > ability to invoke arbitrary boot services.
> 
> I'm not sure I understand. On arm/arm64, "mixed-mode" simply isn't possible.
> 
> I see we already call runtime services directly in efi_get_secureboot()
> in drivers/firmware/efi/libstub/arm-stub.c.
> 
> If this is just to provide a consistent API for the stub, please note
> that.

How about:

    arm/efi: Allow invocation of arbitrary runtime services
    
    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.

David

[toc] | [prev] | [next] | [standalone]

#1528405 — Re: [PATCH 2/6] arm/efi: Allow invocation of arbitrary runtime services [ver #2]

On Wed, Nov 23, 2016 at 11:46:38AM +0000, David Howells wrote:
> Mark Rutland <mark.rutland@arm.com> wrote:
> > On Wed, Nov 23, 2016 at 12:22:43AM +0000, David Howells wrote:
> > > Provide the ability to perform mixed-mode runtime service calls for arm in
> > > the same way that commit 0a637ee61247bd4bed9b2a07568ef7a1cfc76187
> > > ("x86/efi: Allow invocation of arbitrary boot services") provides the
> > > ability to invoke arbitrary boot services.
> > 
> > I'm not sure I understand. On arm/arm64, "mixed-mode" simply isn't possible.
> > 
> > I see we already call runtime services directly in efi_get_secureboot()
> > in drivers/firmware/efi/libstub/arm-stub.c.
> > 
> > If this is just to provide a consistent API for the stub, please note
> > that.
> 
> How about:
> 
>     arm/efi: Allow invocation of arbitrary runtime services
>     
>     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.

That sounds good to me.

Thanks,
Mark.

[toc] | [prev] | [next] | [standalone]

#1528042 — [PATCH 5/6] efi: Disable secure boot if shim is in insecure mode [ver #2]

From: Josh Boyer <jwboyer@fedoraproject.org>

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

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

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

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

[toc] | [prev] | [next] | [standalone]

#1528411 — Re: [PATCH 5/6] efi: Disable secure boot if shim is in insecure mode [ver #2]

On Wed, Nov 23, 2016 at 12:23:04AM +0000, David Howells wrote:
> +	/* See if a user has put shim into insecure mode.  If so, and if the
> +	 * variable doesn't have the runtime attribute set, we might as well
> +	 * honor that.
> +	 */
> +	size = sizeof(moksbstate);
> +	status = get_efi_var(shim_MokSBState_name, &shim_guid,
> +			     &attr, &size, &moksbstate);
> +
> +	/* If it fails, we don't care why.  Default to secure */
> +	if (status != EFI_SUCCESS)
> +		return 1;

I was going to suggest branching to out_efi_err, but I see that wouldn't
work when EFI_NOT_FOUND was returned. It might be worth noting
explicitly that we can't use that, so as to avoid 'obvious' cleanup in
future.

Thanks,
Mark.

[toc] | [prev] | [next] | [standalone]

#1528043 — [PATCH 4/6] efi: Get the secure boot status [ver #2]

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

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

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

 Documentation/x86/zero-page.txt           |    2 +
 arch/x86/boot/compressed/eboot.c          |    2 +
 arch/x86/include/uapi/asm/bootparam.h     |    3 +
 drivers/firmware/efi/libstub/Makefile     |    2 -
 drivers/firmware/efi/libstub/arm-stub.c   |   46 -------------------
 drivers/firmware/efi/libstub/secureboot.c |   71 +++++++++++++++++++++++++++++
 include/linux/efi.h                       |    2 +
 7 files changed, 80 insertions(+), 48 deletions(-)
 create mode 100644 drivers/firmware/efi/libstub/secureboot.c

diff --git a/Documentation/x86/zero-page.txt b/Documentation/x86/zero-page.txt
index 95a4d34af3fd..b8527c6b7646 100644
--- a/Documentation/x86/zero-page.txt
+++ b/Documentation/x86/zero-page.txt
@@ -31,6 +31,8 @@ Offset	Proto	Name		Meaning
 1E9/001	ALL	eddbuf_entries	Number of entries in eddbuf (below)
 1EA/001	ALL	edd_mbr_sig_buf_entries	Number of entries in edd_mbr_sig_buffer
 				(below)
+1EB/001	ALL     kbd_status      Numlock is enabled
+1EC/001	ALL     secure_boot	Secure boot is enabled in the firmware
 1EF/001	ALL	sentinel	Used to detect broken bootloaders
 290/040	ALL	edd_mbr_sig_buffer EDD MBR signatures
 2D0/A00	ALL	e820_map	E820 memory map table
diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
index c8c32ebcdfdb..6023b0e6f2af 100644
--- a/arch/x86/boot/compressed/eboot.c
+++ b/arch/x86/boot/compressed/eboot.c
@@ -1158,6 +1158,8 @@ struct boot_params *efi_main(struct efi_config *c,
 	else
 		setup_boot_services32(efi_early);
 
+	boot_params->secure_boot = (efi_get_secureboot(sys_table) == 1);
+
 	setup_graphics(boot_params);
 
 	setup_efi_pci(boot_params);
diff --git a/arch/x86/include/uapi/asm/bootparam.h b/arch/x86/include/uapi/asm/bootparam.h
index b10bf319ed20..5138dacf8bb8 100644
--- a/arch/x86/include/uapi/asm/bootparam.h
+++ b/arch/x86/include/uapi/asm/bootparam.h
@@ -135,7 +135,8 @@ struct boot_params {
 	__u8  eddbuf_entries;				/* 0x1e9 */
 	__u8  edd_mbr_sig_buf_entries;			/* 0x1ea */
 	__u8  kbd_status;				/* 0x1eb */
-	__u8  _pad5[3];					/* 0x1ec */
+	__u8  secure_boot;				/* 0x1ec */
+	__u8  _pad5[2];					/* 0x1ed */
 	/*
 	 * The sentinel is set to a nonzero value (0xff) in header.S.
 	 *
diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile
index 6621b13c370f..9af966863612 100644
--- a/drivers/firmware/efi/libstub/Makefile
+++ b/drivers/firmware/efi/libstub/Makefile
@@ -28,7 +28,7 @@ OBJECT_FILES_NON_STANDARD	:= y
 # Prevents link failures: __sanitizer_cov_trace_pc() is not linked in.
 KCOV_INSTRUMENT			:= n
 
-lib-y				:= efi-stub-helper.o gop.o
+lib-y				:= efi-stub-helper.o gop.o secureboot.o
 
 # include the stub's generic dependencies from lib/ when building for ARM/arm64
 arm-deps := fdt_rw.c fdt_ro.c fdt_wip.c fdt.c fdt_empty_tree.c fdt_sw.c sort.c
diff --git a/drivers/firmware/efi/libstub/arm-stub.c b/drivers/firmware/efi/libstub/arm-stub.c
index b4f7d78f9e8b..552ee61ddbed 100644
--- a/drivers/firmware/efi/libstub/arm-stub.c
+++ b/drivers/firmware/efi/libstub/arm-stub.c
@@ -20,52 +20,6 @@
 
 bool __nokaslr;
 
-static int efi_get_secureboot(efi_system_table_t *sys_table_arg)
-{
-	static efi_char16_t const sb_var_name[] = {
-		'S', 'e', 'c', 'u', 'r', 'e', 'B', 'o', 'o', 't', 0 };
-	static efi_char16_t const sm_var_name[] = {
-		'S', 'e', 't', 'u', 'p', 'M', 'o', 'd', 'e', 0 };
-
-	efi_guid_t var_guid = EFI_GLOBAL_VARIABLE_GUID;
-	efi_get_variable_t *f_getvar = sys_table_arg->runtime->get_variable;
-	u8 val;
-	unsigned long size = sizeof(val);
-	efi_status_t status;
-
-	status = f_getvar((efi_char16_t *)sb_var_name, (efi_guid_t *)&var_guid,
-			  NULL, &size, &val);
-
-	if (status != EFI_SUCCESS)
-		goto out_efi_err;
-
-	if (val == 0)
-		return 0;
-
-	status = f_getvar((efi_char16_t *)sm_var_name, (efi_guid_t *)&var_guid,
-			  NULL, &size, &val);
-
-	if (status != EFI_SUCCESS)
-		goto out_efi_err;
-
-	if (val == 1)
-		return 0;
-
-	return 1;
-
-out_efi_err:
-	switch (status) {
-	case EFI_NOT_FOUND:
-		return 0;
-	case EFI_DEVICE_ERROR:
-		return -EIO;
-	case EFI_SECURITY_VIOLATION:
-		return -EACCES;
-	default:
-		return -EINVAL;
-	}
-}
-
 efi_status_t efi_open_volume(efi_system_table_t *sys_table_arg,
 			     void *__image, void **__fh)
 {
diff --git a/drivers/firmware/efi/libstub/secureboot.c b/drivers/firmware/efi/libstub/secureboot.c
new file mode 100644
index 000000000000..466fe24f5866
--- /dev/null
+++ b/drivers/firmware/efi/libstub/secureboot.c
@@ -0,0 +1,71 @@
+/*
+ * Secure boot handling.
+ *
+ * Copyright (C) 2013,2014 Linaro Limited
+ *     Roy Franz <roy.franz@linaro.org
+ * Copyright (C) 2013 Red Hat, Inc.
+ *     Mark Salter <msalter@redhat.com>
+ *
+ * This file is part of the Linux kernel, and is made available under the
+ * terms of the GNU General Public License version 2.
+ *
+ */
+
+#include <linux/efi.h>
+#include <asm/efi.h>
+
+/* BIOS variables */
+static const efi_guid_t efi_variable_guid = EFI_GLOBAL_VARIABLE_GUID;
+static const efi_char16_t const efi_SecureBoot_name[] = {
+	'S', 'e', 'c', 'u', 'r', 'e', 'B', 'o', 'o', 't', 0
+};
+static const efi_char16_t const efi_SetupMode_name[] = {
+	'S', 'e', 't', 'u', 'p', 'M', 'o', 'd', 'e', 0
+};
+
+#define get_efi_var(name, vendor, ...) \
+	efi_call_runtime(get_variable, \
+			 (efi_char16_t *)(name), (efi_guid_t *)(vendor), \
+			 __VA_ARGS__);
+
+/*
+ * Determine whether we're in secure boot mode.
+ */
+int efi_get_secureboot(efi_system_table_t *sys_table_arg)
+{
+	u8 val;
+	unsigned long size = sizeof(val);
+	efi_status_t status;
+
+	status = get_efi_var(efi_SecureBoot_name, &efi_variable_guid,
+			     NULL, &size, &val);
+
+	if (status != EFI_SUCCESS)
+		goto out_efi_err;
+
+	if (val == 0)
+		return 0;
+
+	status = get_efi_var(efi_SetupMode_name, &efi_variable_guid,
+			     NULL, &size, &val);
+
+	if (status != EFI_SUCCESS)
+		goto out_efi_err;
+
+	if (val == 1)
+		return 0;
+
+	return 1;
+
+out_efi_err:
+	switch (status) {
+	case EFI_NOT_FOUND:
+		return 0;
+	case EFI_DEVICE_ERROR:
+		return -EIO;
+	case EFI_SECURITY_VIOLATION:
+		return -EACCES;
+	default:
+		return -EINVAL;
+	}
+}
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 24db4e5ec817..ff01ad6f2823 100644
--- a/include/linux/efi.h
+++ b/include/linux/efi.h
@@ -1477,6 +1477,8 @@ efi_status_t efi_setup_gop(efi_system_table_t *sys_table_arg,
 bool efi_runtime_disabled(void);
 extern void efi_call_virt_check_flags(unsigned long flags, const char *call);
 
+int efi_get_secureboot(efi_system_table_t *sys_table_arg);
+
 /*
  * Arch code can implement the following three template macros, avoiding
  * reptition for the void/non-void return cases of {__,}efi_call_virt():

[toc] | [prev] | [next] | [standalone]

#1528246 — Re: [PATCH 4/6] efi: Get the secure boot status [ver #2]

On Wed, Nov 23, 2016 at 12:22:57AM +0000, David Howells wrote:
> Get the firmware's secure-boot status in the kernel boot wrapper and stash
> it somewhere that the main kernel image can find.
> 
> The efi_get_secureboot() function is extracted from the arm stub and (a)
> generalised so that it can be called from x86 and (b) made to use
> efi_call_runtime() so that it can be run in mixed-mode.
> 
> Suggested-by: Lukas Wunner <lukas@wunner.de>
> Signed-off-by: David Howells <dhowells@redhat.com>
> ---
> 
>  Documentation/x86/zero-page.txt           |    2 +
>  arch/x86/boot/compressed/eboot.c          |    2 +
>  arch/x86/include/uapi/asm/bootparam.h     |    3 +
>  drivers/firmware/efi/libstub/Makefile     |    2 -
>  drivers/firmware/efi/libstub/arm-stub.c   |   46 -------------------
>  drivers/firmware/efi/libstub/secureboot.c |   71 +++++++++++++++++++++++++++++
>  include/linux/efi.h                       |    2 +
>  7 files changed, 80 insertions(+), 48 deletions(-)
>  create mode 100644 drivers/firmware/efi/libstub/secureboot.c
> 
> diff --git a/Documentation/x86/zero-page.txt b/Documentation/x86/zero-page.txt
> index 95a4d34af3fd..b8527c6b7646 100644
> --- a/Documentation/x86/zero-page.txt
> +++ b/Documentation/x86/zero-page.txt
> @@ -31,6 +31,8 @@ Offset	Proto	Name		Meaning
>  1E9/001	ALL	eddbuf_entries	Number of entries in eddbuf (below)
>  1EA/001	ALL	edd_mbr_sig_buf_entries	Number of entries in edd_mbr_sig_buffer
>  				(below)
> +1EB/001	ALL     kbd_status      Numlock is enabled
> +1EC/001	ALL     secure_boot	Secure boot is enabled in the firmware
>  1EF/001	ALL	sentinel	Used to detect broken bootloaders
>  290/040	ALL	edd_mbr_sig_buffer EDD MBR signatures
>  2D0/A00	ALL	e820_map	E820 memory map table
> diff --git a/arch/x86/boot/compressed/eboot.c b/arch/x86/boot/compressed/eboot.c
> index c8c32ebcdfdb..6023b0e6f2af 100644
> --- a/arch/x86/boot/compressed/eboot.c
> +++ b/arch/x86/boot/compressed/eboot.c
> @@ -1158,6 +1158,8 @@ struct boot_params *efi_main(struct efi_config *c,
>  	else
>  		setup_boot_services32(efi_early);
>  
> +	boot_params->secure_boot = (efi_get_secureboot(sys_table) == 1);
> +

It just occurred to me that the boot_params struct is populated in
make_boot_params(), perhaps it makes sense to move this line there.
Otherwise LGTM.

Thanks,

Lukas

>  	setup_graphics(boot_params);
>  
>  	setup_efi_pci(boot_params);
> diff --git a/arch/x86/include/uapi/asm/bootparam.h b/arch/x86/include/uapi/asm/bootparam.h
> index b10bf319ed20..5138dacf8bb8 100644
> --- a/arch/x86/include/uapi/asm/bootparam.h
> +++ b/arch/x86/include/uapi/asm/bootparam.h
> @@ -135,7 +135,8 @@ struct boot_params {
>  	__u8  eddbuf_entries;				/* 0x1e9 */
>  	__u8  edd_mbr_sig_buf_entries;			/* 0x1ea */
>  	__u8  kbd_status;				/* 0x1eb */
> -	__u8  _pad5[3];					/* 0x1ec */
> +	__u8  secure_boot;				/* 0x1ec */
> +	__u8  _pad5[2];					/* 0x1ed */
>  	/*
>  	 * The sentinel is set to a nonzero value (0xff) in header.S.
>  	 *
> diff --git a/drivers/firmware/efi/libstub/Makefile b/drivers/firmware/efi/libstub/Makefile
> index 6621b13c370f..9af966863612 100644
> --- a/drivers/firmware/efi/libstub/Makefile
> +++ b/drivers/firmware/efi/libstub/Makefile
> @@ -28,7 +28,7 @@ OBJECT_FILES_NON_STANDARD	:= y
>  # Prevents link failures: __sanitizer_cov_trace_pc() is not linked in.
>  KCOV_INSTRUMENT			:= n
>  
> -lib-y				:= efi-stub-helper.o gop.o
> +lib-y				:= efi-stub-helper.o gop.o secureboot.o
>  
>  # include the stub's generic dependencies from lib/ when building for ARM/arm64
>  arm-deps := fdt_rw.c fdt_ro.c fdt_wip.c fdt.c fdt_empty_tree.c fdt_sw.c sort.c
> diff --git a/drivers/firmware/efi/libstub/arm-stub.c b/drivers/firmware/efi/libstub/arm-stub.c
> index b4f7d78f9e8b..552ee61ddbed 100644
> --- a/drivers/firmware/efi/libstub/arm-stub.c
> +++ b/drivers/firmware/efi/libstub/arm-stub.c
> @@ -20,52 +20,6 @@
>  
>  bool __nokaslr;
>  
> -static int efi_get_secureboot(efi_system_table_t *sys_table_arg)
> -{
> -	static efi_char16_t const sb_var_name[] = {
> -		'S', 'e', 'c', 'u', 'r', 'e', 'B', 'o', 'o', 't', 0 };
> -	static efi_char16_t const sm_var_name[] = {
> -		'S', 'e', 't', 'u', 'p', 'M', 'o', 'd', 'e', 0 };
> -
> -	efi_guid_t var_guid = EFI_GLOBAL_VARIABLE_GUID;
> -	efi_get_variable_t *f_getvar = sys_table_arg->runtime->get_variable;
> -	u8 val;
> -	unsigned long size = sizeof(val);
> -	efi_status_t status;
> -
> -	status = f_getvar((efi_char16_t *)sb_var_name, (efi_guid_t *)&var_guid,
> -			  NULL, &size, &val);
> -
> -	if (status != EFI_SUCCESS)
> -		goto out_efi_err;
> -
> -	if (val == 0)
> -		return 0;
> -
> -	status = f_getvar((efi_char16_t *)sm_var_name, (efi_guid_t *)&var_guid,
> -			  NULL, &size, &val);
> -
> -	if (status != EFI_SUCCESS)
> -		goto out_efi_err;
> -
> -	if (val == 1)
> -		return 0;
> -
> -	return 1;
> -
> -out_efi_err:
> -	switch (status) {
> -	case EFI_NOT_FOUND:
> -		return 0;
> -	case EFI_DEVICE_ERROR:
> -		return -EIO;
> -	case EFI_SECURITY_VIOLATION:
> -		return -EACCES;
> -	default:
> -		return -EINVAL;
> -	}
> -}
> -
>  efi_status_t efi_open_volume(efi_system_table_t *sys_table_arg,
>  			     void *__image, void **__fh)
>  {
> diff --git a/drivers/firmware/efi/libstub/secureboot.c b/drivers/firmware/efi/libstub/secureboot.c
> new file mode 100644
> index 000000000000..466fe24f5866
> --- /dev/null
> +++ b/drivers/firmware/efi/libstub/secureboot.c
> @@ -0,0 +1,71 @@
> +/*
> + * Secure boot handling.
> + *
> + * Copyright (C) 2013,2014 Linaro Limited
> + *     Roy Franz <roy.franz@linaro.org
> + * Copyright (C) 2013 Red Hat, Inc.
> + *     Mark Salter <msalter@redhat.com>
> + *
> + * This file is part of the Linux kernel, and is made available under the
> + * terms of the GNU General Public License version 2.
> + *
> + */
> +
> +#include <linux/efi.h>
> +#include <asm/efi.h>
> +
> +/* BIOS variables */
> +static const efi_guid_t efi_variable_guid = EFI_GLOBAL_VARIABLE_GUID;
> +static const efi_char16_t const efi_SecureBoot_name[] = {
> +	'S', 'e', 'c', 'u', 'r', 'e', 'B', 'o', 'o', 't', 0
> +};
> +static const efi_char16_t const efi_SetupMode_name[] = {
> +	'S', 'e', 't', 'u', 'p', 'M', 'o', 'd', 'e', 0
> +};
> +
> +#define get_efi_var(name, vendor, ...) \
> +	efi_call_runtime(get_variable, \
> +			 (efi_char16_t *)(name), (efi_guid_t *)(vendor), \
> +			 __VA_ARGS__);
> +
> +/*
> + * Determine whether we're in secure boot mode.
> + */
> +int efi_get_secureboot(efi_system_table_t *sys_table_arg)
> +{
> +	u8 val;
> +	unsigned long size = sizeof(val);
> +	efi_status_t status;
> +
> +	status = get_efi_var(efi_SecureBoot_name, &efi_variable_guid,
> +			     NULL, &size, &val);
> +
> +	if (status != EFI_SUCCESS)
> +		goto out_efi_err;
> +
> +	if (val == 0)
> +		return 0;
> +
> +	status = get_efi_var(efi_SetupMode_name, &efi_variable_guid,
> +			     NULL, &size, &val);
> +
> +	if (status != EFI_SUCCESS)
> +		goto out_efi_err;
> +
> +	if (val == 1)
> +		return 0;
> +
> +	return 1;
> +
> +out_efi_err:
> +	switch (status) {
> +	case EFI_NOT_FOUND:
> +		return 0;
> +	case EFI_DEVICE_ERROR:
> +		return -EIO;
> +	case EFI_SECURITY_VIOLATION:
> +		return -EACCES;
> +	default:
> +		return -EINVAL;
> +	}
> +}
> diff --git a/include/linux/efi.h b/include/linux/efi.h
> index 24db4e5ec817..ff01ad6f2823 100644
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -1477,6 +1477,8 @@ efi_status_t efi_setup_gop(efi_system_table_t *sys_table_arg,
>  bool efi_runtime_disabled(void);
>  extern void efi_call_virt_check_flags(unsigned long flags, const char *call);
>  
> +int efi_get_secureboot(efi_system_table_t *sys_table_arg);
> +
>  /*
>   * Arch code can implement the following three template macros, avoiding
>   * reptition for the void/non-void return cases of {__,}efi_call_virt():
> 

[toc] | [prev] | [next] | [standalone]

#1528261 — Re: [PATCH 4/6] efi: Get the secure boot status [ver #2]

Lukas Wunner <lukas@wunner.de> wrote:

> It just occurred to me that the boot_params struct is populated in
> make_boot_params(), perhaps it makes sense to move this line there.
> Otherwise LGTM.

Ummm...  Looking at arch/x86/boot/compressed/head_64.S, make_boot_params() is
only called if the stub is entered through efi_pe_entry, not if entered
through efi64_stub_entry, whereas efi_main() is called in both cases.  I think
entry through efi32_stub_entry may also skip make_boot_params().

The comment on make_boot_params() suggests that this function is only used if
whoever booted the kernel didn't supply it.

David

[toc] | [prev] | [next] | [standalone]

#1528284 — Re: [PATCH 4/6] efi: Get the secure boot status [ver #2]

On Wed, Nov 23, 2016 at 09:53:00AM +0000, David Howells wrote:
> Lukas Wunner <lukas@wunner.de> wrote:
> > It just occurred to me that the boot_params struct is populated in
> > make_boot_params(), perhaps it makes sense to move this line there.
> > Otherwise LGTM.
> 
> Ummm...  Looking at arch/x86/boot/compressed/head_64.S, make_boot_params() is
> only called if the stub is entered through efi_pe_entry, not if entered
> through efi64_stub_entry, whereas efi_main() is called in both cases.  I think
> entry through efi32_stub_entry may also skip make_boot_params().
> 
> The comment on make_boot_params() suggests that this function is only used if
> whoever booted the kernel didn't supply it.

Good point. :-) I didn't dig this deep, it was just something that
crossed my mind when skimming over the patch.

You're also right about the (efi_##f##_t *) addition in patch 2.

Thanks,

Lukas

[toc] | [prev] | [next] | [standalone]

#1528304 — Re: [PATCH 4/6] efi: Get the secure boot status [ver #2]

On Wed, Nov 23, 2016 at 12:22:57AM +0000, David Howells wrote:
> @@ -1158,6 +1158,8 @@ struct boot_params *efi_main(struct efi_config *c,
>  	else
>  		setup_boot_services32(efi_early);
>  
> +	boot_params->secure_boot = (efi_get_secureboot(sys_table) == 1);

In the arm stub's efi_entry(), we fail-safe, and assume secure boot for any
non-zero status (including errors). e.g. 

	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=")) {
		pr_efi(sys_table, "Ignoring DTB from command line.\n");

... should we not do likewise here, e.g.

	int 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");

	/*
	 * Fail-safe in the case of an error determining the secure boot
	 * status.
	 */
	boot_params->secure_boot = (secure_boot != 0);

... ?

Thanks,
Mark.

[toc] | [prev] | [next] | [standalone]

#1528330 — Re: [PATCH 4/6] efi: Get the secure boot status [ver #2]

Mark Rutland <mark.rutland@arm.com> wrote:

> 	int 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");

In which case, should this be moved into efi_get_secureboot() and it return a
bool?

David

[toc] | [prev] | [next] | [standalone]

#1528429 — Re: [PATCH 4/6] efi: Get the secure boot status [ver #2]

On Wed, Nov 23, 2016 at 11:25:57AM +0000, David Howells wrote:
> Mark Rutland <mark.rutland@arm.com> wrote:
> 
> > 	int 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");
> 
> In which case, should this be moved into efi_get_secureboot() and it return a
> bool?

That would make sense to me, provided we're only likely to call that
once (and only log once).

I guess it would also make sense to change the latter case to soemthing
like:
	
	Could not determine UEFI Secure Boot status. Assuming enabled.

... so as to make it clear what the effect is.

Thanks,
Mark.

[toc] | [prev] | [next] | [standalone]

#1528434 — Re: [PATCH 4/6] efi: Get the secure boot status [ver #2]

Mark Rutland <mark.rutland@arm.com> wrote:

> > > 	if (secure_boot < 0)
> > > 		pr_efi_err(sys_table,
> > > 			"could not determine UEFI Secure Boot status.\n");
> > 
> > In which case, should this be moved into efi_get_secureboot() and it return a
> > bool?
> 
> That would make sense to me, provided we're only likely to call that
> once (and only log once).
> 
> I guess it would also make sense to change the latter case to soemthing
> like:
> 	
> 	Could not determine UEFI Secure Boot status. Assuming enabled.
> 
> ... so as to make it clear what the effect is.

Actually, the two arches have a different interpretation on how to deal with
an error.  Matthew Garrett's original x86 patch assumes that if we get an
error when trying to read SecureBoot and SetupMode that we're *not* in secure
mode, but ARM assumes the opposite.

David

[toc] | [prev] | [next] | [standalone]

Page 1 of 2  [1] 2  Next page →

Back to top | Article view | linux.kernel

csiph-web