Re: [PATCH v3 1/3] efi: generalize efi_get_secureboot
Hi Ard,
Thanks for your time and reviewing.
On Fri, Oct 30, 2020 at 12:51:10PM +0100, Ard Biesheuvel wrote:
> Hello Chester,
>
> Thanks again for looking into this.
>
> On Fri, 30 Oct 2020 at 07:09, Chester Lin wrote:
> >
> > Generalize the efi_get_secureboot() function so not only efistub but also
> > other subsystems can use it.
> >
> > Signed-off-by: Chester Lin
> > ---
> > drivers/firmware/efi/libstub/Makefile | 2 +-
> > drivers/firmware/efi/libstub/efi-stub.c | 2 +-
> > drivers/firmware/efi/libstub/efistub.h| 22 ---
> > drivers/firmware/efi/libstub/secureboot.c | 76 ---
> > drivers/firmware/efi/libstub/x86-stub.c | 2 +-
> > include/linux/efi.h | 41 +++-
> > 6 files changed, 57 insertions(+), 88 deletions(-)
> > delete mode 100644 drivers/firmware/efi/libstub/secureboot.c
> >
> > diff --git a/drivers/firmware/efi/libstub/Makefile
> > b/drivers/firmware/efi/libstub/Makefile
> > index 8a94388e38b3..88e47b0ca09d 100644
> > --- a/drivers/firmware/efi/libstub/Makefile
> > +++ b/drivers/firmware/efi/libstub/Makefile
> > @@ -49,7 +49,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 secureboot.o
> > tpm.o \
> > +lib-y := efi-stub-helper.o gop.o tpm.o \
> >file.o mem.o random.o randomalloc.o
> > pci.o \
> >skip_spaces.o lib-cmdline.o lib-ctype.o \
> >alignedmem.o relocate.o vsprintf.o
> > diff --git a/drivers/firmware/efi/libstub/efi-stub.c
> > b/drivers/firmware/efi/libstub/efi-stub.c
> > index 914a343c7785..ad96f1d786a9 100644
> > --- a/drivers/firmware/efi/libstub/efi-stub.c
> > +++ b/drivers/firmware/efi/libstub/efi-stub.c
> > @@ -196,7 +196,7 @@ efi_status_t __efiapi efi_pe_entry(efi_handle_t handle,
> > /* Ask the firmware to clear memory on unclean shutdown */
> > efi_enable_reset_attack_mitigation();
> >
> > - secure_boot = efi_get_secureboot();
> > + secure_boot = efi_get_secureboot(get_efi_var);
> >
> > /*
> > * Unauthenticated device tree data is a security hazard, so ignore
> > diff --git a/drivers/firmware/efi/libstub/efistub.h
> > b/drivers/firmware/efi/libstub/efistub.h
> > index 2d7abcd99de9..b1833b51e6d6 100644
> > --- a/drivers/firmware/efi/libstub/efistub.h
> > +++ b/drivers/firmware/efi/libstub/efistub.h
> > @@ -91,14 +91,6 @@ efi_status_t __efiapi efi_pe_entry(efi_handle_t handle,
> > fdt_setprop((fdt), (node_offset), (name), &(var), sizeof(var))
> > #endif
> >
> > -#define get_efi_var(name, vendor, ...) \
> > - efi_rt_call(get_variable, (efi_char16_t *)(name), \
> > - (efi_guid_t *)(vendor), __VA_ARGS__)
> > -
> > -#define set_efi_var(name, vendor, ...) \
> > - efi_rt_call(set_variable, (efi_char16_t *)(name), \
> > - (efi_guid_t *)(vendor), __VA_ARGS__)
> > -
> > #define efi_get_handle_at(array, idx) \
> > (efi_is_native() ? (array)[idx] \
> > : (efi_handle_t)(unsigned long)((u32 *)(array))[idx])
> > @@ -112,6 +104,20 @@ efi_status_t __efiapi efi_pe_entry(efi_handle_t handle,
> > ((handle = efi_get_handle_at((array), i)) || true); \
> > i++)
> >
> > +static inline
> > +efi_status_t get_efi_var(efi_char16_t *name, efi_guid_t *vendor, u32 *attr,
> > +unsigned long *size, void *data)
> > +{
> > + return efi_rt_call(get_variable, name, vendor, attr, size, data);
> > +}
> > +
> > +static inline
> > +efi_status_t set_efi_var(efi_char16_t *name, efi_guid_t *vendor, u32 attr,
> > +unsigned long size, void *data)
> > +{
> > + return efi_rt_call(set_variable, name, vendor, attr, size, data);
> > +}
> > +
> > static inline
> > void efi_set_u64_split(u64 data, u32 *lo, u32 *hi)
> > {
> > diff --git a/drivers/firmware/efi/libstub/secureboot.c
> > b/drivers/firmware/efi/libstub/secureboot.c
> > deleted file mode 100644
> > index 5efc524b14be..
> > --- a/drivers/firmware/efi/libstub/secureboot.c
> > +++ /dev/null
>
> Please keep this file (see below)
>
> > @@ -1,76 +0,0 @@
> > -// SPDX-License-Identifier: GPL-2.0
> > -/*
> > - * Secure boot handling.
> > - *
> > - * Copyright (C) 2013,2014 Linaro Limited
> > - * Roy Franz > - * Copyright (C) 2013 Red Hat, Inc.
> > - * Mark Salter
> > - */
> > -#include
> > -#include
> > -
> > -#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[] = L"SecureBoot";
> > -static const
Re: [PATCH v3 1/3] efi: generalize efi_get_secureboot
Hello Chester,
Thanks again for looking into this.
On Fri, 30 Oct 2020 at 07:09, Chester Lin wrote:
>
> Generalize the efi_get_secureboot() function so not only efistub but also
> other subsystems can use it.
>
> Signed-off-by: Chester Lin
> ---
> drivers/firmware/efi/libstub/Makefile | 2 +-
> drivers/firmware/efi/libstub/efi-stub.c | 2 +-
> drivers/firmware/efi/libstub/efistub.h| 22 ---
> drivers/firmware/efi/libstub/secureboot.c | 76 ---
> drivers/firmware/efi/libstub/x86-stub.c | 2 +-
> include/linux/efi.h | 41 +++-
> 6 files changed, 57 insertions(+), 88 deletions(-)
> delete mode 100644 drivers/firmware/efi/libstub/secureboot.c
>
> diff --git a/drivers/firmware/efi/libstub/Makefile
> b/drivers/firmware/efi/libstub/Makefile
> index 8a94388e38b3..88e47b0ca09d 100644
> --- a/drivers/firmware/efi/libstub/Makefile
> +++ b/drivers/firmware/efi/libstub/Makefile
> @@ -49,7 +49,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 secureboot.o tpm.o
> \
> +lib-y := efi-stub-helper.o gop.o tpm.o \
>file.o mem.o random.o randomalloc.o pci.o \
>skip_spaces.o lib-cmdline.o lib-ctype.o \
>alignedmem.o relocate.o vsprintf.o
> diff --git a/drivers/firmware/efi/libstub/efi-stub.c
> b/drivers/firmware/efi/libstub/efi-stub.c
> index 914a343c7785..ad96f1d786a9 100644
> --- a/drivers/firmware/efi/libstub/efi-stub.c
> +++ b/drivers/firmware/efi/libstub/efi-stub.c
> @@ -196,7 +196,7 @@ efi_status_t __efiapi efi_pe_entry(efi_handle_t handle,
> /* Ask the firmware to clear memory on unclean shutdown */
> efi_enable_reset_attack_mitigation();
>
> - secure_boot = efi_get_secureboot();
> + secure_boot = efi_get_secureboot(get_efi_var);
>
> /*
> * Unauthenticated device tree data is a security hazard, so ignore
> diff --git a/drivers/firmware/efi/libstub/efistub.h
> b/drivers/firmware/efi/libstub/efistub.h
> index 2d7abcd99de9..b1833b51e6d6 100644
> --- a/drivers/firmware/efi/libstub/efistub.h
> +++ b/drivers/firmware/efi/libstub/efistub.h
> @@ -91,14 +91,6 @@ efi_status_t __efiapi efi_pe_entry(efi_handle_t handle,
> fdt_setprop((fdt), (node_offset), (name), &(var), sizeof(var))
> #endif
>
> -#define get_efi_var(name, vendor, ...) \
> - efi_rt_call(get_variable, (efi_char16_t *)(name), \
> - (efi_guid_t *)(vendor), __VA_ARGS__)
> -
> -#define set_efi_var(name, vendor, ...) \
> - efi_rt_call(set_variable, (efi_char16_t *)(name), \
> - (efi_guid_t *)(vendor), __VA_ARGS__)
> -
> #define efi_get_handle_at(array, idx) \
> (efi_is_native() ? (array)[idx] \
> : (efi_handle_t)(unsigned long)((u32 *)(array))[idx])
> @@ -112,6 +104,20 @@ efi_status_t __efiapi efi_pe_entry(efi_handle_t handle,
> ((handle = efi_get_handle_at((array), i)) || true); \
> i++)
>
> +static inline
> +efi_status_t get_efi_var(efi_char16_t *name, efi_guid_t *vendor, u32 *attr,
> +unsigned long *size, void *data)
> +{
> + return efi_rt_call(get_variable, name, vendor, attr, size, data);
> +}
> +
> +static inline
> +efi_status_t set_efi_var(efi_char16_t *name, efi_guid_t *vendor, u32 attr,
> +unsigned long size, void *data)
> +{
> + return efi_rt_call(set_variable, name, vendor, attr, size, data);
> +}
> +
> static inline
> void efi_set_u64_split(u64 data, u32 *lo, u32 *hi)
> {
> diff --git a/drivers/firmware/efi/libstub/secureboot.c
> b/drivers/firmware/efi/libstub/secureboot.c
> deleted file mode 100644
> index 5efc524b14be..
> --- a/drivers/firmware/efi/libstub/secureboot.c
> +++ /dev/null
Please keep this file (see below)
> @@ -1,76 +0,0 @@
> -// SPDX-License-Identifier: GPL-2.0
> -/*
> - * Secure boot handling.
> - *
> - * Copyright (C) 2013,2014 Linaro Limited
> - * Roy Franz - * Copyright (C) 2013 Red Hat, Inc.
> - * Mark Salter
> - */
> -#include
> -#include
> -
> -#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[] = L"SecureBoot";
> -static const efi_char16_t efi_SetupMode_name[] = L"SetupMode";
> -
> -/* SHIM variables */
> -static const efi_guid_t shim_guid = EFI_SHIM_LOCK_GUID;
> -static const efi_char16_t shim_MokSBState_name[] = L"MokSBState";
> -
> -/*
> - * Determine whether we're in secure boot mode.
> - *
> - * Please keep the logic in sync with
> - *
[PATCH v3 1/3] efi: generalize efi_get_secureboot
Generalize the efi_get_secureboot() function so not only efistub but also
other subsystems can use it.
Signed-off-by: Chester Lin
---
drivers/firmware/efi/libstub/Makefile | 2 +-
drivers/firmware/efi/libstub/efi-stub.c | 2 +-
drivers/firmware/efi/libstub/efistub.h| 22 ---
drivers/firmware/efi/libstub/secureboot.c | 76 ---
drivers/firmware/efi/libstub/x86-stub.c | 2 +-
include/linux/efi.h | 41 +++-
6 files changed, 57 insertions(+), 88 deletions(-)
delete mode 100644 drivers/firmware/efi/libstub/secureboot.c
diff --git a/drivers/firmware/efi/libstub/Makefile
b/drivers/firmware/efi/libstub/Makefile
index 8a94388e38b3..88e47b0ca09d 100644
--- a/drivers/firmware/efi/libstub/Makefile
+++ b/drivers/firmware/efi/libstub/Makefile
@@ -49,7 +49,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 secureboot.o tpm.o \
+lib-y := efi-stub-helper.o gop.o tpm.o \
file.o mem.o random.o randomalloc.o pci.o \
skip_spaces.o lib-cmdline.o lib-ctype.o \
alignedmem.o relocate.o vsprintf.o
diff --git a/drivers/firmware/efi/libstub/efi-stub.c
b/drivers/firmware/efi/libstub/efi-stub.c
index 914a343c7785..ad96f1d786a9 100644
--- a/drivers/firmware/efi/libstub/efi-stub.c
+++ b/drivers/firmware/efi/libstub/efi-stub.c
@@ -196,7 +196,7 @@ efi_status_t __efiapi efi_pe_entry(efi_handle_t handle,
/* Ask the firmware to clear memory on unclean shutdown */
efi_enable_reset_attack_mitigation();
- secure_boot = efi_get_secureboot();
+ secure_boot = efi_get_secureboot(get_efi_var);
/*
* Unauthenticated device tree data is a security hazard, so ignore
diff --git a/drivers/firmware/efi/libstub/efistub.h
b/drivers/firmware/efi/libstub/efistub.h
index 2d7abcd99de9..b1833b51e6d6 100644
--- a/drivers/firmware/efi/libstub/efistub.h
+++ b/drivers/firmware/efi/libstub/efistub.h
@@ -91,14 +91,6 @@ efi_status_t __efiapi efi_pe_entry(efi_handle_t handle,
fdt_setprop((fdt), (node_offset), (name), &(var), sizeof(var))
#endif
-#define get_efi_var(name, vendor, ...) \
- efi_rt_call(get_variable, (efi_char16_t *)(name), \
- (efi_guid_t *)(vendor), __VA_ARGS__)
-
-#define set_efi_var(name, vendor, ...) \
- efi_rt_call(set_variable, (efi_char16_t *)(name), \
- (efi_guid_t *)(vendor), __VA_ARGS__)
-
#define efi_get_handle_at(array, idx) \
(efi_is_native() ? (array)[idx] \
: (efi_handle_t)(unsigned long)((u32 *)(array))[idx])
@@ -112,6 +104,20 @@ efi_status_t __efiapi efi_pe_entry(efi_handle_t handle,
((handle = efi_get_handle_at((array), i)) || true); \
i++)
+static inline
+efi_status_t get_efi_var(efi_char16_t *name, efi_guid_t *vendor, u32 *attr,
+unsigned long *size, void *data)
+{
+ return efi_rt_call(get_variable, name, vendor, attr, size, data);
+}
+
+static inline
+efi_status_t set_efi_var(efi_char16_t *name, efi_guid_t *vendor, u32 attr,
+unsigned long size, void *data)
+{
+ return efi_rt_call(set_variable, name, vendor, attr, size, data);
+}
+
static inline
void efi_set_u64_split(u64 data, u32 *lo, u32 *hi)
{
diff --git a/drivers/firmware/efi/libstub/secureboot.c
b/drivers/firmware/efi/libstub/secureboot.c
deleted file mode 100644
index 5efc524b14be..
--- a/drivers/firmware/efi/libstub/secureboot.c
+++ /dev/null
@@ -1,76 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0
-/*
- * Secure boot handling.
- *
- * Copyright (C) 2013,2014 Linaro Limited
- * Roy Franz
- */
-#include
-#include
-
-#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[] = L"SecureBoot";
-static const efi_char16_t efi_SetupMode_name[] = L"SetupMode";
-
-/* SHIM variables */
-static const efi_guid_t shim_guid = EFI_SHIM_LOCK_GUID;
-static const efi_char16_t shim_MokSBState_name[] = L"MokSBState";
-
-/*
- * 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)
-{
- u32 attr;
- u8 secboot, setupmode, moksbstate;
- unsigned long size;
- efi_status_t status;
-
- size = sizeof(secboot);
- status = get_efi_var(efi_SecureBoot_name, _variable_guid,
-NULL, , );
- if (status == EFI_NOT_FOUND)
- return efi_secureboot_mode_disabled;
-
