| Message ID | 20230106174703.1883495-2-ardb@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | efi: Follow-up fixes for EFI runtime stack | expand |
On Fri, Jan 06, 2023 at 06:47:02PM +0100, Ard Biesheuvel wrote: > Comparing current_work() against efi_rts_work.work is sufficient to > decide whether current is currently running EFI runtime services code at > any level in its call stack. > > However, there are other potential users of the EFI runtime stack, such > as the ACPI subsystem, which may invoke efi_call_virt_pointer() > directly, and so any sync exceptions occurring in firmware during those > calls are currently misidentified. > > So instead, let's check whether 'current' has preemption disabled, and > whether the stashed value of the thread stack pointer points into > current's thread stack. This can only be the case if current was > interrupted while running EFI runtime code. This implies that we should > clear the stashed value after switching back, to avoid false positives. > > Signed-off-by: Ard Biesheuvel <ardb@kernel.org> Reviewed-by: Mark Rutland <mark.rutland@arm.com> Thanks for this! Mark. > --- > arch/arm64/include/asm/efi.h | 9 +++++++++ > arch/arm64/kernel/efi-rt-wrapper.S | 6 ++++++ > arch/arm64/kernel/efi.c | 3 ++- > 3 files changed, 17 insertions(+), 1 deletion(-) > > diff --git a/arch/arm64/include/asm/efi.h b/arch/arm64/include/asm/efi.h > index 31d13a6001df49c4..f68d13c3a44e7bb2 100644 > --- a/arch/arm64/include/asm/efi.h > +++ b/arch/arm64/include/asm/efi.h > @@ -48,8 +48,17 @@ int efi_set_mapping_permissions(struct mm_struct *mm, efi_memory_desc_t *md); > }) > > extern spinlock_t efi_rt_lock; > +extern u64 *efi_rt_stack_top; > efi_status_t __efi_rt_asm_wrapper(void *, const char *, ...); > > +/* > + * efi_rt_stack_top[-1] contains the value the stack pointer had before > + * switching to the EFI runtime stack. > + */ > +#define current_in_efi() \ > + (!preemptible() && \ > + on_task_stack(current, READ_ONCE(efi_rt_stack_top[-1]), 1)) > + > #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 a00886410537d6a6..11f3ec9f09e86de6 100644 > --- a/arch/arm64/kernel/efi-rt-wrapper.S > +++ b/arch/arm64/kernel/efi-rt-wrapper.S > @@ -45,7 +45,10 @@ SYM_FUNC_START(__efi_rt_asm_wrapper) > mov x4, x6 > blr x8 > > + mov x16, sp > mov sp, x29 > + str xzr, [x16, #8] // clear recorded task SP value > + > ldp x1, x2, [sp, #16] > cmp x2, x18 > ldp x29, x30, [sp], #112 > @@ -70,6 +73,9 @@ SYM_FUNC_END(__efi_rt_asm_wrapper) > SYM_CODE_START(__efi_rt_asm_recover) > mov sp, x30 > > + ldr_l x16, efi_rt_stack_top // clear recorded task SP value > + str xzr, [x16, #-8] > + > ldp x19, x20, [sp, #32] > ldp x21, x22, [sp, #48] > ldp x23, x24, [sp, #64] > diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c > index fab05de2e12dd5d8..b273900f45668587 100644 > --- a/arch/arm64/kernel/efi.c > +++ b/arch/arm64/kernel/efi.c > @@ -11,6 +11,7 @@ > #include <linux/init.h> > > #include <asm/efi.h> > +#include <asm/stacktrace.h> > > static bool region_is_misaligned(const efi_memory_desc_t *md) > { > @@ -154,7 +155,7 @@ asmlinkage efi_status_t __efi_rt_asm_recover(void); > bool efi_runtime_fixup_exception(struct pt_regs *regs, const char *msg) > { > /* Check whether the exception occurred while running the firmware */ > - if (current_work() != &efi_rts_work.work || regs->pc >= TASK_SIZE_64) > + if (!current_in_efi() || regs->pc >= TASK_SIZE_64) > return false; > > pr_err(FW_BUG "Unable to handle %s in EFI runtime service\n", msg); > -- > 2.39.0 >
diff --git a/arch/arm64/include/asm/efi.h b/arch/arm64/include/asm/efi.h index 31d13a6001df49c4..f68d13c3a44e7bb2 100644 --- a/arch/arm64/include/asm/efi.h +++ b/arch/arm64/include/asm/efi.h @@ -48,8 +48,17 @@ int efi_set_mapping_permissions(struct mm_struct *mm, efi_memory_desc_t *md); }) extern spinlock_t efi_rt_lock; +extern u64 *efi_rt_stack_top; efi_status_t __efi_rt_asm_wrapper(void *, const char *, ...); +/* + * efi_rt_stack_top[-1] contains the value the stack pointer had before + * switching to the EFI runtime stack. + */ +#define current_in_efi() \ + (!preemptible() && \ + on_task_stack(current, READ_ONCE(efi_rt_stack_top[-1]), 1)) + #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 a00886410537d6a6..11f3ec9f09e86de6 100644 --- a/arch/arm64/kernel/efi-rt-wrapper.S +++ b/arch/arm64/kernel/efi-rt-wrapper.S @@ -45,7 +45,10 @@ SYM_FUNC_START(__efi_rt_asm_wrapper) mov x4, x6 blr x8 + mov x16, sp mov sp, x29 + str xzr, [x16, #8] // clear recorded task SP value + ldp x1, x2, [sp, #16] cmp x2, x18 ldp x29, x30, [sp], #112 @@ -70,6 +73,9 @@ SYM_FUNC_END(__efi_rt_asm_wrapper) SYM_CODE_START(__efi_rt_asm_recover) mov sp, x30 + ldr_l x16, efi_rt_stack_top // clear recorded task SP value + str xzr, [x16, #-8] + ldp x19, x20, [sp, #32] ldp x21, x22, [sp, #48] ldp x23, x24, [sp, #64] diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c index fab05de2e12dd5d8..b273900f45668587 100644 --- a/arch/arm64/kernel/efi.c +++ b/arch/arm64/kernel/efi.c @@ -11,6 +11,7 @@ #include <linux/init.h> #include <asm/efi.h> +#include <asm/stacktrace.h> static bool region_is_misaligned(const efi_memory_desc_t *md) { @@ -154,7 +155,7 @@ asmlinkage efi_status_t __efi_rt_asm_recover(void); bool efi_runtime_fixup_exception(struct pt_regs *regs, const char *msg) { /* Check whether the exception occurred while running the firmware */ - if (current_work() != &efi_rts_work.work || regs->pc >= TASK_SIZE_64) + if (!current_in_efi() || regs->pc >= TASK_SIZE_64) return false; pr_err(FW_BUG "Unable to handle %s in EFI runtime service\n", msg);
Comparing current_work() against efi_rts_work.work is sufficient to decide whether current is currently running EFI runtime services code at any level in its call stack. However, there are other potential users of the EFI runtime stack, such as the ACPI subsystem, which may invoke efi_call_virt_pointer() directly, and so any sync exceptions occurring in firmware during those calls are currently misidentified. So instead, let's check whether 'current' has preemption disabled, and whether the stashed value of the thread stack pointer points into current's thread stack. This can only be the case if current was interrupted while running EFI runtime code. This implies that we should clear the stashed value after switching back, to avoid false positives. Signed-off-by: Ard Biesheuvel <ardb@kernel.org> --- arch/arm64/include/asm/efi.h | 9 +++++++++ arch/arm64/kernel/efi-rt-wrapper.S | 6 ++++++ arch/arm64/kernel/efi.c | 3 ++- 3 files changed, 17 insertions(+), 1 deletion(-)