* [PATCH v3 1/3] efi: generalize efi_get_secureboot
2020-10-30 6:08 [PATCH v3 0/3] add ima_arch support for ARM64 Chester Lin
@ 2020-10-30 6:08 ` Chester Lin
0 siblings, 0 replies; 11+ messages in thread
From: Chester Lin @ 2020-10-30 6:08 UTC (permalink / raw)
To: ardb, zohar, jmorris, serge, dmitry.kasatkin, catalin.marinas,
will, tglx, mingo, bp, hpa
Cc: linux-kernel, linux-arm-kernel, linux-efi, linux-integrity,
linux-security-module, x86, jlee, clin
Generalize the efi_get_secureboot() function so not only efistub but also
other subsystems can use it.
Signed-off-by: Chester Lin <clin@suse.com>
---
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
@@ -1,76 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0
-/*
- * 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>
- */
-#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);
-
- /* 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;
-}
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;
+
+ /*
+ * 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
^ permalink raw reply related [flat|nested] 11+ messages in thread
* [PATCH v3 1/3] efi: generalize efi_get_secureboot
@ 2020-10-30 6:08 ` Chester Lin
0 siblings, 0 replies; 11+ messages in thread
From: Chester Lin @ 2020-10-30 6:08 UTC (permalink / raw)
To: ardb, zohar, jmorris, serge, dmitry.kasatkin, catalin.marinas,
will, tglx, mingo, bp, hpa
Cc: clin, linux-efi, x86, linux-kernel, jlee, linux-security-module,
linux-integrity, linux-arm-kernel
Generalize the efi_get_secureboot() function so not only efistub but also
other subsystems can use it.
Signed-off-by: Chester Lin <clin@suse.com>
---
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
@@ -1,76 +0,0 @@
-// SPDX-License-Identifier: GPL-2.0
-/*
- * 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>
- */
-#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);
-
- /* 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;
-}
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;
+
+ /*
+ * 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
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply related [flat|nested] 11+ messages in thread
* Re: [PATCH v3 1/3] efi: generalize efi_get_secureboot
2020-10-30 6:08 ` Chester Lin
@ 2020-10-30 11:51 ` Ard Biesheuvel
-1 siblings, 0 replies; 11+ messages in thread
From: Ard Biesheuvel @ 2020-10-30 11:51 UTC (permalink / raw)
To: Chester Lin
Cc: Mimi Zohar, James Morris, Serge E. Hallyn, Dmitry Kasatkin,
Catalin Marinas, Will Deacon, Thomas Gleixner, Ingo Molnar,
Borislav Petkov, H. Peter Anvin, Linux Kernel Mailing List,
Linux ARM, linux-efi, linux-integrity, linux-security-module,
X86 ML, Lee, Chun-Yi
Hello Chester,
Thanks again for looking into this.
On Fri, 30 Oct 2020 at 07:09, Chester Lin <clin@suse.com> wrote:
>
> Generalize the efi_get_secureboot() function so not only efistub but also
> other subsystems can use it.
>
> Signed-off-by: Chester Lin <clin@suse.com>
> ---
> 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@linaro.org
> - * Copyright (C) 2013 Red Hat, Inc.
> - * Mark Salter <msalter@redhat.com>
> - */
> -#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
first place)
> - /* 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.
> 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
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 1/3] efi: generalize efi_get_secureboot
@ 2020-10-30 11:51 ` Ard Biesheuvel
0 siblings, 0 replies; 11+ messages in thread
From: Ard Biesheuvel @ 2020-10-30 11:51 UTC (permalink / raw)
To: Chester Lin
Cc: X86 ML, linux-efi, Dmitry Kasatkin, H. Peter Anvin, James Morris,
Mimi Zohar, Linux Kernel Mailing List, Lee, Chun-Yi,
linux-security-module, Ingo Molnar, Borislav Petkov,
Catalin Marinas, Thomas Gleixner, Will Deacon, linux-integrity,
Linux ARM, Serge E. Hallyn
Hello Chester,
Thanks again for looking into this.
On Fri, 30 Oct 2020 at 07:09, Chester Lin <clin@suse.com> wrote:
>
> Generalize the efi_get_secureboot() function so not only efistub but also
> other subsystems can use it.
>
> Signed-off-by: Chester Lin <clin@suse.com>
> ---
> 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@linaro.org
> - * Copyright (C) 2013 Red Hat, Inc.
> - * Mark Salter <msalter@redhat.com>
> - */
> -#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
first place)
> - /* 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.
> 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
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 1/3] efi: generalize efi_get_secureboot
2020-10-30 6:08 ` Chester Lin
(?)
(?)
@ 2020-10-31 2:39 ` kernel test robot
-1 siblings, 0 replies; 11+ messages in thread
From: kernel test robot @ 2020-10-31 2:39 UTC (permalink / raw)
To: kbuild-all
[-- Attachment #1: Type: text/plain, Size: 4544 bytes --]
Hi Chester,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on efi/next]
[also build test WARNING on arm64/for-next/core integrity/next-integrity v5.10-rc1 next-20201030]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Chester-Lin/add-ima_arch-support-for-ARM64/20201030-141043
base: https://git.kernel.org/pub/scm/linux/kernel/git/efi/efi.git next
config: i386-randconfig-c001-20201030 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
reproduce (this is a W=1 build):
# https://github.com/0day-ci/linux/commit/945001ead079043268e8ad1b9d1df9bd5cabf020
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Chester-Lin/add-ima_arch-support-for-ARM64/20201030-141043
git checkout 945001ead079043268e8ad1b9d1df9bd5cabf020
# save the attached .config to linux build tree
make W=1 ARCH=i386
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
In file included from arch/x86/boot/compressed/acpi.c:8:
include/linux/efi.h: In function 'efi_get_secureboot':
>> include/linux/efi.h:1103:19: warning: passing argument 1 of 'get_var' from incompatible pointer type [-Wincompatible-pointer-types]
1103 | status = get_var(L"SecureBoot", &var_guid, NULL, &size, &secboot);
| ^~~~~~~~~~~~~
| |
| long int *
include/linux/efi.h:1103:19: note: expected 'efi_char16_t *' {aka 'short unsigned int *'} but argument is of type 'long int *'
include/linux/efi.h:1111:19: warning: passing argument 1 of 'get_var' from incompatible pointer type [-Wincompatible-pointer-types]
1111 | status = get_var(L"SetupMode", &var_guid, NULL, &size, &setupmode);
| ^~~~~~~~~~~~
| |
| long int *
include/linux/efi.h:1111:19: note: expected 'efi_char16_t *' {aka 'short unsigned int *'} but argument is of type 'long int *'
include/linux/efi.h:1124:19: warning: passing argument 1 of 'get_var' from incompatible pointer type [-Wincompatible-pointer-types]
1124 | status = get_var(L"MokSBState", &shim_guid, &attr, &size, &moksbstate);
| ^~~~~~~~~~~~~
| |
| long int *
include/linux/efi.h:1124:19: note: expected 'efi_char16_t *' {aka 'short unsigned int *'} but argument is of type 'long int *'
vim +/get_var +1103 include/linux/efi.h
1092
1093 static inline enum efi_secureboot_mode efi_get_secureboot(efi_get_variable_t *get_var)
1094 {
1095 efi_guid_t var_guid = EFI_GLOBAL_VARIABLE_GUID;
1096 efi_guid_t shim_guid = EFI_SHIM_LOCK_GUID;
1097 efi_status_t status;
1098 unsigned long size;
1099 u8 secboot, setupmode, moksbstate;
1100 u32 attr;
1101
1102 size = sizeof(secboot);
> 1103 status = get_var(L"SecureBoot", &var_guid, NULL, &size, &secboot);
1104
1105 if (status == EFI_NOT_FOUND)
1106 return efi_secureboot_mode_disabled;
1107 if (status != EFI_SUCCESS)
1108 return efi_secureboot_mode_unknown;
1109
1110 size = sizeof(setupmode);
1111 status = get_var(L"SetupMode", &var_guid, NULL, &size, &setupmode);
1112
1113 if (status != EFI_SUCCESS)
1114 return efi_secureboot_mode_unknown;
1115 if (secboot == 0 || setupmode == 1)
1116 return efi_secureboot_mode_disabled;
1117
1118 /*
1119 * See if a user has put the shim into insecure mode. If so, and if the
1120 * variable doesn't have the runtime attribute set, we might as well
1121 * honor that.
1122 */
1123 size = sizeof(moksbstate);
1124 status = get_var(L"MokSBState", &shim_guid, &attr, &size, &moksbstate);
1125 /* If it fails, we don't care why. Default to secure */
1126 if (status == EFI_SUCCESS && moksbstate == 1
1127 && !(attr & EFI_VARIABLE_RUNTIME_ACCESS))
1128 return efi_secureboot_mode_disabled;
1129
1130 return efi_secureboot_mode_enabled;
1131 }
1132
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 27122 bytes --]
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 1/3] efi: generalize efi_get_secureboot
2020-10-30 11:51 ` Ard Biesheuvel
@ 2020-11-02 5:30 ` Chester Lin
-1 siblings, 0 replies; 11+ messages in thread
From: Chester Lin @ 2020-11-02 5:30 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: Mimi Zohar, James Morris, Serge E. Hallyn, Dmitry Kasatkin,
Catalin Marinas, Will Deacon, Thomas Gleixner, Ingo Molnar,
Borislav Petkov, H. Peter Anvin, Linux Kernel Mailing List,
Linux ARM, linux-efi, linux-integrity, linux-security-module,
X86 ML, Lee, Chun-Yi
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@suse.com> wrote:
> >
> > Generalize the efi_get_secureboot() function so not only efistub but also
> > other subsystems can use it.
> >
> > Signed-off-by: Chester Lin <clin@suse.com>
> > ---
> > 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@linaro.org
> > - * Copyright (C) 2013 Red Hat, Inc.
> > - * Mark Salter <msalter@redhat.com>
> > - */
> > -#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
> >
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 1/3] efi: generalize efi_get_secureboot
@ 2020-11-02 5:30 ` Chester Lin
0 siblings, 0 replies; 11+ messages in thread
From: Chester Lin @ 2020-11-02 5:30 UTC (permalink / raw)
To: Ard Biesheuvel
Cc: X86 ML, linux-efi, Dmitry Kasatkin, H. Peter Anvin, James Morris,
Mimi Zohar, Linux Kernel Mailing List, Lee, Chun-Yi,
linux-security-module, Ingo Molnar, Borislav Petkov,
Catalin Marinas, Thomas Gleixner, Will Deacon, linux-integrity,
Linux ARM, Serge E. Hallyn
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@suse.com> wrote:
> >
> > Generalize the efi_get_secureboot() function so not only efistub but also
> > other subsystems can use it.
> >
> > Signed-off-by: Chester Lin <clin@suse.com>
> > ---
> > 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@linaro.org
> > - * Copyright (C) 2013 Red Hat, Inc.
> > - * Mark Salter <msalter@redhat.com>
> > - */
> > -#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
> >
>
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 1/3] efi: generalize efi_get_secureboot
2020-10-30 6:08 ` Chester Lin
` (2 preceding siblings ...)
(?)
@ 2020-11-02 13:03 ` kernel test robot
-1 siblings, 0 replies; 11+ messages in thread
From: kernel test robot @ 2020-11-02 13:03 UTC (permalink / raw)
To: kbuild-all
[-- Attachment #1: Type: text/plain, Size: 5691 bytes --]
Hi Chester,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on efi/next]
[also build test WARNING on arm64/for-next/core integrity/next-integrity v5.10-rc2 next-20201102]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Chester-Lin/add-ima_arch-support-for-ARM64/20201030-141043
base: https://git.kernel.org/pub/scm/linux/kernel/git/efi/efi.git next
config: i386-allyesconfig (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
reproduce (this is a W=1 build):
# https://github.com/0day-ci/linux/commit/945001ead079043268e8ad1b9d1df9bd5cabf020
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Chester-Lin/add-ima_arch-support-for-ARM64/20201030-141043
git checkout 945001ead079043268e8ad1b9d1df9bd5cabf020
# save the attached .config to linux build tree
make W=1 ARCH=i386
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
All warnings (new ones prefixed by >>):
drivers/firmware/efi/libstub/tpm.c: In function 'efi_enable_reset_attack_mitigation':
>> drivers/firmware/efi/libstub/tpm.c:36:23: warning: passing argument 1 of 'get_efi_var' discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
36 | status = get_efi_var(efi_MemoryOverWriteRequest_name, &var_guid,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from drivers/firmware/efi/libstub/tpm.c:14:
drivers/firmware/efi/libstub/efistub.h:108:40: note: expected 'efi_char16_t *' {aka 'short unsigned int *'} but argument is of type 'const efi_char16_t *' {aka 'const short unsigned int *'}
108 | efi_status_t get_efi_var(efi_char16_t *name, efi_guid_t *vendor, u32 *attr,
| ~~~~~~~~~~~~~~^~~~
>> drivers/firmware/efi/libstub/tpm.c:42:14: warning: passing argument 1 of 'set_efi_var' discards 'const' qualifier from pointer target type [-Wdiscarded-qualifiers]
42 | set_efi_var(efi_MemoryOverWriteRequest_name, &var_guid,
| ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
In file included from drivers/firmware/efi/libstub/tpm.c:14:
drivers/firmware/efi/libstub/efistub.h:115:40: note: expected 'efi_char16_t *' {aka 'short unsigned int *'} but argument is of type 'const efi_char16_t *' {aka 'const short unsigned int *'}
115 | efi_status_t set_efi_var(efi_char16_t *name, efi_guid_t *vendor, u32 attr,
| ~~~~~~~~~~~~~~^~~~
vim +36 drivers/firmware/efi/libstub/tpm.c
ccc829ba3624beb Matthew Garrett 2017-08-25 15
33b6d03469b2206 Thiebaud Weksteen 2017-09-20 16 #ifdef CONFIG_RESET_ATTACK_MITIGATION
36b649760e94968 Ard Biesheuvel 2018-03-12 17 static const efi_char16_t efi_MemoryOverWriteRequest_name[] =
36b649760e94968 Ard Biesheuvel 2018-03-12 18 L"MemoryOverwriteRequestControl";
ccc829ba3624beb Matthew Garrett 2017-08-25 19
ccc829ba3624beb Matthew Garrett 2017-08-25 20 #define MEMORY_ONLY_RESET_CONTROL_GUID \
ccc829ba3624beb Matthew Garrett 2017-08-25 21 EFI_GUID(0xe20939be, 0x32d4, 0x41be, 0xa1, 0x50, 0x89, 0x7f, 0x85, 0xd4, 0x98, 0x29)
ccc829ba3624beb Matthew Garrett 2017-08-25 22
ccc829ba3624beb Matthew Garrett 2017-08-25 23 /*
ccc829ba3624beb Matthew Garrett 2017-08-25 24 * Enable reboot attack mitigation. This requests that the firmware clear the
ccc829ba3624beb Matthew Garrett 2017-08-25 25 * RAM on next reboot before proceeding with boot, ensuring that any secrets
ccc829ba3624beb Matthew Garrett 2017-08-25 26 * are cleared. If userland has ensured that all secrets have been removed
ccc829ba3624beb Matthew Garrett 2017-08-25 27 * from RAM before reboot it can simply reset this variable.
ccc829ba3624beb Matthew Garrett 2017-08-25 28 */
cd33a5c1d53e43b Ard Biesheuvel 2019-12-24 29 void efi_enable_reset_attack_mitigation(void)
ccc829ba3624beb Matthew Garrett 2017-08-25 30 {
ccc829ba3624beb Matthew Garrett 2017-08-25 31 u8 val = 1;
ccc829ba3624beb Matthew Garrett 2017-08-25 32 efi_guid_t var_guid = MEMORY_ONLY_RESET_CONTROL_GUID;
ccc829ba3624beb Matthew Garrett 2017-08-25 33 efi_status_t status;
ccc829ba3624beb Matthew Garrett 2017-08-25 34 unsigned long datasize = 0;
ccc829ba3624beb Matthew Garrett 2017-08-25 35
ccc829ba3624beb Matthew Garrett 2017-08-25 @36 status = get_efi_var(efi_MemoryOverWriteRequest_name, &var_guid,
ccc829ba3624beb Matthew Garrett 2017-08-25 37 NULL, &datasize, NULL);
ccc829ba3624beb Matthew Garrett 2017-08-25 38
ccc829ba3624beb Matthew Garrett 2017-08-25 39 if (status == EFI_NOT_FOUND)
ccc829ba3624beb Matthew Garrett 2017-08-25 40 return;
ccc829ba3624beb Matthew Garrett 2017-08-25 41
ccc829ba3624beb Matthew Garrett 2017-08-25 @42 set_efi_var(efi_MemoryOverWriteRequest_name, &var_guid,
ccc829ba3624beb Matthew Garrett 2017-08-25 43 EFI_VARIABLE_NON_VOLATILE |
ccc829ba3624beb Matthew Garrett 2017-08-25 44 EFI_VARIABLE_BOOTSERVICE_ACCESS |
ccc829ba3624beb Matthew Garrett 2017-08-25 45 EFI_VARIABLE_RUNTIME_ACCESS, sizeof(val), &val);
ccc829ba3624beb Matthew Garrett 2017-08-25 46 }
33b6d03469b2206 Thiebaud Weksteen 2017-09-20 47
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 75915 bytes --]
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 1/3] efi: generalize efi_get_secureboot
2020-10-30 6:08 ` Chester Lin
` (3 preceding siblings ...)
(?)
@ 2020-11-04 4:20 ` kernel test robot
-1 siblings, 0 replies; 11+ messages in thread
From: kernel test robot @ 2020-11-04 4:20 UTC (permalink / raw)
To: kbuild-all
[-- Attachment #1: Type: text/plain, Size: 5084 bytes --]
Hi Chester,
Thank you for the patch! Perhaps something to improve:
[auto build test WARNING on efi/next]
[also build test WARNING on arm64/for-next/core integrity/next-integrity v5.10-rc2 next-20201103]
[If your patch is applied to the wrong git tree, kindly drop us a note.
And when submitting patch, we suggest to use '--base' as documented in
https://git-scm.com/docs/git-format-patch]
url: https://github.com/0day-ci/linux/commits/Chester-Lin/add-ima_arch-support-for-ARM64/20201030-141043
base: https://git.kernel.org/pub/scm/linux/kernel/git/efi/efi.git next
config: i386-randconfig-s031-20201103 (attached as .config)
compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
reproduce:
# apt-get install sparse
# sparse version: v0.6.3-76-gf680124b-dirty
# https://github.com/0day-ci/linux/commit/945001ead079043268e8ad1b9d1df9bd5cabf020
git remote add linux-review https://github.com/0day-ci/linux
git fetch --no-tags linux-review Chester-Lin/add-ima_arch-support-for-ARM64/20201030-141043
git checkout 945001ead079043268e8ad1b9d1df9bd5cabf020
# save the attached .config to linux build tree
make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=i386
If you fix the issue, kindly add following tag as appropriate
Reported-by: kernel test robot <lkp@intel.com>
"sparse warnings: (new ones prefixed by >>)"
>> drivers/firmware/efi/libstub/tpm.c:36:30: sparse: sparse: incorrect type in argument 1 (different modifiers) @@ expected unsigned short [usertype] *name @@ got unsigned short const * @@
>> drivers/firmware/efi/libstub/tpm.c:36:30: sparse: expected unsigned short [usertype] *name
>> drivers/firmware/efi/libstub/tpm.c:36:30: sparse: got unsigned short const *
drivers/firmware/efi/libstub/tpm.c:42:21: sparse: sparse: incorrect type in argument 1 (different modifiers) @@ expected unsigned short [usertype] *name @@ got unsigned short const * @@
drivers/firmware/efi/libstub/tpm.c:42:21: sparse: expected unsigned short [usertype] *name
drivers/firmware/efi/libstub/tpm.c:42:21: sparse: got unsigned short const *
vim +36 drivers/firmware/efi/libstub/tpm.c
ccc829ba3624beb Matthew Garrett 2017-08-25 15
33b6d03469b2206 Thiebaud Weksteen 2017-09-20 16 #ifdef CONFIG_RESET_ATTACK_MITIGATION
36b649760e94968 Ard Biesheuvel 2018-03-12 17 static const efi_char16_t efi_MemoryOverWriteRequest_name[] =
36b649760e94968 Ard Biesheuvel 2018-03-12 18 L"MemoryOverwriteRequestControl";
ccc829ba3624beb Matthew Garrett 2017-08-25 19
ccc829ba3624beb Matthew Garrett 2017-08-25 20 #define MEMORY_ONLY_RESET_CONTROL_GUID \
ccc829ba3624beb Matthew Garrett 2017-08-25 21 EFI_GUID(0xe20939be, 0x32d4, 0x41be, 0xa1, 0x50, 0x89, 0x7f, 0x85, 0xd4, 0x98, 0x29)
ccc829ba3624beb Matthew Garrett 2017-08-25 22
ccc829ba3624beb Matthew Garrett 2017-08-25 23 /*
ccc829ba3624beb Matthew Garrett 2017-08-25 24 * Enable reboot attack mitigation. This requests that the firmware clear the
ccc829ba3624beb Matthew Garrett 2017-08-25 25 * RAM on next reboot before proceeding with boot, ensuring that any secrets
ccc829ba3624beb Matthew Garrett 2017-08-25 26 * are cleared. If userland has ensured that all secrets have been removed
ccc829ba3624beb Matthew Garrett 2017-08-25 27 * from RAM before reboot it can simply reset this variable.
ccc829ba3624beb Matthew Garrett 2017-08-25 28 */
cd33a5c1d53e43b Ard Biesheuvel 2019-12-24 29 void efi_enable_reset_attack_mitigation(void)
ccc829ba3624beb Matthew Garrett 2017-08-25 30 {
ccc829ba3624beb Matthew Garrett 2017-08-25 31 u8 val = 1;
ccc829ba3624beb Matthew Garrett 2017-08-25 32 efi_guid_t var_guid = MEMORY_ONLY_RESET_CONTROL_GUID;
ccc829ba3624beb Matthew Garrett 2017-08-25 33 efi_status_t status;
ccc829ba3624beb Matthew Garrett 2017-08-25 34 unsigned long datasize = 0;
ccc829ba3624beb Matthew Garrett 2017-08-25 35
ccc829ba3624beb Matthew Garrett 2017-08-25 @36 status = get_efi_var(efi_MemoryOverWriteRequest_name, &var_guid,
ccc829ba3624beb Matthew Garrett 2017-08-25 37 NULL, &datasize, NULL);
ccc829ba3624beb Matthew Garrett 2017-08-25 38
ccc829ba3624beb Matthew Garrett 2017-08-25 39 if (status == EFI_NOT_FOUND)
ccc829ba3624beb Matthew Garrett 2017-08-25 40 return;
ccc829ba3624beb Matthew Garrett 2017-08-25 41
ccc829ba3624beb Matthew Garrett 2017-08-25 42 set_efi_var(efi_MemoryOverWriteRequest_name, &var_guid,
ccc829ba3624beb Matthew Garrett 2017-08-25 43 EFI_VARIABLE_NON_VOLATILE |
ccc829ba3624beb Matthew Garrett 2017-08-25 44 EFI_VARIABLE_BOOTSERVICE_ACCESS |
ccc829ba3624beb Matthew Garrett 2017-08-25 45 EFI_VARIABLE_RUNTIME_ACCESS, sizeof(val), &val);
ccc829ba3624beb Matthew Garrett 2017-08-25 46 }
33b6d03469b2206 Thiebaud Weksteen 2017-09-20 47
---
0-DAY CI Kernel Test Service, Intel Corporation
https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
[-- Attachment #2: config.gz --]
[-- Type: application/gzip, Size: 32683 bytes --]
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 1/3] efi: generalize efi_get_secureboot
[not found] <975b1b2c05fdbd73f25b09b85d6a23370e557536.camel@linux.ibm.com>
@ 2020-11-04 18:52 ` Ard Biesheuvel
2020-11-05 7:10 ` Philip Li
0 siblings, 1 reply; 11+ messages in thread
From: Ard Biesheuvel @ 2020-11-04 18:52 UTC (permalink / raw)
To: kbuild-all
[-- Attachment #1: Type: text/plain, Size: 5811 bytes --]
On Wed, 4 Nov 2020 at 19:14, Mimi Zohar <zohar@linux.ibm.com> wrote:
>
> On Wed, 2020-11-04 at 12:20 +0800, kernel test robot wrote:
> > Hi Chester,
> >
> > Thank you for the patch! Perhaps something to improve:
> >
> > [auto build test WARNING on efi/next]
> > [also build test WARNING on arm64/for-next/core integrity/next-integrity v5.10-rc2 next-20201103]
> > [If your patch is applied to the wrong git tree, kindly drop us a note.
> > And when submitting patch, we suggest to use '--base' as documented in
> > https://git-scm.com/docs/git-format-patch]
> >
> > url: https://github.com/0day-ci/linux/commits/Chester-Lin/add-ima_arch-support-for-ARM64/20201030-141043
> > base: https://git.kernel.org/pub/scm/linux/kernel/git/efi/efi.git next
> > config: i386-randconfig-s031-20201103 (attached as .config)
> > compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
> > reproduce:
> > # apt-get install sparse
> > # sparse version: v0.6.3-76-gf680124b-dirty
> > # https://github.com/0day-ci/linux/commit/945001ead079043268e8ad1b9d1df9bd5cabf020
> > git remote add linux-review https://github.com/0day-ci/linux
> > git fetch --no-tags linux-review Chester-Lin/add-ima_arch-support-for-ARM64/20201030-141043
> > git checkout 945001ead079043268e8ad1b9d1df9bd5cabf020
> > # save the attached .config to linux build tree
> > make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=i386
> >
> > If you fix the issue, kindly add following tag as appropriate
> > Reported-by: kernel test robot <lkp@intel.com>
> >
> >
> > "sparse warnings: (new ones prefixed by >>)"
> > >> drivers/firmware/efi/libstub/tpm.c:36:30: sparse: sparse: incorrect type in argument 1 (different modifiers) @@ expected unsigned short [usertype] *name @@ got unsigned short const * @@
> > >> drivers/firmware/efi/libstub/tpm.c:36:30: sparse: expected unsigned short [usertype] *name
> > >> drivers/firmware/efi/libstub/tpm.c:36:30: sparse: got unsigned short const *
> > drivers/firmware/efi/libstub/tpm.c:42:21: sparse: sparse: incorrect type in argument 1 (different modifiers) @@ expected unsigned short [usertype] *name @@ got unsigned short const * @@
> > drivers/firmware/efi/libstub/tpm.c:42:21: sparse: expected unsigned short [usertype] *name
> > drivers/firmware/efi/libstub/tpm.c:42:21: sparse: got unsigned short const *
> >
>
> This seems to be the result of commit 36b649760e94 ("efi: Use string
> literals for efi_char16_t variable initializers"), not this patch.
>
Indeed. tpm.c is not even touched by the patch in question afaict.
>
> > vim +36 drivers/firmware/efi/libstub/tpm.c
> >
> > ccc829ba3624beb Matthew Garrett 2017-08-25 15
> > 33b6d03469b2206 Thiebaud Weksteen 2017-09-20 16 #ifdef CONFIG_RESET_ATTACK_MITIGATION
> > 36b649760e94968 Ard Biesheuvel 2018-03-12 17 static const efi_char16_t efi_MemoryOverWriteRequest_name[] =
> > 36b649760e94968 Ard Biesheuvel 2018-03-12 18 L"MemoryOverwriteRequestControl";
> > ccc829ba3624beb Matthew Garrett 2017-08-25 19
> > ccc829ba3624beb Matthew Garrett 2017-08-25 20 #define MEMORY_ONLY_RESET_CONTROL_GUID \
> > ccc829ba3624beb Matthew Garrett 2017-08-25 21 EFI_GUID(0xe20939be, 0x32d4, 0x41be, 0xa1, 0x50, 0x89, 0x7f, 0x85, 0xd4, 0x98, 0x29)
> > ccc829ba3624beb Matthew Garrett 2017-08-25 22
> > ccc829ba3624beb Matthew Garrett 2017-08-25 23 /*
> > ccc829ba3624beb Matthew Garrett 2017-08-25 24 * Enable reboot attack mitigation. This requests that the firmware clear the
> > ccc829ba3624beb Matthew Garrett 2017-08-25 25 * RAM on next reboot before proceeding with boot, ensuring that any secrets
> > ccc829ba3624beb Matthew Garrett 2017-08-25 26 * are cleared. If userland has ensured that all secrets have been removed
> > ccc829ba3624beb Matthew Garrett 2017-08-25 27 * from RAM before reboot it can simply reset this variable.
> > ccc829ba3624beb Matthew Garrett 2017-08-25 28 */
> > cd33a5c1d53e43b Ard Biesheuvel 2019-12-24 29 void efi_enable_reset_attack_mitigation(void)
> > ccc829ba3624beb Matthew Garrett 2017-08-25 30 {
> > ccc829ba3624beb Matthew Garrett 2017-08-25 31 u8 val = 1;
> > ccc829ba3624beb Matthew Garrett 2017-08-25 32 efi_guid_t var_guid = MEMORY_ONLY_RESET_CONTROL_GUID;
> > ccc829ba3624beb Matthew Garrett 2017-08-25 33 efi_status_t status;
> > ccc829ba3624beb Matthew Garrett 2017-08-25 34 unsigned long datasize = 0;
> > ccc829ba3624beb Matthew Garrett 2017-08-25 35
> > ccc829ba3624beb Matthew Garrett 2017-08-25 @36 status = get_efi_var(efi_MemoryOverWriteRequest_name, &var_guid,
> > ccc829ba3624beb Matthew Garrett 2017-08-25 37 NULL, &datasize, NULL);
> > ccc829ba3624beb Matthew Garrett 2017-08-25 38
> > ccc829ba3624beb Matthew Garrett 2017-08-25 39 if (status == EFI_NOT_FOUND)
> > ccc829ba3624beb Matthew Garrett 2017-08-25 40 return;
> > ccc829ba3624beb Matthew Garrett 2017-08-25 41
> > ccc829ba3624beb Matthew Garrett 2017-08-25 42 set_efi_var(efi_MemoryOverWriteRequest_name, &var_guid,
> > ccc829ba3624beb Matthew Garrett 2017-08-25 43 EFI_VARIABLE_NON_VOLATILE |
> > ccc829ba3624beb Matthew Garrett 2017-08-25 44 EFI_VARIABLE_BOOTSERVICE_ACCESS |
> > ccc829ba3624beb Matthew Garrett 2017-08-25 45 EFI_VARIABLE_RUNTIME_ACCESS, sizeof(val), &val);
> > ccc829ba3624beb Matthew Garrett 2017-08-25 46 }
> > 33b6d03469b2206 Thiebaud Weksteen 2017-09-20 47
> >
> > ---
> > 0-DAY CI Kernel Test Service, Intel Corporation
> > https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
>
>
^ permalink raw reply [flat|nested] 11+ messages in thread
* Re: [PATCH v3 1/3] efi: generalize efi_get_secureboot
2020-11-04 18:52 ` [PATCH v3 1/3] efi: generalize efi_get_secureboot Ard Biesheuvel
@ 2020-11-05 7:10 ` Philip Li
0 siblings, 0 replies; 11+ messages in thread
From: Philip Li @ 2020-11-05 7:10 UTC (permalink / raw)
To: kbuild-all
[-- Attachment #1: Type: text/plain, Size: 6302 bytes --]
On Wed, Nov 04, 2020 at 07:52:19PM +0100, Ard Biesheuvel wrote:
> On Wed, 4 Nov 2020 at 19:14, Mimi Zohar <zohar@linux.ibm.com> wrote:
> >
> > On Wed, 2020-11-04 at 12:20 +0800, kernel test robot wrote:
> > > Hi Chester,
> > >
> > > Thank you for the patch! Perhaps something to improve:
> > >
> > > [auto build test WARNING on efi/next]
> > > [also build test WARNING on arm64/for-next/core integrity/next-integrity v5.10-rc2 next-20201103]
> > > [If your patch is applied to the wrong git tree, kindly drop us a note.
> > > And when submitting patch, we suggest to use '--base' as documented in
> > > https://git-scm.com/docs/git-format-patch]
> > >
> > > url: https://github.com/0day-ci/linux/commits/Chester-Lin/add-ima_arch-support-for-ARM64/20201030-141043
> > > base: https://git.kernel.org/pub/scm/linux/kernel/git/efi/efi.git next
> > > config: i386-randconfig-s031-20201103 (attached as .config)
> > > compiler: gcc-9 (Debian 9.3.0-15) 9.3.0
> > > reproduce:
> > > # apt-get install sparse
> > > # sparse version: v0.6.3-76-gf680124b-dirty
> > > # https://github.com/0day-ci/linux/commit/945001ead079043268e8ad1b9d1df9bd5cabf020
> > > git remote add linux-review https://github.com/0day-ci/linux
> > > git fetch --no-tags linux-review Chester-Lin/add-ima_arch-support-for-ARM64/20201030-141043
> > > git checkout 945001ead079043268e8ad1b9d1df9bd5cabf020
> > > # save the attached .config to linux build tree
> > > make W=1 C=1 CF='-fdiagnostic-prefix -D__CHECK_ENDIAN__' ARCH=i386
> > >
> > > If you fix the issue, kindly add following tag as appropriate
> > > Reported-by: kernel test robot <lkp@intel.com>
> > >
> > >
> > > "sparse warnings: (new ones prefixed by >>)"
> > > >> drivers/firmware/efi/libstub/tpm.c:36:30: sparse: sparse: incorrect type in argument 1 (different modifiers) @@ expected unsigned short [usertype] *name @@ got unsigned short const * @@
> > > >> drivers/firmware/efi/libstub/tpm.c:36:30: sparse: expected unsigned short [usertype] *name
> > > >> drivers/firmware/efi/libstub/tpm.c:36:30: sparse: got unsigned short const *
> > > drivers/firmware/efi/libstub/tpm.c:42:21: sparse: sparse: incorrect type in argument 1 (different modifiers) @@ expected unsigned short [usertype] *name @@ got unsigned short const * @@
> > > drivers/firmware/efi/libstub/tpm.c:42:21: sparse: expected unsigned short [usertype] *name
> > > drivers/firmware/efi/libstub/tpm.c:42:21: sparse: got unsigned short const *
> > >
> >
> > This seems to be the result of commit 36b649760e94 ("efi: Use string
> > literals for efi_char16_t variable initializers"), not this patch.
> >
>
> Indeed. tpm.c is not even touched by the patch in question afaict.
sorry for false positive, we will look into it to resolve ths issue.
>
>
>
> >
> > > vim +36 drivers/firmware/efi/libstub/tpm.c
> > >
> > > ccc829ba3624beb Matthew Garrett 2017-08-25 15
> > > 33b6d03469b2206 Thiebaud Weksteen 2017-09-20 16 #ifdef CONFIG_RESET_ATTACK_MITIGATION
> > > 36b649760e94968 Ard Biesheuvel 2018-03-12 17 static const efi_char16_t efi_MemoryOverWriteRequest_name[] =
> > > 36b649760e94968 Ard Biesheuvel 2018-03-12 18 L"MemoryOverwriteRequestControl";
> > > ccc829ba3624beb Matthew Garrett 2017-08-25 19
> > > ccc829ba3624beb Matthew Garrett 2017-08-25 20 #define MEMORY_ONLY_RESET_CONTROL_GUID \
> > > ccc829ba3624beb Matthew Garrett 2017-08-25 21 EFI_GUID(0xe20939be, 0x32d4, 0x41be, 0xa1, 0x50, 0x89, 0x7f, 0x85, 0xd4, 0x98, 0x29)
> > > ccc829ba3624beb Matthew Garrett 2017-08-25 22
> > > ccc829ba3624beb Matthew Garrett 2017-08-25 23 /*
> > > ccc829ba3624beb Matthew Garrett 2017-08-25 24 * Enable reboot attack mitigation. This requests that the firmware clear the
> > > ccc829ba3624beb Matthew Garrett 2017-08-25 25 * RAM on next reboot before proceeding with boot, ensuring that any secrets
> > > ccc829ba3624beb Matthew Garrett 2017-08-25 26 * are cleared. If userland has ensured that all secrets have been removed
> > > ccc829ba3624beb Matthew Garrett 2017-08-25 27 * from RAM before reboot it can simply reset this variable.
> > > ccc829ba3624beb Matthew Garrett 2017-08-25 28 */
> > > cd33a5c1d53e43b Ard Biesheuvel 2019-12-24 29 void efi_enable_reset_attack_mitigation(void)
> > > ccc829ba3624beb Matthew Garrett 2017-08-25 30 {
> > > ccc829ba3624beb Matthew Garrett 2017-08-25 31 u8 val = 1;
> > > ccc829ba3624beb Matthew Garrett 2017-08-25 32 efi_guid_t var_guid = MEMORY_ONLY_RESET_CONTROL_GUID;
> > > ccc829ba3624beb Matthew Garrett 2017-08-25 33 efi_status_t status;
> > > ccc829ba3624beb Matthew Garrett 2017-08-25 34 unsigned long datasize = 0;
> > > ccc829ba3624beb Matthew Garrett 2017-08-25 35
> > > ccc829ba3624beb Matthew Garrett 2017-08-25 @36 status = get_efi_var(efi_MemoryOverWriteRequest_name, &var_guid,
> > > ccc829ba3624beb Matthew Garrett 2017-08-25 37 NULL, &datasize, NULL);
> > > ccc829ba3624beb Matthew Garrett 2017-08-25 38
> > > ccc829ba3624beb Matthew Garrett 2017-08-25 39 if (status == EFI_NOT_FOUND)
> > > ccc829ba3624beb Matthew Garrett 2017-08-25 40 return;
> > > ccc829ba3624beb Matthew Garrett 2017-08-25 41
> > > ccc829ba3624beb Matthew Garrett 2017-08-25 42 set_efi_var(efi_MemoryOverWriteRequest_name, &var_guid,
> > > ccc829ba3624beb Matthew Garrett 2017-08-25 43 EFI_VARIABLE_NON_VOLATILE |
> > > ccc829ba3624beb Matthew Garrett 2017-08-25 44 EFI_VARIABLE_BOOTSERVICE_ACCESS |
> > > ccc829ba3624beb Matthew Garrett 2017-08-25 45 EFI_VARIABLE_RUNTIME_ACCESS, sizeof(val), &val);
> > > ccc829ba3624beb Matthew Garrett 2017-08-25 46 }
> > > 33b6d03469b2206 Thiebaud Weksteen 2017-09-20 47
> > >
> > > ---
> > > 0-DAY CI Kernel Test Service, Intel Corporation
> > > https://lists.01.org/hyperkitty/list/kbuild-all(a)lists.01.org
> >
> >
> _______________________________________________
> kbuild-all mailing list -- kbuild-all(a)lists.01.org
> To unsubscribe send an email to kbuild-all-leave(a)lists.01.org
^ permalink raw reply [flat|nested] 11+ messages in thread
end of thread, other threads:[~2020-11-05 7:10 UTC | newest]
Thread overview: 11+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
[not found] <975b1b2c05fdbd73f25b09b85d6a23370e557536.camel@linux.ibm.com>
2020-11-04 18:52 ` [PATCH v3 1/3] efi: generalize efi_get_secureboot Ard Biesheuvel
2020-11-05 7:10 ` Philip Li
2020-10-30 6:08 [PATCH v3 0/3] add ima_arch support for ARM64 Chester Lin
2020-10-30 6:08 ` [PATCH v3 1/3] efi: generalize efi_get_secureboot Chester Lin
2020-10-30 6:08 ` Chester Lin
2020-10-30 11:51 ` Ard Biesheuvel
2020-10-30 11:51 ` Ard Biesheuvel
2020-11-02 5:30 ` Chester Lin
2020-11-02 5:30 ` Chester Lin
2020-10-31 2:39 ` kernel test robot
2020-11-02 13:03 ` kernel test robot
2020-11-04 4:20 ` kernel test robot
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.