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 >