Re: [PATCH v3 5/7] efi/runtime-wrappers: bound the wait for EFI runtime service calls

[Yeoreum Yun]

Hi Berno,

> When an EFI runtime service hangs in firmware, the efi_rts_wq worker is
> stuck inside the call and cannot be cancelled. __efi_queue_work() then
> waits on the completion forever while holding efi_runtime_lock, so every
> later EFI caller is wedged until reboot; the only symptom is a "workqueue
> lockup" and tasks piling up on the semaphore.
> 
> Replace wait_for_completion() with wait_for_completion_timeout() bounded
> by EFI_RTS_TIMEOUT (120 seconds). On timeout, clear EFI_RUNTIME_SERVICES
> and return EFI_ABORTED so later callers fail fast at the entry check
> instead of each paying another 120 seconds. The wedged worker is
> intentionally leaked and keeps ownership of efi_rts_work.
> 
> A worker that only starts running after the timeout would otherwise
> dereference efi_rts_work.args, now pointing into the caller's freed stack
> frame, and hand stale pointers to firmware. Park it with
> efi_rts_park_worker() at the entry of efi_call_rts() when runtime
> services are already disabled, before it touches args or enters firmware.
> 
> Known limitation: a worker already inside firmware when the timeout fires
> still holds efi_rts_args pointing into the caller's stack frame; if
> firmware unblocks afterwards and writes the output buffers, they land in
> reused memory. Firmware hung this long rarely recovers; a follow-up could
> bounce the buffers through kmalloc.
> 
> Signed-off-by: Breno Leitao <leitao@xxxxxxxxxx>
> ---
>  drivers/firmware/efi/runtime-wrappers.c | 19 ++++++++++++++++++-
>  1 file changed, 18 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/firmware/efi/runtime-wrappers.c b/drivers/firmware/efi/runtime-wrappers.c
> index ae974edc0b04e..2ec5cbdf46d07 100644
> --- a/drivers/firmware/efi/runtime-wrappers.c
> +++ b/drivers/firmware/efi/runtime-wrappers.c
> @@ -118,6 +118,14 @@ union efi_rts_args {
>  
>  struct efi_runtime_work efi_rts_work;
>  
> +/*
> + * Upper bound on how long we wait for a single EFI runtime service
> + * call to finish before declaring firmware wedged. Chosen to be longer
> + * than any plausible legitimate call (including UpdateCapsule on slow
> + * SPI-NOR) while still bounding userspace wait time.
> + */
> +#define EFI_RTS_TIMEOUT		(120 * HZ)
> +
>  /*
>   * efi_queue_work:	Queue EFI runtime service call and wait for completion
>   * @_rts:		EFI runtime service function identifier
> @@ -234,6 +242,9 @@ static void __nocfi efi_call_rts(struct work_struct *work)
>  	efi_status_t status = EFI_NOT_FOUND;
>  	unsigned long flags;
>  
> +	if (!efi_enabled(EFI_RUNTIME_SERVICES))
> +		efi_rts_park_worker();
> +
>  	efi_runtime_lock_owner = current;
>  
>  	arch_efi_call_virt_setup();
> @@ -355,7 +366,13 @@ static efi_status_t __efi_queue_work(enum efi_rts_ids id,
>  		goto exit;
>  	}
>  
> -	wait_for_completion(&efi_rts_work.efi_rts_comp);
> +	if (!wait_for_completion_timeout(&efi_rts_work.efi_rts_comp,
> +					 EFI_RTS_TIMEOUT)) {
> +		pr_err("EFI runtime service %d wedged in firmware; disabling EFI runtime services\n",
> +		       id);
> +		clear_bit(EFI_RUNTIME_SERVICES, &efi.flags);
> +		return EFI_ABORTED;
> +	}

One of my concern is for the UpdateCapsule runtime service.
It might have a case where takes more then 2 mins and there's one
RFC to support a runtime capsule update for arm platform [1].

But I'm not sure how we can discern whether it's a time consuming or
for the bug on the firmware.

At least, should EFI_RTS be configurable via Kconfig and boot param
(timeout or keep the former behavior -- wait_for_completion without
timeout)?

Thanks.

Link: [1] https://github.com/tianocore/tianocore-wiki.github.io/pull/34

-- 
Sincerely,
Yeoreum Yun