Re: [PATCH 1/2] arm64: efi: Execute runtime services from a dedicated stack

[Lee Jones <lee@kernel.org>]

On Mon, 05 Dec 2022, Ard Biesheuvel wrote:

> With the introduction of PRMT in the ACPI subsystem, the EFI rts
> workqueue is no longer the only caller of efi_call_virt_pointer() in the
> kernel. This means the EFI runtime services lock is no longer sufficient
> to manage concurrent calls into firmware, but also that firmware calls
> may occur that are not marshalled via the workqueue mechanism, but
> originate directly from the caller context.
> 
> For added robustness, and to ensure that the runtime services have 8 KiB
> of stack space available as per the EFI spec, introduce a spinlock
> protected EFI runtime stack of 8 KiB, where the spinlock also ensures
> serialization between the EFI rts workqueue (which itself serializes EFI
> runtime calls) and other callers of efi_call_virt_pointer().
> 
> While at it, use the stack pivot to avoid reloading the shadow call
> stack pointer from the ordinary stack, as doing so could produce a
> gadget to defeat it.
> 
> Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
> ---
>  arch/arm64/include/asm/efi.h       |  3 +++
>  arch/arm64/kernel/efi-rt-wrapper.S | 13 +++++++++-
>  arch/arm64/kernel/efi.c            | 25 ++++++++++++++++++++
>  3 files changed, 40 insertions(+), 1 deletion(-)

Could we have this in Stable please?

Upstream commit: ff7a167961d1b ("arm64: efi: Execute runtime services from a dedicated stack")

Ard, do we need Patch 2 as well, or can this be applied on its own?

> diff --git a/arch/arm64/include/asm/efi.h b/arch/arm64/include/asm/efi.h
> index 7c12e01c2b312e7b..1c408ec3c8b3a883 100644
> --- a/arch/arm64/include/asm/efi.h
> +++ b/arch/arm64/include/asm/efi.h
> @@ -25,6 +25,7 @@ int efi_set_mapping_permissions(struct mm_struct *mm, efi_memory_desc_t *md);
>  ({									\
>  	efi_virtmap_load();						\
>  	__efi_fpsimd_begin();						\
> +	spin_lock(&efi_rt_lock);					\
>  })
>  
>  #undef arch_efi_call_virt
> @@ -33,10 +34,12 @@ int efi_set_mapping_permissions(struct mm_struct *mm, efi_memory_desc_t *md);
>  
>  #define arch_efi_call_virt_teardown()					\
>  ({									\
> +	spin_unlock(&efi_rt_lock);					\
>  	__efi_fpsimd_end();						\
>  	efi_virtmap_unload();						\
>  })
>  
> +extern spinlock_t efi_rt_lock;
>  efi_status_t __efi_rt_asm_wrapper(void *, const char *, ...);
>  
>  #define ARCH_EFI_IRQ_FLAGS_MASK (PSR_D_BIT | PSR_A_BIT | PSR_I_BIT | PSR_F_BIT)
> diff --git a/arch/arm64/kernel/efi-rt-wrapper.S b/arch/arm64/kernel/efi-rt-wrapper.S
> index 75691a2641c1c0f8..b2786b968fee68dd 100644
> --- a/arch/arm64/kernel/efi-rt-wrapper.S
> +++ b/arch/arm64/kernel/efi-rt-wrapper.S
> @@ -16,6 +16,12 @@ SYM_FUNC_START(__efi_rt_asm_wrapper)
>  	 */
>  	stp	x1, x18, [sp, #16]
>  
> +	ldr_l	x16, efi_rt_stack_top
> +	mov	sp, x16
> +#ifdef CONFIG_SHADOW_CALL_STACK
> +	str	x18, [sp, #-16]!
> +#endif
> +
>  	/*
>  	 * We are lucky enough that no EFI runtime services take more than
>  	 * 5 arguments, so all are passed in registers rather than via the
> @@ -29,6 +35,7 @@ SYM_FUNC_START(__efi_rt_asm_wrapper)
>  	mov	x4, x6
>  	blr	x8
>  
> +	mov	sp, x29
>  	ldp	x1, x2, [sp, #16]
>  	cmp	x2, x18
>  	ldp	x29, x30, [sp], #32
> @@ -42,6 +49,10 @@ SYM_FUNC_START(__efi_rt_asm_wrapper)
>  	 * called with preemption disabled and a separate shadow stack is used
>  	 * for interrupts.
>  	 */
> -	mov	x18, x2
> +#ifdef CONFIG_SHADOW_CALL_STACK
> +	ldr_l	x18, efi_rt_stack_top
> +	ldr	x18, [x18, #-16]
> +#endif
> +
>  	b	efi_handle_corrupted_x18	// tail call
>  SYM_FUNC_END(__efi_rt_asm_wrapper)
> diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
> index a908a37f03678b6b..8cb2e005f8aca589 100644
> --- a/arch/arm64/kernel/efi.c
> +++ b/arch/arm64/kernel/efi.c
> @@ -144,3 +144,28 @@ asmlinkage efi_status_t efi_handle_corrupted_x18(efi_status_t s, const char *f)
>  	pr_err_ratelimited(FW_BUG "register x18 corrupted by EFI %s\n", f);
>  	return s;
>  }
> +
> +DEFINE_SPINLOCK(efi_rt_lock);
> +
> +asmlinkage u64 *efi_rt_stack_top __ro_after_init;
> +
> +/* required by the EFI spec */
> +static_assert(THREAD_SIZE >= SZ_8K);
> +
> +int __init arm64_efi_rt_init(void)
> +{
> +	void *p = __vmalloc_node_range(THREAD_SIZE, THREAD_ALIGN,
> +				       VMALLOC_START, VMALLOC_END, GFP_KERNEL,
> +				       PAGE_KERNEL, 0, NUMA_NO_NODE,
> +				       __builtin_return_address(0));
> +
> +	if (!p) {
> +		pr_warn("Failed to allocate EFI runtime stack\n");
> +		clear_bit(EFI_RUNTIME_SERVICES, &efi.flags);
> +		return -ENOMEM;
> +	}
> +
> +	efi_rt_stack_top = p + THREAD_SIZE;
> +	return 0;
> +}
> +core_initcall(arm64_efi_rt_init);
> -- 
> 2.35.1
> 
> 

-- 
Lee Jones [李琼斯]