Skip to contentSkip to conversation

efi: x86/xen: switch to efi_get_secureboot_mode helper

PATCH In review Ard Biesheuvel 2 participants 3 messages Nov 4, 2020

Subsystems (3)
lore
Share & tools
lore

Ard Biesheuvel

author maintainer
#0 lore raw

PATCH 1/1 efi: x86/xen: switch to efi_get_secureboot_mode helper

Now that we have a static inline helper to discover the platform's secure boot mode that can be shared between the EFI stub and the kernel proper, switch to it, and drop some comments about keeping them in sync manually.

---
 arch/x86/xen/efi.c                        | 37 +++++---------------
 drivers/firmware/efi/libstub/secureboot.c |  3 --
 2 files changed, 9 insertions(+), 31 deletions(-)
arch/x86/xen/efi.c +9 -28
index 205a9bc981b0..a27444acaf1e 100644
@@ -93,37 +93,22 @@ static efi_system_table_t __init *xen_efi_probe(void)
 
 /*
  * Determine whether we're in secure boot mode.
- *
- * Please keep the logic in sync with
- * drivers/firmware/efi/libstub/secureboot.c:efi_get_secureboot().
  */
 static enum efi_secureboot_mode xen_efi_get_secureboot(void)
 {
-	static efi_guid_t efi_variable_guid = EFI_GLOBAL_VARIABLE_GUID;
 	static efi_guid_t shim_guid = EFI_SHIM_LOCK_GUID;
+	enum efi_secureboot_mode mode;
 	efi_status_t status;
-	u8 moksbstate, secboot, setupmode;
+	u8 moksbstate;
 	unsigned long size;
 
-	size = sizeof(secboot);
-	status = efi.get_variable(L"SecureBoot", &efi_variable_guid,
-				  NULL, &size, &secboot);
-
-	if (status == EFI_NOT_FOUND)
-		return efi_secureboot_mode_disabled;
-
-	if (status != EFI_SUCCESS)
-		goto out_efi_err;
-
-	size = sizeof(setupmode);
-	status = efi.get_variable(L"SetupMode", &efi_variable_guid,
-				  NULL, &size, &setupmode);
-
-	if (status != EFI_SUCCESS)
-		goto out_efi_err;
-
-	if (secboot == 0 || setupmode == 1)
-		return efi_secureboot_mode_disabled;
+	mode = efi_get_secureboot_mode(efi.get_variable);
+	if (mode == efi_secureboot_mode_unknown) {
+		efi_err("Could not determine UEFI Secure Boot status.\n");
+		return efi_secureboot_mode_unknown;
+	}
+	if (mode != efi_secureboot_mode_enabled)
+		return mode;
 
 	/* See if a user has put the shim into insecure mode. */
 	size = sizeof(moksbstate);
@@ -140,10 +125,6 @@ static enum efi_secureboot_mode xen_efi_get_secureboot(void)
  secure_boot_enabled:
 	pr_info("UEFI Secure Boot is enabled.\n");
 	return efi_secureboot_mode_enabled;
-
- out_efi_err:
-	pr_err("Could not determine UEFI Secure Boot status.\n");
-	return efi_secureboot_mode_unknown;
 }
 
 void __init xen_efi_init(struct boot_params *boot_params)
drivers/firmware/efi/libstub/secureboot.c +0 -3
index af18d86c1604..8a18930f3eb6 100644
@@ -24,9 +24,6 @@ static efi_status_t get_var(efi_char16_t *name, efi_guid_t *vendor, u32 *attr,
 
 /*
  * Determine whether we're in secure boot mode.
- *
- * Please keep the logic in sync with
- * arch/x86/xen/efi.c:xen_efi_get_secureboot().
  */
 enum efi_secureboot_mode efi_get_secureboot(void)
 {
Signature · 1 line
2.17.1
#1 lore raw
Quoted 42 lines · Ard Biesheuvel

Now that we have a static inline helper to discover the platform's secure boot mode that can be shared between the EFI stub and the kernel proper, switch to it, and drop some comments about keeping them in sync manually.

---
 arch/x86/xen/efi.c                        | 37 +++++---------------
 drivers/firmware/efi/libstub/secureboot.c |  3 --
 2 files changed, 9 insertions(+), 31 deletions(-)
arch/x86/xen/efi.c +2 -15
index 205a9bc981b0..a27444acaf1e 100644
@@ -93,37 +93,22 @@ static efi_system_table_t __init *xen_efi_probe(void)
 
 /*
  * Determine whether we're in secure boot mode.
- *
- * Please keep the logic in sync with
- * drivers/firmware/efi/libstub/secureboot.c:efi_get_secureboot().
  */
 static enum efi_secureboot_mode xen_efi_get_secureboot(void)
 {
-	static efi_guid_t efi_variable_guid = EFI_GLOBAL_VARIABLE_GUID;
 	static efi_guid_t shim_guid = EFI_SHIM_LOCK_GUID;
+	enum efi_secureboot_mode mode;
 	efi_status_t status;
-	u8 moksbstate, secboot, setupmode;
+	u8 moksbstate;
 	unsigned long size;
 
-	size = sizeof(secboot);
-	status = efi.get_variable(L"SecureBoot", &efi_variable_guid,
-				  NULL, &size, &secboot);
-
-	if (status == EFI_NOT_FOUND)
-		return efi_secureboot_mode_disabled;
-
-	if (status != EFI_SUCCESS)
-		goto out_efi_err;
-

arch/x86/xen/efi.c static efi_system_table_t __init *xen_efi_probe(void) · line 105

Quoted 10 lines
arch/x86/xen/efi.c +1 -9
-	size = sizeof(setupmode);
-	status = efi.get_variable(L"SetupMode", &efi_variable_guid,
-				  NULL, &size, &setupmode);
-
-	if (status != EFI_SUCCESS)
-		goto out_efi_err;
-
-	if (secboot == 0 || setupmode == 1)
-		return efi_secureboot_mode_disabled;
+	mode = efi_get_secureboot_mode(efi.get_variable);

Which tree is this patch against? I don't see a definition of efi_get_secureboot_mode().

arch/x86/xen/efi.c static efi_system_table_t __init *xen_efi_probe(void) · line 107

Quoted 2 lines
arch/x86/xen/efi.c +2 -0
+	if (mode == efi_secureboot_mode_unknown) {
+		efi_err("Could not determine UEFI Secure Boot status.\n");

We need to include drivers/firmware/efi/libstub/efistub.h for that. Which I am not sure is meant to be included anywhere outside of libstub.

-boris

author maintainer
#2 lore raw
Quoted 60 lines · On Thu, 5 Nov 2020 at 00:53, <[email protected]> wrote:
Quoted 42 lines · Ard Biesheuvel

Now that we have a static inline helper to discover the platform's secure boot mode that can be shared between the EFI stub and the kernel proper, switch to it, and drop some comments about keeping them in sync manually.

---
 arch/x86/xen/efi.c                        | 37 +++++---------------
 drivers/firmware/efi/libstub/secureboot.c |  3 --
 2 files changed, 9 insertions(+), 31 deletions(-)
arch/x86/xen/efi.c +2 -15
index 205a9bc981b0..a27444acaf1e 100644
@@ -93,37 +93,22 @@ static efi_system_table_t __init *xen_efi_probe(void)
 
 /*
  * Determine whether we're in secure boot mode.
- *
- * Please keep the logic in sync with
- * drivers/firmware/efi/libstub/secureboot.c:efi_get_secureboot().
  */
 static enum efi_secureboot_mode xen_efi_get_secureboot(void)
 {
-     static efi_guid_t efi_variable_guid = EFI_GLOBAL_VARIABLE_GUID;
      static efi_guid_t shim_guid = EFI_SHIM_LOCK_GUID;
+     enum efi_secureboot_mode mode;
      efi_status_t status;
-     u8 moksbstate, secboot, setupmode;
+     u8 moksbstate;
      unsigned long size;
 
-     size = sizeof(secboot);
-     status = efi.get_variable(L"SecureBoot", &efi_variable_guid,
-                               NULL, &size, &secboot);
-
-     if (status == EFI_NOT_FOUND)
-             return efi_secureboot_mode_disabled;
-
-     if (status != EFI_SUCCESS)
-             goto out_efi_err;
-

arch/x86/xen/efi.c static efi_system_table_t __init *xen_efi_probe(void) · line 105

Quoted 10 lines
arch/x86/xen/efi.c +1 -9
-     size = sizeof(setupmode);
-     status = efi.get_variable(L"SetupMode", &efi_variable_guid,
-                               NULL, &size, &setupmode);
-
-     if (status != EFI_SUCCESS)
-             goto out_efi_err;
-
-     if (secboot == 0 || setupmode == 1)
-             return efi_secureboot_mode_disabled;
+     mode = efi_get_secureboot_mode(efi.get_variable);

Which tree is this patch against? I don't see a definition of efi_get_secureboot_mode().

Quoted 6 lines

arch/x86/xen/efi.c static efi_system_table_t __init *xen_efi_probe(void) · line 107

Quoted 2 lines
arch/x86/xen/efi.c +2 -0
+     if (mode == efi_secureboot_mode_unknown) {
+             efi_err("Could not determine UEFI Secure Boot status.\n");

We need to include drivers/firmware/efi/libstub/efistub.h for that. Which I am not sure is meant to be included anywhere outside of libstub.

Ah yes, my mistake - that should be pr_err, not efi_err.