[PATCH 0/4] x86/xen/efi: Initialize UEFI secure boot state during dom0 boot

[PATCH 0/4] x86/xen/efi: Initialize UEFI secure boot state during dom0 boot

[Daniel Kiper]

Hi,

Initialize UEFI secure boot state during dom0 boot. Otherwise the kernel
may not even know that it runs on secure boot enabled platform.

Daniel

 arch/x86/xen/Makefile                          |    4 +++-
 arch/x86/xen/efi.c                             |   14 +++++++++++++
 drivers/firmware/efi/libstub/secureboot-core.c |   77 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 drivers/firmware/efi/libstub/secureboot.c      |   66 +++++------------------------------------------------------
 4 files changed, 99 insertions(+), 62 deletions(-)

Daniel Kiper (4):
      efi/stub: Extract efi_get_secureboot() to separate file
      x86/xen/efi: Initialize boot_params.secure_boot in xen_efi_init()
      efi: Tweak efi_get_secureboot() and its data section assignment
      efi: Rename efi_get_secureboot() to __efi_get_secureboot() and make it static

[PATCH 1/4] efi/stub: Extract efi_get_secureboot() to separate file

[Daniel Kiper]

We have to call efi_get_secureboot() from early Xen dom0 boot code to properly
initialize boot_params.secure_boot. Sadly it lives in the EFI stub. Hence, it is
not readily reachable from the kernel proper. So, move efi_get_secureboot() to
separate file which can be included from the core kernel code. Subsequent patch
will add efi_get_secureboot() call from Xen dom0 boot code.

There is no functional change.

Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
---
 drivers/firmware/efi/libstub/secureboot-core.c |   77 ++++++++++++++++++++++++
 drivers/firmware/efi/libstub/secureboot.c      |   66 +-------------------
 2 files changed, 78 insertions(+), 65 deletions(-)
 create mode 100644 drivers/firmware/efi/libstub/secureboot-core.c

diff --git a/drivers/firmware/efi/libstub/secureboot-core.c b/drivers/firmware/efi/libstub/secureboot-core.c
new file mode 100644
index 0000000..11a4feb
--- /dev/null
+++ b/drivers/firmware/efi/libstub/secureboot-core.c
@@ -0,0 +1,77 @@
+/*
+ * 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.
+ */
+
+/* BIOS variables */
+static const efi_guid_t efi_variable_guid = EFI_GLOBAL_VARIABLE_GUID;
+static const efi_char16_t efi_SecureBoot_name[] = {
+	'S', 'e', 'c', 'u', 'r', 'e', 'B', 'o', 'o', 't', 0
+};
+static const efi_char16_t 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 const efi_char16_t shim_MokSBState_name[] = {
+	'M', 'o', 'k', 'S', 'B', 'S', 't', 'a', 't', 'e', 0
+};
+
+/*
+ * Determine whether we're in secure boot mode.
+ */
+enum efi_secureboot_mode efi_get_secureboot(efi_system_table_t *sys_table_arg)
+{
+	u32 attr;
+	u8 secboot, setupmode, moksbstate;
+	unsigned long size;
+	efi_status_t status;
+
+	size = sizeof(secboot);
+	status = get_efi_var(efi_SecureBoot_name, &efi_variable_guid,
+			     NULL, &size, &secboot);
+	if (status == EFI_NOT_FOUND)
+		return efi_secureboot_mode_disabled;
+	if (status != EFI_SUCCESS)
+		goto out_efi_err;
+
+	size = sizeof(setupmode);
+	status = get_efi_var(efi_SetupMode_name, &efi_variable_guid,
+			     NULL, &size, &setupmode);
+	if (status != EFI_SUCCESS)
+		goto out_efi_err;
+
+	if (secboot == 0 || setupmode == 1)
+		return efi_secureboot_mode_disabled;
+
+	/*
+	 * See if a user has put the shim into insecure mode. If so, and if the
+	 * variable doesn't have the runtime attribute set, we might as well
+	 * honor that.
+	 */
+	size = sizeof(moksbstate);
+	status = get_efi_var(shim_MokSBState_name, &shim_guid,
+			     &attr, &size, &moksbstate);
+
+	/* If it fails, we don't care why. Default to secure */
+	if (status != EFI_SUCCESS)
+		goto secure_boot_enabled;
+	if (!(attr & EFI_VARIABLE_RUNTIME_ACCESS) && moksbstate == 1)
+		return efi_secureboot_mode_disabled;
+
+secure_boot_enabled:
+	pr_efi(sys_table_arg, "UEFI Secure Boot is enabled.\n");
+	return efi_secureboot_mode_enabled;
+
+out_efi_err:
+	pr_efi_err(sys_table_arg, "Could not determine UEFI Secure Boot status.\n");
+	return efi_secureboot_mode_unknown;
+}
diff --git a/drivers/firmware/efi/libstub/secureboot.c b/drivers/firmware/efi/libstub/secureboot.c
index 959777e..4a6159f 100644
--- a/drivers/firmware/efi/libstub/secureboot.c
+++ b/drivers/firmware/efi/libstub/secureboot.c
@@ -14,73 +14,9 @@
 
 #include "efistub.h"
 
-/* BIOS variables */
-static const efi_guid_t efi_variable_guid = EFI_GLOBAL_VARIABLE_GUID;
-static const efi_char16_t efi_SecureBoot_name[] = {
-	'S', 'e', 'c', 'u', 'r', 'e', 'B', 'o', 'o', 't', 0
-};
-static const efi_char16_t 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), \
 			 __VA_ARGS__);
 
-/*
- * Determine whether we're in secure boot mode.
- */
-enum efi_secureboot_mode efi_get_secureboot(efi_system_table_t *sys_table_arg)
-{
-	u32 attr;
-	u8 secboot, setupmode, moksbstate;
-	unsigned long size;
-	efi_status_t status;
-
-	size = sizeof(secboot);
-	status = get_efi_var(efi_SecureBoot_name, &efi_variable_guid,
-			     NULL, &size, &secboot);
-	if (status == EFI_NOT_FOUND)
-		return efi_secureboot_mode_disabled;
-	if (status != EFI_SUCCESS)
-		goto out_efi_err;
-
-	size = sizeof(setupmode);
-	status = get_efi_var(efi_SetupMode_name, &efi_variable_guid,
-			     NULL, &size, &setupmode);
-	if (status != EFI_SUCCESS)
-		goto out_efi_err;
-
-	if (secboot == 0 || setupmode == 1)
-		return efi_secureboot_mode_disabled;
-
-	/*
-	 * See if a user has put the shim into insecure mode. If so, and if the
-	 * variable doesn't have the runtime attribute set, we might as well
-	 * honor that.
-	 */
-	size = sizeof(moksbstate);
-	status = get_efi_var(shim_MokSBState_name, &shim_guid,
-			     &attr, &size, &moksbstate);
-
-	/* If it fails, we don't care why. Default to secure */
-	if (status != EFI_SUCCESS)
-		goto secure_boot_enabled;
-	if (!(attr & EFI_VARIABLE_RUNTIME_ACCESS) && moksbstate == 1)
-		return efi_secureboot_mode_disabled;
-
-secure_boot_enabled:
-	pr_efi(sys_table_arg, "UEFI Secure Boot is enabled.\n");
-	return efi_secureboot_mode_enabled;
-
-out_efi_err:
-	pr_efi_err(sys_table_arg, "Could not determine UEFI Secure Boot status.\n");
-	return efi_secureboot_mode_unknown;
-}
+#include "secureboot-core.c"
-- 
1.7.10.4

[PATCH 2/4] x86/xen/efi: Initialize boot_params.secure_boot in xen_efi_init()

[Daniel Kiper]

Otherwise the kernel reports incorrect UEFI secure boot state in the Xen dom0.

By the way fix CFLAGS_mmu_pv.o assignment alignment.

Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
---
 arch/x86/xen/Makefile |    4 +++-
 arch/x86/xen/efi.c    |   11 +++++++++++
 2 files changed, 14 insertions(+), 1 deletion(-)

diff --git a/arch/x86/xen/Makefile b/arch/x86/xen/Makefile
index d83cb54..1b07664 100644
--- a/arch/x86/xen/Makefile
+++ b/arch/x86/xen/Makefile
@@ -12,7 +12,9 @@ endif
 # Make sure early boot has no stackprotector
 nostackp := $(call cc-option, -fno-stack-protector)
 CFLAGS_enlighten_pv.o		:= $(nostackp)
-CFLAGS_mmu_pv.o		:= $(nostackp)
+CFLAGS_mmu_pv.o			:= $(nostackp)
+
+CFLAGS_efi.o			+= -I$(srctree)/drivers/firmware
 
 obj-y		:= enlighten.o multicalls.o mmu.o irq.o \
 			time.o xen-asm.o xen-asm_$(BITS).o \
diff --git a/arch/x86/xen/efi.c b/arch/x86/xen/efi.c
index a18703b..e089fa7 100644
--- a/arch/x86/xen/efi.c
+++ b/arch/x86/xen/efi.c
@@ -28,6 +28,15 @@
 #include <asm/setup.h>
 #include <asm/xen/hypercall.h>
 
+#define pr_efi(sys_table, msg)
+#define pr_efi_err(sys_table, msg)
+
+#define get_efi_var(name, vendor, attr, data_size, data) \
+		xen_efi_get_variable((efi_char16_t *)name, (efi_guid_t *)vendor, \
+				     attr, data_size, data)
+
+#include <efi/libstub/secureboot-core.c>
+
 static efi_char16_t vendor[100] __initdata;
 
 static efi_system_table_t efi_systab_xen __initdata = {
@@ -129,6 +138,8 @@ void __init xen_efi_init(void)
 	boot_params.efi_info.efi_systab = (__u32)__pa(efi_systab_xen);
 	boot_params.efi_info.efi_systab_hi = (__u32)(__pa(efi_systab_xen) >> 32);
 
+	boot_params.secure_boot = efi_get_secureboot(efi_systab_xen);
+
 	set_bit(EFI_BOOT, &efi.flags);
 	set_bit(EFI_PARAVIRT, &efi.flags);
 	set_bit(EFI_64BIT, &efi.flags);
-- 
1.7.10.4

[PATCH 3/4] efi: Tweak efi_get_secureboot() and its data section assignment

[Daniel Kiper]

Otherwise they are not freed after the kernel proper init.

Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
---
 arch/x86/xen/efi.c                             |    3 +++
 drivers/firmware/efi/libstub/secureboot-core.c |   12 ++++++------
 drivers/firmware/efi/libstub/secureboot.c      |    3 +++
 3 files changed, 12 insertions(+), 6 deletions(-)

diff --git a/arch/x86/xen/efi.c b/arch/x86/xen/efi.c
index e089fa7..5ad2b8f 100644
--- a/arch/x86/xen/efi.c
+++ b/arch/x86/xen/efi.c
@@ -28,6 +28,9 @@
 #include <asm/setup.h>
 #include <asm/xen/hypercall.h>
 
+#define __sb_init	__init
+#define __sb_initconst	__initconst
+
 #define pr_efi(sys_table, msg)
 #define pr_efi_err(sys_table, msg)
 
diff --git a/drivers/firmware/efi/libstub/secureboot-core.c b/drivers/firmware/efi/libstub/secureboot-core.c
index 11a4feb..d503ee4 100644
--- a/drivers/firmware/efi/libstub/secureboot-core.c
+++ b/drivers/firmware/efi/libstub/secureboot-core.c
@@ -11,24 +11,24 @@
  */
 
 /* BIOS variables */
-static const efi_guid_t efi_variable_guid = EFI_GLOBAL_VARIABLE_GUID;
-static const efi_char16_t efi_SecureBoot_name[] = {
+static const efi_guid_t efi_variable_guid __sb_initconst = EFI_GLOBAL_VARIABLE_GUID;
+static const efi_char16_t efi_SecureBoot_name[] __sb_initconst = {
 	'S', 'e', 'c', 'u', 'r', 'e', 'B', 'o', 'o', 't', 0
 };
-static const efi_char16_t efi_SetupMode_name[] = {
+static const efi_char16_t efi_SetupMode_name[] __sb_initconst = {
 	'S', 'e', 't', 'u', 'p', 'M', 'o', 'd', 'e', 0
 };
 
 /* SHIM variables */
-static const efi_guid_t shim_guid = EFI_SHIM_LOCK_GUID;
-static const efi_char16_t shim_MokSBState_name[] = {
+static const efi_guid_t shim_guid __sb_initconst = EFI_SHIM_LOCK_GUID;
+static const efi_char16_t shim_MokSBState_name[] __sb_initconst = {
 	'M', 'o', 'k', 'S', 'B', 'S', 't', 'a', 't', 'e', 0
 };
 
 /*
  * Determine whether we're in secure boot mode.
  */
-enum efi_secureboot_mode efi_get_secureboot(efi_system_table_t *sys_table_arg)
+enum __sb_init efi_secureboot_mode efi_get_secureboot(efi_system_table_t *sys_table_arg)
 {
 	u32 attr;
 	u8 secboot, setupmode, moksbstate;
diff --git a/drivers/firmware/efi/libstub/secureboot.c b/drivers/firmware/efi/libstub/secureboot.c
index 4a6159f..1142170 100644
--- a/drivers/firmware/efi/libstub/secureboot.c
+++ b/drivers/firmware/efi/libstub/secureboot.c
@@ -14,6 +14,9 @@
 
 #include "efistub.h"
 
+#define __sb_init
+#define __sb_initconst
+
 #define get_efi_var(name, vendor, ...) \
 	efi_call_runtime(get_variable, \
 			 (efi_char16_t *)(name), (efi_guid_t *)(vendor), \
-- 
1.7.10.4

[PATCH 4/4] efi: Rename efi_get_secureboot() to __efi_get_secureboot() and make it static

[Daniel Kiper]

This may help compiler to do some function call optimization.

This is rather cosmetic. If you like this patch apply.
If you do not you may ignore it.

Signed-off-by: Daniel Kiper <daniel.kiper@oracle.com>
---
 arch/x86/xen/efi.c                             |    2 +-
 drivers/firmware/efi/libstub/secureboot-core.c |    2 +-
 drivers/firmware/efi/libstub/secureboot.c      |    5 +++++
 3 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/arch/x86/xen/efi.c b/arch/x86/xen/efi.c
index 5ad2b8f..d45677f 100644
--- a/arch/x86/xen/efi.c
+++ b/arch/x86/xen/efi.c
@@ -141,7 +141,7 @@ void __init xen_efi_init(void)
 	boot_params.efi_info.efi_systab = (__u32)__pa(efi_systab_xen);
 	boot_params.efi_info.efi_systab_hi = (__u32)(__pa(efi_systab_xen) >> 32);
 
-	boot_params.secure_boot = efi_get_secureboot(efi_systab_xen);
+	boot_params.secure_boot = __efi_get_secureboot(efi_systab_xen);
 
 	set_bit(EFI_BOOT, &efi.flags);
 	set_bit(EFI_PARAVIRT, &efi.flags);
diff --git a/drivers/firmware/efi/libstub/secureboot-core.c b/drivers/firmware/efi/libstub/secureboot-core.c
index d503ee4..07526a6 100644
--- a/drivers/firmware/efi/libstub/secureboot-core.c
+++ b/drivers/firmware/efi/libstub/secureboot-core.c
@@ -28,7 +28,7 @@
 /*
  * Determine whether we're in secure boot mode.
  */
-enum __sb_init efi_secureboot_mode efi_get_secureboot(efi_system_table_t *sys_table_arg)
+static enum __sb_init efi_secureboot_mode __efi_get_secureboot(efi_system_table_t *sys_table_arg)
 {
 	u32 attr;
 	u8 secboot, setupmode, moksbstate;
diff --git a/drivers/firmware/efi/libstub/secureboot.c b/drivers/firmware/efi/libstub/secureboot.c
index 1142170..f872afd 100644
--- a/drivers/firmware/efi/libstub/secureboot.c
+++ b/drivers/firmware/efi/libstub/secureboot.c
@@ -23,3 +23,8 @@
 			 __VA_ARGS__);
 
 #include "secureboot-core.c"
+
+enum efi_secureboot_mode efi_get_secureboot(efi_system_table_t *sys_table_arg)
+{
+	return __efi_get_secureboot(sys_table_arg);
+}
-- 
1.7.10.4

Re: [PATCH 0/4] x86/xen/efi: Initialize UEFI secure boot state during dom0 boot

[Ard Biesheuvel]

On 9 January 2018 at 14:22, Daniel Kiper <daniel.kiper@oracle.com> wrote:
> Hi,
>
> Initialize UEFI secure boot state during dom0 boot. Otherwise the kernel
> may not even know that it runs on secure boot enabled platform.
>

Hi Daniel,

I must say, I am not too thrilled with the approach you have chosen
here. #including .c files in other .c files, and using #defines to
override C functions or other stub functionality is rather fragile. In
particular, it means we have to start caring about not breaking
Xen/x86 code when making modifications to the EFI stub, and that code
is already difficult enough to maintain, given that it is shared
between ARM, arm64 and x86, and runs either from the decompressor or
the kernel proper (arm64) but in the context of the UEFI firmware.
None of the stub code currently runs in ordinary kernel context.

So please, could you try to find another way to do this?

Thanks,
Ard.


>
>  arch/x86/xen/Makefile                          |    4 +++-
>  arch/x86/xen/efi.c                             |   14 +++++++++++++
>  drivers/firmware/efi/libstub/secureboot-core.c |   77 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  drivers/firmware/efi/libstub/secureboot.c      |   66 +++++------------------------------------------------------
>  4 files changed, 99 insertions(+), 62 deletions(-)
>
> Daniel Kiper (4):
>       efi/stub: Extract efi_get_secureboot() to separate file
>       x86/xen/efi: Initialize boot_params.secure_boot in xen_efi_init()
>       efi: Tweak efi_get_secureboot() and its data section assignment
>       efi: Rename efi_get_secureboot() to __efi_get_secureboot() and make it static
>

Re: [PATCH 0/4] x86/xen/efi: Initialize UEFI secure boot state during dom0 boot

[Daniel Kiper]

Hi Ard,

On Thu, Jan 11, 2018 at 12:51:07PM +0000, Ard Biesheuvel wrote:
> On 9 January 2018 at 14:22, Daniel Kiper <daniel.kiper@oracle.com> wrote:
> > Hi,
> >
> > Initialize UEFI secure boot state during dom0 boot. Otherwise the kernel
> > may not even know that it runs on secure boot enabled platform.
>
> Hi Daniel,
>
> I must say, I am not too thrilled with the approach you have chosen
> here. #including .c files in other .c files, and using #defines to
> override C functions or other stub functionality is rather fragile. In

TBH I do not like it too. Sadly I have not find a better solution for
that. I wish to avoid code duplication as much as possible because
otherwise it will fall out of sync sooner or later (usually sooner).
Similar thing happened in different part of Xen EFI code a few months ago.

> particular, it means we have to start caring about not breaking
> Xen/x86 code when making modifications to the EFI stub, and that code
> is already difficult enough to maintain, given that it is shared
> between ARM, arm64 and x86, and runs either from the decompressor or
> the kernel proper (arm64) but in the context of the UEFI firmware.

I understand that.

> None of the stub code currently runs in ordinary kernel context.

Yep.

> So please, could you try to find another way to do this?

I am happy to improve the situation, however, I am afraid that it is
difficult here. Stub and kernel proper are separate entities and simple
linking does not work. So, It seems to me that only play with includes
will allow us to not duplicate the code. However, if you have a better
idea I am happy to implement it.

Daniel

Re: [PATCH 0/4] x86/xen/efi: Initialize UEFI secure boot state during dom0 boot

[Ard Biesheuvel]

On 12 January 2018 at 11:24, Daniel Kiper <daniel.kiper@oracle.com> wrote:
> Hi Ard,
>
> On Thu, Jan 11, 2018 at 12:51:07PM +0000, Ard Biesheuvel wrote:
>> On 9 January 2018 at 14:22, Daniel Kiper <daniel.kiper@oracle.com> wrote:
>> > Hi,
>> >
>> > Initialize UEFI secure boot state during dom0 boot. Otherwise the kernel
>> > may not even know that it runs on secure boot enabled platform.
>>
>> Hi Daniel,
>>
>> I must say, I am not too thrilled with the approach you have chosen
>> here. #including .c files in other .c files, and using #defines to
>> override C functions or other stub functionality is rather fragile. In
>
> TBH I do not like it too. Sadly I have not find a better solution for
> that. I wish to avoid code duplication as much as possible because
> otherwise it will fall out of sync sooner or later (usually sooner).
> Similar thing happened in different part of Xen EFI code a few months ago.
>
>> particular, it means we have to start caring about not breaking
>> Xen/x86 code when making modifications to the EFI stub, and that code
>> is already difficult enough to maintain, given that it is shared
>> between ARM, arm64 and x86, and runs either from the decompressor or
>> the kernel proper (arm64) but in the context of the UEFI firmware.
>
> I understand that.
>
>> None of the stub code currently runs in ordinary kernel context.
>
> Yep.
>
>> So please, could you try to find another way to do this?
>
> I am happy to improve the situation, however, I am afraid that it is
> difficult here. Stub and kernel proper are separate entities and simple
> linking does not work. So, It seems to me that only play with includes
> will allow us to not duplicate the code. However, if you have a better
> idea I am happy to implement it.
>

Actually, there is another reason why it does not make sense to reuse that code.

This code

/*
* See if a user has put the 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(L"MokSBState", &shim_guid,
     &attr, &size, &moksbstate);

/* If it fails, we don't care why. Default to secure */
if (status != EFI_SUCCESS)
    goto secure_boot_enabled;
if (!(attr & EFI_VARIABLE_RUNTIME_ACCESS) && moksbstate == 1)
    return efi_secureboot_mode_disabled;

will always fail after exiting boot services, so it makes no sense to
call it from xen_efi_init().

So I suggest you just clone the function and only keep the pieces that
make sense for Xen.

-- 
Ard.