Hi,
On Tue, Nov 08, 2022 at 04:15:09PM +0100, Ard Biesheuvel wrote:
> Alexandru reports that his Ampere Altra machine, whose buggy firmware
> triggers a synchronous exception in its implementation of SetTime() when
> called without SetVirtualAddressMap() having been called first, doesn't
> quite recover from this, and starts spewing error messages into the log
> that are unrelated to the buggy runtime service.
>
> The driver in question is the EFI RTC driver, which should be fixed in
> any case, as flooding the log like that (or doing any logging to the
> kernel log at all on something whuch is not a severe issue) is not ok.
>
> However, in this particular case, it would be beneficial for both
> ordinary use as well as diagnostics regarding broken firmware if we only
> prevent the broken runtime service from being called again, and permit
> others (such as GetTime() which triggers the logging or the variable
> services) from being used as normal.
>
> So wire up the existing efi.runtime_supported_mask, and clear the
> service's bit in the mask if the generic runtime service wrapper
> observes a return value of EFI_ABORTED, which only happens if a service
> call is aborted due to an exception. (EFI_ABORTED is not documented as a
> valid error code for any of the EFI runtime services).
With a kernel built from v6.1-rc4, when doing efibootmgr after the EFI panic
happens (so with runtime services disabled), this is what I get:
# efibootmgr
Skipping unreadable variable "Boot0001": Interrupted system call
Skipping unreadable variable "Boot0002": Interrupted system call
show_order(): Interrupted system call
And dmesg shows:
[ 55.941312] efi: EFI Runtime Services are disabled!
With this patch on top of v6.1-rc4:
# efibootmgr
Skipping unreadable variable "Boot0001": Invalid argument
Skipping unreadable variable "Boot0002": Invalid argument
show_order(): Invalid argument
Same thing happens if I cat the Boot001 efivars file. Nothing is printed
on dmesg.
Changed efi_call_rts() to print the return value, status is
0x8000_0000_0000_000f (or 15 in decimal if casted into an int). Tried to
debug further, but I'm not familiar with all the structs and what they
represent (for example, efi_call_virt(get_variable, args) calls
efi_call_virt_pointer(efi.runtime, get_variable, args), does it end up as
__efi_rt_asm_wrapper((efi.runtime)->get_variable, "get_variable", args?) As
an aside, it would be really helpful to document the arguments for
__efi_rt_asm_wrapper. Pointers here how to debug further would be very
welcome.
Thanks,
Alex
>
> Cc: Alexandru Elisei <alexandru.elisei@xxxxxxx>
> Signed-off-by: Ard Biesheuvel <ardb@xxxxxxxxxx>
> ---
> arch/arm64/kernel/efi.c | 1 -
> arch/x86/platform/efi/quirks.c | 2 +-
> drivers/firmware/efi/runtime-wrappers.c | 6 ++++--
> include/linux/efi.h | 24 ++++++++++++------------
> 4 files changed, 17 insertions(+), 16 deletions(-)
>
> diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
> index ee53f2a0aa03f54a..a3b8852f2698d9f7 100644
> --- a/arch/arm64/kernel/efi.c
> +++ b/arch/arm64/kernel/efi.c
> @@ -153,7 +153,6 @@ asmlinkage efi_status_t __efi_rt_asm_recover(void);
> asmlinkage efi_status_t efi_handle_runtime_exception(const char *f)
> {
> pr_err(FW_BUG "Synchronous exception occurred in EFI runtime service %s()\n", f);
> - clear_bit(EFI_RUNTIME_SERVICES, &efi.flags);
> return EFI_ABORTED;
> }
>
> diff --git a/arch/x86/platform/efi/quirks.c b/arch/x86/platform/efi/quirks.c
> index b0b848d6933afbcf..3f48fdf42d97dc1f 100644
> --- a/arch/x86/platform/efi/quirks.c
> +++ b/arch/x86/platform/efi/quirks.c
> @@ -757,9 +757,9 @@ void efi_crash_gracefully_on_page_fault(unsigned long phys_addr)
>
> /* Signal error status to the efi caller process */
> efi_rts_work.status = EFI_ABORTED;
> + efi.runtime_supported_mask = 0;
> complete(&efi_rts_work.efi_rts_comp);
>
> - clear_bit(EFI_RUNTIME_SERVICES, &efi.flags);
> pr_info("Froze efi_rts_wq and disabled EFI Runtime Services\n");
>
> /*
> diff --git a/drivers/firmware/efi/runtime-wrappers.c b/drivers/firmware/efi/runtime-wrappers.c
> index f3e54f6616f02475..336b8bcec86d0127 100644
> --- a/drivers/firmware/efi/runtime-wrappers.c
> +++ b/drivers/firmware/efi/runtime-wrappers.c
> @@ -60,8 +60,8 @@ struct efi_runtime_work efi_rts_work;
> ({ \
> efi_rts_work.status = EFI_ABORTED; \
> \
> - if (!efi_enabled(EFI_RUNTIME_SERVICES)) { \
> - pr_warn_once("EFI Runtime Services are disabled!\n"); \
> + if (!efi_rt_services_supported(efi_rts_work.efi_rts_id)) { \
> + pr_warn_ratelimited("EFI Runtime Service is disabled!\n");\
> goto exit; \
> } \
> \
> @@ -83,6 +83,8 @@ struct efi_runtime_work efi_rts_work;
> else \
> pr_err("Failed to queue work to efi_rts_wq.\n"); \
> \
> + if (efi_rts_work.status == EFI_ABORTED) \
> + efi.runtime_supported_mask &= ~efi_rts_work.efi_rts_id; \
> exit: \
> efi_rts_work.efi_rts_id = EFI_NONE; \
> efi_rts_work.status; \
> diff --git a/include/linux/efi.h b/include/linux/efi.h
> index 929d559ad41d29c6..61b252386d61cc4d 100644
> --- a/include/linux/efi.h
> +++ b/include/linux/efi.h
> @@ -1254,18 +1254,18 @@ extern unsigned long rci2_table_phys;
> */
> enum efi_rts_ids {
> EFI_NONE,
> - EFI_GET_TIME,
> - EFI_SET_TIME,
> - EFI_GET_WAKEUP_TIME,
> - EFI_SET_WAKEUP_TIME,
> - EFI_GET_VARIABLE,
> - EFI_GET_NEXT_VARIABLE,
> - EFI_SET_VARIABLE,
> - EFI_QUERY_VARIABLE_INFO,
> - EFI_GET_NEXT_HIGH_MONO_COUNT,
> - EFI_RESET_SYSTEM,
> - EFI_UPDATE_CAPSULE,
> - EFI_QUERY_CAPSULE_CAPS,
> + EFI_GET_TIME = EFI_RT_SUPPORTED_GET_TIME,
> + EFI_SET_TIME = EFI_RT_SUPPORTED_SET_TIME,
> + EFI_GET_WAKEUP_TIME = EFI_RT_SUPPORTED_GET_WAKEUP_TIME,
> + EFI_SET_WAKEUP_TIME = EFI_RT_SUPPORTED_SET_WAKEUP_TIME,
> + EFI_GET_VARIABLE = EFI_RT_SUPPORTED_GET_VARIABLE,
> + EFI_GET_NEXT_VARIABLE = EFI_RT_SUPPORTED_GET_NEXT_VARIABLE_NAME,
> + EFI_SET_VARIABLE = EFI_RT_SUPPORTED_SET_VARIABLE,
> + EFI_GET_NEXT_HIGH_MONO_COUNT = EFI_RT_SUPPORTED_GET_NEXT_HIGH_MONOTONIC_COUNT,
> + EFI_RESET_SYSTEM = EFI_RT_SUPPORTED_RESET_SYSTEM,
> + EFI_UPDATE_CAPSULE = EFI_RT_SUPPORTED_UPDATE_CAPSULE,
> + EFI_QUERY_CAPSULE_CAPS = EFI_RT_SUPPORTED_QUERY_CAPSULE_CAPABILITIES,
> + EFI_QUERY_VARIABLE_INFO = EFI_RT_SUPPORTED_QUERY_VARIABLE_INFO,
> };
>
> /*
> --
> 2.35.1
>
