Groups > linux.kernel > #1523861 > unrolled thread

[PATCH 02/16] efi: Get the secure boot status

Started by	David Howells <dhowells@redhat.com>
First post	2016-11-16 22:50 +0100
Last post	2016-11-22 21:40 +0100
Articles	13 on this page of 33 — 4 participants

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

  [PATCH 02/16] efi: Get the secure boot status David Howells <dhowells@redhat.com> - 2016-11-16 22:50 +0100
    Re: [PATCH 02/16] efi: Get the secure boot status Lukas Wunner <lukas@wunner.de> - 2016-11-17 15:40 +0100
      Re: [PATCH 02/16] efi: Get the secure boot status David Howells <dhowells@redhat.com> - 2016-11-21 12:50 +0100
        Re: [PATCH 02/16] efi: Get the secure boot status Lukas Wunner <lukas@wunner.de> - 2016-11-21 21:00 +0100
      Re: [PATCH 02/16] efi: Get the secure boot status David Howells <dhowells@redhat.com> - 2016-11-21 12:50 +0100
        Re: [PATCH 02/16] efi: Get the secure boot status Ard Biesheuvel <ard.biesheuvel@linaro.org> - 2016-11-21 13:00 +0100
          Re: [PATCH 02/16] efi: Get the secure boot status David Howells <dhowells@redhat.com> - 2016-11-21 13:50 +0100
            Re: [PATCH 02/16] efi: Get the secure boot status Ard Biesheuvel <ard.biesheuvel@linaro.org> - 2016-11-21 14:20 +0100
              Re: [PATCH 02/16] efi: Get the secure boot status Lukas Wunner <lukas@wunner.de> - 2016-11-21 16:20 +0100
                Re: [PATCH 02/16] efi: Get the secure boot status Ard Biesheuvel <ard.biesheuvel@linaro.org> - 2016-11-21 16:30 +0100
      [PATCH 6/6] efi: Add EFI_SECURE_BOOT bit David Howells <dhowells@redhat.com> - 2016-11-22 01:40 +0100
        Re: [PATCH 6/6] efi: Add EFI_SECURE_BOOT bit Lukas Wunner <lukas@wunner.de> - 2016-11-22 14:10 +0100
      [PATCH 5/6] efi: Disable secure boot if shim is in insecure mode David Howells <dhowells@redhat.com> - 2016-11-22 01:40 +0100
        Re: [PATCH 5/6] efi: Disable secure boot if shim is in insecure mode Lukas Wunner <lukas@wunner.de> - 2016-11-22 14:10 +0100
      [PATCH 2/6] arm/efi: Allow invocation of arbitrary runtime services David Howells <dhowells@redhat.com> - 2016-11-22 01:40 +0100
      [PATCH 1/6] x86/efi: Allow invocation of arbitrary runtime services David Howells <dhowells@redhat.com> - 2016-11-22 01:40 +0100
        Re: [PATCH 1/6] x86/efi: Allow invocation of arbitrary runtime  services Lukas Wunner <lukas@wunner.de> - 2016-11-22 11:20 +0100
          Re: [PATCH 1/6] x86/efi: Allow invocation of arbitrary runtime services David Howells <dhowells@redhat.com> - 2016-11-22 15:20 +0100
            Re: [PATCH 1/6] x86/efi: Allow invocation of arbitrary runtime  services Joe Perches <joe@perches.com> - 2016-11-22 16:00 +0100
              Re: [PATCH 1/6] x86/efi: Allow invocation of arbitrary runtime services David Howells <dhowells@redhat.com> - 2016-11-22 17:00 +0100
                Re: [PATCH 1/6] x86/efi: Allow invocation of arbitrary runtime  services Joe Perches <joe@perches.com> - 2016-11-22 17:30 +0100
                  Re: [PATCH 1/6] x86/efi: Allow invocation of arbitrary runtime services David Howells <dhowells@redhat.com> - 2016-11-22 17:50 +0100
                    Re: [PATCH 1/6] x86/efi: Allow invocation of arbitrary runtime  services Joe Perches <joe@perches.com> - 2016-11-22 18:00 +0100
      [PATCH 3/6] efi: Add SHIM and image security database GUID  definitions David Howells <dhowells@redhat.com> - 2016-11-22 01:40 +0100
      [PATCH 4/6] efi: Get the secure boot status David Howells <dhowells@redhat.com> - 2016-11-22 01:40 +0100
        Re: [PATCH 4/6] efi: Get the secure boot status Lukas Wunner <lukas@wunner.de> - 2016-11-22 11:50 +0100
          Re: [PATCH 4/6] efi: Get the secure boot status Ard Biesheuvel <ard.biesheuvel@linaro.org> - 2016-11-22 11:50 +0100
          Re: [PATCH 4/6] efi: Get the secure boot status David Howells <dhowells@redhat.com> - 2016-11-22 15:50 +0100
            Re: [PATCH 4/6] efi: Get the secure boot status David Howells <dhowells@redhat.com> - 2016-11-22 16:00 +0100
            Re: [PATCH 4/6] efi: Get the secure boot status Lukas Wunner <lukas@wunner.de> - 2016-11-22 21:30 +0100
              Re: [PATCH 4/6] efi: Get the secure boot status David Howells <dhowells@redhat.com> - 2016-11-23 01:10 +0100
          Re: [PATCH 4/6] efi: Get the secure boot status David Howells <dhowells@redhat.com> - 2016-11-22 16:10 +0100
            Re: [PATCH 4/6] efi: Get the secure boot status Lukas Wunner <lukas@wunner.de> - 2016-11-22 21:40 +0100

Page 2 of 2 — ← Prev page 1 [2]

#1527662 — Re: [PATCH 1/6] x86/efi: Allow invocation of arbitrary runtime services

From	Joe Perches <joe@perches.com>
Date	2016-11-22 17:30 +0100
Subject	Re: [PATCH 1/6] x86/efi: Allow invocation of arbitrary runtime services
Message-ID	<sGm7g-1bI-9@gated-at.bofh.it>
In reply to	#1527629

On Tue, 2016-11-22 at 15:52 +0000, David Howells wrote:
> Joe Perches <joe@perches.com> wrote:
> 
> > > > Small nit, checkpatch usually complains that this should be written as
> > > > 12-character SHA-1 followed by the commit subject, i.e.
> > > > 
> > > > 0a637ee61247 ("x86/efi: Allow invocation of arbitrary boot services")
> > > 
> > > In this case, checkpatch is wrong.
> > 
> > Why do you think so?
> 
> Actually, checkpatch doesn't complain about embedded commit IDs anymore, so in
> that case, it's just about acceptable.

checkpatch still emits warnings about the format of
commmit IDs.

What version of checkpatch are yuu using?

> Apart from that, I think we should put in the full SHA-1 commit.  The
> probability of a collision in a 12-digit hex number for the >5,000,000 commits
> just in Linus's tree is currently at ~4.5% and gradually increasing.  Add in
> all the commits in not-yet-upstreamed trees - which might be another million
> commits, say - then we're over 6%..

Umm, no, that's not correct.
SHA-1 lengths of 12 are unique for quite awhile yet.

https://blog.cuviper.com/2013/11/10/how-short-can-git-abbreviate/

Using Linus' tree today, from commit 3b404a519815
the current output of the git-uniq-abbrev script is:

$ git-uniq-abbrev 
5048673 objects
 4: 5048673 / 65536
 5: 5007413 / 998721
 6: 1312496 / 623343
 7: 94487 / 47089
 8: 6163 / 3081
 9: 416 / 208
10: 28 / 14
11: 4 / 2
12: 0 / 0
d597639e2036f04f0226761e2d818b31f2db7820
d597639e203a100156501df8a0756fd09573e2de
ef91b6e893a00d903400f8e1303efc4d52b710af
ef91b6e893afc4c4ca488453ea9f19ced5fa5861

> Oh, yes, and speaking of checkpatch, can you make it so that if it sees:
> 
> commit 12345...
> Author: foo <foo@bar>
> Date: blah
> 
>    Subject line
> 
>    Description lines
>    ...
>    ...
>    ...
>    ...
> 
>    Signed-off-by-and-suchline-lines
> 
> diff ...
> 
> with the all description indented by 4 spaces, then assume that it's the
> output of git show and not give the warnings about signed-off-by and other
> things being indented?

No.  Use --format=email as appropriate instead.

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

#1527689 — Re: [PATCH 1/6] x86/efi: Allow invocation of arbitrary runtime services

From	David Howells <dhowells@redhat.com>
Date	2016-11-22 17:50 +0100
Subject	Re: [PATCH 1/6] x86/efi: Allow invocation of arbitrary runtime services
Message-ID	<sGmqC-1ia-15@gated-at.bofh.it>
In reply to	#1527662

[Multipart message — attachments visible in raw view] — view raw

Joe Perches <joe@perches.com> wrote:

> Umm, no, that's not correct.
> SHA-1 lengths of 12 are unique for quite awhile yet.
> 
> https://blog.cuviper.com/2013/11/10/how-short-can-git-abbreviate/

The article says:

	1.9% at 12

which is for 3253824 objects (I get 1.86%).

However, that was three years ago, and we now have over five million objects,
so the collision possibility is 4.5% now.

If we add another 2 million over the next three years, then the probability
will be over 8% then.

I've attached my spreadsheet for you to have a look at.

> No.  Use --format=email as appropriate instead.

Fix checkpatch.  This is an entirely reasonable supposition.

David

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

#1527702 — Re: [PATCH 1/6] x86/efi: Allow invocation of arbitrary runtime services

From	Joe Perches <joe@perches.com>
Date	2016-11-22 18:00 +0100
Subject	Re: [PATCH 1/6] x86/efi: Allow invocation of arbitrary runtime services
Message-ID	<sGmAn-1lo-19@gated-at.bofh.it>
In reply to	#1527689

On Tue, 2016-11-22 at 16:40 +0000, David Howells wrote:
> Joe Perches <joe@perches.com> wrote:
> 
> > Umm, no, that's not correct.
> > SHA-1 lengths of 12 are unique for quite awhile yet.
> > 
> > https://blog.cuviper.com/2013/11/10/how-short-can-git-abbreviate/
> 
> The article says:
> 
> 	1.9% at 12
> 
> which is for 3253824 objects (I get 1.86%).
> 
> However, that was three years ago, and we now have over five million objects,
> so the collision possibility is 4.5% now.
> 
> If we add another 2 million over the next three years, then the probability
> will be over 8% then.
> 
> I've attached my spreadsheet for you to have a look at.
> 
> > No.  Use --format=email as appropriate instead.
> 
> Fix checkpatch.  This is an entirely reasonable supposition.

No.  There's nothing to fix there IMO.
Of course you are welcome to submit patches.

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

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

From	David Howells <dhowells@redhat.com>
Date	2016-11-22 01:40 +0100
Subject	[PATCH 3/6] efi: Add SHIM and image security database GUID definitions
Message-ID	<sG7hU-8nV-47@gated-at.bofh.it>
In reply to	#1524457

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]

#1527161 — [PATCH 4/6] efi: Get the secure boot status

From	David Howells <dhowells@redhat.com>
Date	2016-11-22 01:40 +0100
Subject	[PATCH 4/6] efi: Get the secure boot status
Message-ID	<sG7hU-8nV-49@gated-at.bofh.it>
In reply to	#1524457

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

Signed-off-by: Matthew Garrett <matthew.garrett@nebula.com>
Signed-off-by: David Howells <dhowells@redhat.com>
---

 Documentation/x86/zero-page.txt           |    2 +
 arch/x86/boot/compressed/eboot.c          |    5 ++
 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 |   66 +++++++++++++++++++++++++++++
 include/linux/efi.h                       |    2 +
 7 files changed, 78 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..fd6506de480d 100644
--- a/arch/x86/boot/compressed/eboot.c
+++ b/arch/x86/boot/compressed/eboot.c
@@ -12,6 +12,7 @@
 #include <asm/efi.h>
 #include <asm/setup.h>
 #include <asm/desc.h>
+#include <asm/bootparam_utils.h>
 
 #include "../string.h"
 #include "eboot.h"
@@ -1158,6 +1159,10 @@ struct boot_params *efi_main(struct efi_config *c,
 	else
 		setup_boot_services32(efi_early);
 
+	sanitize_boot_params(boot_params);
+
+	boot_params->secure_boot = efi_get_secureboot();
+
 	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..e44d8c9ee150
--- /dev/null
+++ b/drivers/firmware/efi/libstub/secureboot.c
@@ -0,0 +1,66 @@
+/*
+ * 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 <linux/sort.h>
+#include <asm/efi.h>
+
+#include "efistub.h"
+
+int efi_get_secureboot(void)
+{
+	static const efi_char16_t const sb_var_name[] = {
+		'S', 'e', 'c', 'u', 'r', 'e', 'B', 'o', 'o', 't', 0 };
+	static const efi_char16_t const sm_var_name[] = {
+		'S', 'e', 't', 'u', 'p', 'M', 'o', 'd', 'e', 0 };
+
+	static const efi_guid_t var_guid = EFI_GLOBAL_VARIABLE_GUID;
+
+	u8 val;
+	unsigned long size = sizeof(val);
+	efi_status_t status;
+
+#define f_getvar(...) efi_call_runtime(get_variable, __VA_ARGS__)
+
+	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;
+	}
+}
diff --git a/include/linux/efi.h b/include/linux/efi.h
index 24db4e5ec817..615d8704f048 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(void);
+
 /*
  * 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]

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

From	Lukas Wunner <lukas@wunner.de>
Date	2016-11-22 11:50 +0100
Subject	Re: [PATCH 4/6] efi: Get the secure boot status
Message-ID	<sGgOd-61X-21@gated-at.bofh.it>
In reply to	#1527161

On Tue, Nov 22, 2016 at 12:32:01AM +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.

That's a bit terse.  You could write here that you're moving the
existing ARM function to generic stub code to be able to reuse it
on x86.

Further comments below.


> 
> Signed-off-by: Matthew Garrett <matthew.garrett@nebula.com>
> Signed-off-by: David Howells <dhowells@redhat.com>
> ---
> 
>  Documentation/x86/zero-page.txt           |    2 +
>  arch/x86/boot/compressed/eboot.c          |    5 ++
>  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 |   66 +++++++++++++++++++++++++++++
>  include/linux/efi.h                       |    2 +
>  7 files changed, 78 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..fd6506de480d 100644
> --- a/arch/x86/boot/compressed/eboot.c
> +++ b/arch/x86/boot/compressed/eboot.c
> @@ -12,6 +12,7 @@
>  #include <asm/efi.h>
>  #include <asm/setup.h>
>  #include <asm/desc.h>
> +#include <asm/bootparam_utils.h>
>  
>  #include "../string.h"
>  #include "eboot.h"
> @@ -1158,6 +1159,10 @@ struct boot_params *efi_main(struct efi_config *c,
>  	else
>  		setup_boot_services32(efi_early);
>  
> +	sanitize_boot_params(boot_params);

What is the connection of this change to the rest of the patch?
Needs an explanation in the commit message.


> +
> +	boot_params->secure_boot = efi_get_secureboot();
> +
>  	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..e44d8c9ee150
> --- /dev/null
> +++ b/drivers/firmware/efi/libstub/secureboot.c
> @@ -0,0 +1,66 @@
> +/*
> + * 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 <linux/sort.h>

You don't need sort.h.


> +#include <asm/efi.h>
> +
> +#include "efistub.h"

From a cursory look at efistub.h, you don't seem to need this either.


> +
> +int efi_get_secureboot(void)

It looks like you didn't compile-test this on ARM.

You dropped the efi_system_table_t *sys_table_arg argument but this
isn't defined anywhere as a static global.


> +{
> +	static const efi_char16_t const sb_var_name[] = {
> +		'S', 'e', 'c', 'u', 'r', 'e', 'B', 'o', 'o', 't', 0 };
> +	static const efi_char16_t const sm_var_name[] = {
> +		'S', 'e', 't', 'u', 'p', 'M', 'o', 'd', 'e', 0 };
> +
> +	static const efi_guid_t var_guid = EFI_GLOBAL_VARIABLE_GUID;
> +

Gratuitous newline in-between variable declarations.

> +	u8 val;
> +	unsigned long size = sizeof(val);
> +	efi_status_t status;
> +
> +#define f_getvar(...) efi_call_runtime(get_variable, __VA_ARGS__)
> +
> +	status = f_getvar((efi_char16_t *)sb_var_name, (efi_guid_t *)&var_guid,
> +			  NULL, &size, &val);

Just replace the f_getvar yourself instead of having cpp do it:

	status = efi_call_runtime(get_variable, (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);

Same here.


> +
> +	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;
> +	}

The "out_efi_err" portion differs from the previous version of this
patch.  Setting a __u8 to a negative value, is this really what you
want?

Thanks,

Lukas

> +}
> diff --git a/include/linux/efi.h b/include/linux/efi.h
> index 24db4e5ec817..615d8704f048 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(void);
> +
>  /*
>   * 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]

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

From	Ard Biesheuvel <ard.biesheuvel@linaro.org>
Date	2016-11-22 11:50 +0100
Subject	Re: [PATCH 4/6] efi: Get the secure boot status
Message-ID	<sGgOd-61X-27@gated-at.bofh.it>
In reply to	#1527398

On 22 November 2016 at 10:44, Lukas Wunner <lukas@wunner.de> wrote:
> On Tue, Nov 22, 2016 at 12:32:01AM +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.
>
> That's a bit terse.  You could write here that you're moving the
> existing ARM function to generic stub code to be able to reuse it
> on x86.
>
> Further comments below.
>
>
>>
>> Signed-off-by: Matthew Garrett <matthew.garrett@nebula.com>
>> Signed-off-by: David Howells <dhowells@redhat.com>
>> ---
>>
>>  Documentation/x86/zero-page.txt           |    2 +
>>  arch/x86/boot/compressed/eboot.c          |    5 ++
>>  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 |   66 +++++++++++++++++++++++++++++
>>  include/linux/efi.h                       |    2 +
>>  7 files changed, 78 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..fd6506de480d 100644
>> --- a/arch/x86/boot/compressed/eboot.c
>> +++ b/arch/x86/boot/compressed/eboot.c
>> @@ -12,6 +12,7 @@
>>  #include <asm/efi.h>
>>  #include <asm/setup.h>
>>  #include <asm/desc.h>
>> +#include <asm/bootparam_utils.h>
>>
>>  #include "../string.h"
>>  #include "eboot.h"
>> @@ -1158,6 +1159,10 @@ struct boot_params *efi_main(struct efi_config *c,
>>       else
>>               setup_boot_services32(efi_early);
>>
>> +     sanitize_boot_params(boot_params);
>
> What is the connection of this change to the rest of the patch?
> Needs an explanation in the commit message.
>
>
>> +
>> +     boot_params->secure_boot = efi_get_secureboot();
>> +
>>       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..e44d8c9ee150
>> --- /dev/null
>> +++ b/drivers/firmware/efi/libstub/secureboot.c
>> @@ -0,0 +1,66 @@
>> +/*
>> + * 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 <linux/sort.h>
>
> You don't need sort.h.
>
>
>> +#include <asm/efi.h>
>> +
>> +#include "efistub.h"
>
> From a cursory look at efistub.h, you don't seem to need this either.
>
>
>> +
>> +int efi_get_secureboot(void)
>
> It looks like you didn't compile-test this on ARM.
>
> You dropped the efi_system_table_t *sys_table_arg argument but this
> isn't defined anywhere as a static global.
>

That is actually a thing that has been annoying me: the efi_call_xxx
macros on ARM/arm64 rely on sys_table_arg being defined in the current
scope, which hides this dependency from users of the macro, and is
also a pain given that you are forced to use the exact name
'sys_table_arg'. Patches that clean that up are gladly accepted, or I
may take a stab at this myself (but not for a week or two)

>
>> +{
>> +     static const efi_char16_t const sb_var_name[] = {
>> +             'S', 'e', 'c', 'u', 'r', 'e', 'B', 'o', 'o', 't', 0 };
>> +     static const efi_char16_t const sm_var_name[] = {
>> +             'S', 'e', 't', 'u', 'p', 'M', 'o', 'd', 'e', 0 };
>> +
>> +     static const efi_guid_t var_guid = EFI_GLOBAL_VARIABLE_GUID;
>> +
>
> Gratuitous newline in-between variable declarations.
>
>> +     u8 val;
>> +     unsigned long size = sizeof(val);
>> +     efi_status_t status;
>> +
>> +#define f_getvar(...) efi_call_runtime(get_variable, __VA_ARGS__)
>> +
>> +     status = f_getvar((efi_char16_t *)sb_var_name, (efi_guid_t *)&var_guid,
>> +                       NULL, &size, &val);
>
> Just replace the f_getvar yourself instead of having cpp do it:
>
>         status = efi_call_runtime(get_variable, (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);
>
> Same here.
>
>
>> +
>> +     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;
>> +     }
>
> The "out_efi_err" portion differs from the previous version of this
> patch.  Setting a __u8 to a negative value, is this really what you
> want?
>
> Thanks,
>
> Lukas
>
>> +}
>> diff --git a/include/linux/efi.h b/include/linux/efi.h
>> index 24db4e5ec817..615d8704f048 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(void);
>> +
>>  /*
>>   * Arch code can implement the following three template macros, avoiding
>>   * reptition for the void/non-void return cases of {__,}efi_call_virt():
>>
> --
> To unsubscribe from this list: send the line "unsubscribe linux-efi" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

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

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

From	David Howells <dhowells@redhat.com>
Date	2016-11-22 15:50 +0100
Subject	Re: [PATCH 4/6] efi: Get the secure boot status
Message-ID	<sGkyt-8rC-13@gated-at.bofh.it>
In reply to	#1527398

Lukas Wunner <lukas@wunner.de> wrote:

> > +int efi_get_secureboot(void)
> 
> It looks like you didn't compile-test this on ARM.

Yes.  What arm config would you suggest?

> > +#define f_getvar(...) efi_call_runtime(get_variable, __VA_ARGS__)
> > +
> > +	status = f_getvar((efi_char16_t *)sb_var_name, (efi_guid_t *)&var_guid,
> > +			  NULL, &size, &val);
>
> Just replace the f_getvar yourself instead of having cpp do it:
>
> 	status = efi_call_runtime(get_variable, (efi_char16_t *)sb_var_name,
> 				  (efi_guid_t *)&var_guid, NULL, &size, &val);

That makes it less clear.  I think something like this makes it much more
obvious:

    static efi_status_t get_efi_var(const efi_char16_t *name,
				const efi_guid_t *vendor,
				u32 *attr,
				unsigned long *data_size, void *data)
    {
	return efi_call_runtime(get_variable,
				(efi_char16_t *)name, (efi_guid_t *)vendor,
				attr, data_size, data);
    }

And then doing:

	status = get_efi_var(efi_SecureBoot_name, &efi_variable_guid,
			     NULL, &size, &val);

which the compiler will inline.

> The "out_efi_err" portion differs from the previous version of this
> patch.  Setting a __u8 to a negative value, is this really what you
> want?

Eh?  efi_get_secureboot() returns an int as before.  The out_efi_err: portions
are exactly the same:

> -static int efi_get_secureboot(...)	> +int efi_get_secureboot(...)
> ...					> ...
> -out_efi_err:				> +out_efi_err:
> -	switch (status) {		> +	switch (status) {
> -	case EFI_NOT_FOUND:		> +	case EFI_NOT_FOUND:
> -		return 0;		> +		return 0;
> -	case EFI_DEVICE_ERROR:		> +	case EFI_DEVICE_ERROR:
> -		return -EIO;		> +		return -EIO;
> -	case EFI_SECURITY_VIOLATION:	> +	case EFI_SECURITY_VIOLATION:
> -		return -EACCES;		> +		return -EACCES;
> -	default:			> +	default:
> -		return -EINVAL;		> +		return -EINVAL;
> -	}				> +	}
> -}

David

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

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

From	David Howells <dhowells@redhat.com>
Date	2016-11-22 16:00 +0100
Subject	Re: [PATCH 4/6] efi: Get the secure boot status
Message-ID	<sGkI9-8uI-15@gated-at.bofh.it>
In reply to	#1527574

David Howells <dhowells@redhat.com> wrote:

> That makes it less clear.  I think something like this makes it much more
> obvious:
> 
>     static efi_status_t get_efi_var(const efi_char16_t *name,
> 				const efi_guid_t *vendor,
> 				u32 *attr,
> 				unsigned long *data_size, void *data)
>     {
> 	return efi_call_runtime(get_variable,
> 				(efi_char16_t *)name, (efi_guid_t *)vendor,
> 				attr, data_size, data);
>     }
> 
> And then doing:
> 
> 	status = get_efi_var(efi_SecureBoot_name, &efi_variable_guid,
> 			     NULL, &size, &val);
> 
> which the compiler will inline.

Of course, it has to be a macro because efi_call_runtime() has an undeclared
argument on ARM...

David

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

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

From	Lukas Wunner <lukas@wunner.de>
Date	2016-11-22 21:30 +0100
Subject	Re: [PATCH 4/6] efi: Get the secure boot status
Message-ID	<sGpRw-3CL-29@gated-at.bofh.it>
In reply to	#1527574

On Tue, Nov 22, 2016 at 02:47:27PM +0000, David Howells wrote:
> Lukas Wunner <lukas@wunner.de> wrote:
> > The "out_efi_err" portion differs from the previous version of this
> > patch.  Setting a __u8 to a negative value, is this really what you
> > want?
> 
> Eh?  efi_get_secureboot() returns an int as before.  The out_efi_err:
> portions are exactly the same:

By "the previous version of this patch" I was referring to your
submission of Nov 16, not the existing code in the kernel.
Your patch didn't contain the out_efi_err portion.

You're assigning a negative value to boot_params->secure_boot
(which is declared __u8).

In the next patch you're just checking if the value isn't 0
and you're considerung secure boot to be enabled even though
GetVariable failed.  Hence my question above, is this what
you want?  Likely not, perhaps this is what you really want:

	boot_params->secure_boot = (efi_get_secureboot() == 1);

Best regards,

Lukas

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

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

From	David Howells <dhowells@redhat.com>
Date	2016-11-23 01:10 +0100
Subject	Re: [PATCH 4/6] efi: Get the secure boot status
Message-ID	<sGtip-5Rm-3@gated-at.bofh.it>
In reply to	#1527903

Lukas Wunner <lukas@wunner.de> wrote:

> On Tue, Nov 22, 2016 at 02:47:27PM +0000, David Howells wrote:
> > Lukas Wunner <lukas@wunner.de> wrote:
> > > The "out_efi_err" portion differs from the previous version of this
> > > patch.  Setting a __u8 to a negative value, is this really what you
> > > want?
> > 
> > Eh?  efi_get_secureboot() returns an int as before.  The out_efi_err:
> > portions are exactly the same:
> 
> By "the previous version of this patch" I was referring to your
> submission of Nov 16, not the existing code in the kernel.
> Your patch didn't contain the out_efi_err portion.
> 
> You're assigning a negative value to boot_params->secure_boot
> (which is declared __u8).

Ah, yes.  Sorry, you confused me by specifying a comparison against the last
version.

David

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

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

From	David Howells <dhowells@redhat.com>
Date	2016-11-22 16:10 +0100
Subject	Re: [PATCH 4/6] efi: Get the secure boot status
Message-ID	<sGkRQ-lv-39@gated-at.bofh.it>
In reply to	#1527398

Lukas Wunner <lukas@wunner.de> wrote:

> You dropped the efi_system_table_t *sys_table_arg argument but this
> isn't defined anywhere as a static global.

It seems to me that passing this value in on x86 is probably a bad idea as
it's not mixed-mode safe.  Should I just pass NULL there in that case?

David

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

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

From	Lukas Wunner <lukas@wunner.de>
Date	2016-11-22 21:40 +0100
Subject	Re: [PATCH 4/6] efi: Get the secure boot status
Message-ID	<sGq1c-3Gf-25@gated-at.bofh.it>
In reply to	#1527586

On Tue, Nov 22, 2016 at 02:52:01PM +0000, David Howells wrote:
> Lukas Wunner <lukas@wunner.de> wrote:
> > You dropped the efi_system_table_t *sys_table_arg argument but this
> > isn't defined anywhere as a static global.
> 
> It seems to me that passing this value in on x86 is probably a bad idea as
> it's not mixed-mode safe.  Should I just pass NULL there in that case?

It's safe, it's merely a pointer below 4 Gig.  Just on dereference it
needs to be cast to the correct variant.  Passing in sys_table_arg is
done all over the place in the EFI stub, see e.g. efi_printk().

Best regards,

Lukas

[toc] | [prev] | [standalone]

Page 2 of 2 — ← Prev page 1 [2]

csiph-web

[PATCH 02/16] efi: Get the secure boot status

Contents

#1527662 — Re: [PATCH 1/6] x86/efi: Allow invocation of arbitrary runtime services

#1527689 — Re: [PATCH 1/6] x86/efi: Allow invocation of arbitrary runtime services

#1527702 — Re: [PATCH 1/6] x86/efi: Allow invocation of arbitrary runtime services

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

#1527161 — [PATCH 4/6] efi: Get the secure boot status

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

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

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

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

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

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

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

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