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 <clin@xxxxxxxx> wrote:
> >
> > Generalize the efi_get_secureboot() function so not only efistub but also
> > other subsystems can use it.
> >
> > Signed-off-by: Chester Lin <clin@xxxxxxxx>
> > ---
> > 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..000000000000
> > --- 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 <roy.franz@xxxxxxxxxx
> > - * Copyright (C) 2013 Red Hat, Inc.
> > - * Mark Salter <msalter@xxxxxxxxxx>
> > - */
> > -#include <linux/efi.h>
> > -#include <asm/efi.h>
> > -
> > -#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, &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);
> > -
>
> MokSBState is a boot time variable, so we cannot access it when
> running under the OS. Xen also has a code flow similar to this one,
> but it looks at MokSbStateRt instead (which may be a mistake but let's
> forget about that for now)
>
> So what we will need to do is factor out only the top part of this
> function (which, incidentally, is the only part that IMA uses in the
i> first place)
>
Thanks for the reminder. I will take this change into next revision.
> > - /* 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:
> > - efi_info("UEFI Secure Boot is enabled.\n");
> > - return efi_secureboot_mode_enabled;
> > -
> > -out_efi_err:
> > - efi_err("Could not determine UEFI Secure Boot status.\n");
> > - return efi_secureboot_mode_unknown;
> > -}
>
> So let's keep this file, and also, let's put a wrapper function around
> get_efi_var() here, of which you can take the address and pass to the
> static inline function.
If I understand correctly, that means it's better to define a new wrapper
function around the get_efi_var() rather than changing it from a macro to
an inline function. Please feel free to let me know if I misunderstand it.
>
> > diff --git a/drivers/firmware/efi/libstub/x86-stub.c b/drivers/firmware/efi/libstub/x86-stub.c
> > index 3672539cb96e..3f9b492c566b 100644
> > --- a/drivers/firmware/efi/libstub/x86-stub.c
> > +++ b/drivers/firmware/efi/libstub/x86-stub.c
> > @@ -781,7 +781,7 @@ unsigned long efi_main(efi_handle_t handle,
> > * otherwise we ask the BIOS.
> > */
> > if (boot_params->secure_boot == efi_secureboot_mode_unset)
> > - boot_params->secure_boot = efi_get_secureboot();
> > + boot_params->secure_boot = efi_get_secureboot(get_efi_var);
> >
> > /* Ask the firmware to clear memory on unclean shutdown */
> > efi_enable_reset_attack_mitigation();
> > diff --git a/include/linux/efi.h b/include/linux/efi.h
> > index d7c0e73af2b9..cc2d3de39031 100644
> > --- a/include/linux/efi.h
> > +++ b/include/linux/efi.h
> > @@ -1089,7 +1089,46 @@ enum efi_secureboot_mode {
> > efi_secureboot_mode_disabled,
> > efi_secureboot_mode_enabled,
> > };
> > -enum efi_secureboot_mode efi_get_secureboot(void);
> > +
> > +static inline enum efi_secureboot_mode efi_get_secureboot(efi_get_variable_t *get_var)
> > +{
> > + efi_guid_t var_guid = EFI_GLOBAL_VARIABLE_GUID;
> > + efi_guid_t shim_guid = EFI_SHIM_LOCK_GUID;
> > + efi_status_t status;
> > + unsigned long size;
> > + u8 secboot, setupmode, moksbstate;
> > + u32 attr;
> > +
> > + size = sizeof(secboot);
> > + status = get_var(L"SecureBoot", &var_guid, NULL, &size, &secboot);
> > +
> > + if (status == EFI_NOT_FOUND)
> > + return efi_secureboot_mode_disabled;
> > + if (status != EFI_SUCCESS)
> > + return efi_secureboot_mode_unknown;
> > +
> > + size = sizeof(setupmode);
> > + status = get_var(L"SetupMode", &var_guid, NULL, &size, &setupmode);
> > +
> > + if (status != EFI_SUCCESS)
> > + return efi_secureboot_mode_unknown;
> > + if (secboot == 0 || setupmode == 1)
> > + return efi_secureboot_mode_disabled;
> > +
>
> So keep until here and move the rest back into the .c file
>
> > + /*
> > + * 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_var(L"MokSBState", &shim_guid, &attr, &size, &moksbstate);
> > + /* If it fails, we don't care why. Default to secure */
> > + if (status == EFI_SUCCESS && moksbstate == 1
> > + && !(attr & EFI_VARIABLE_RUNTIME_ACCESS))
> > + return efi_secureboot_mode_disabled;
> > +
> > + return efi_secureboot_mode_enabled;
> > +}
> >
> > #ifdef CONFIG_RESET_ATTACK_MITIGATION
> > void efi_enable_reset_attack_mitigation(void);
> > --
> > 2.28.0
> >
>
