Re: [PATCH v2 1/2] arm64: efi: Avoid workqueue to check whether EFI runtime is live

On Thu, 5 Jan 2023 at 13:46, Mark Rutland [off-list ref] wrote:

Hi Ard,

On Wed, Jan 04, 2023 at 06:44:32PM +0100, Ard Biesheuvel wrote:

quoted

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 the spinlock is locked, 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.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 arch/arm64/include/asm/efi.h | 10 ++++++++++
 arch/arm64/kernel/efi.c      |  3 ++-
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/arch/arm64/include/asm/efi.h b/arch/arm64/include/asm/efi.h
index 31d13a6001df49c4..aca6dcaa33efbac4 100644
--- a/arch/arm64/include/asm/efi.h
+++ b/arch/arm64/include/asm/efi.h

@@ -42,14 +42,24 @@ int efi_set_mapping_permissions(struct mm_struct *mm, efi_memory_desc_t *md);

 #define arch_efi_call_virt_teardown()                                        \
 ({                                                                   \
+     efi_rt_stack_top[-1] = 0;                                       \

Is there any reason not to do this in the asm, given all the other setting of
this occurs there? I know that'd mean duplicating the writ for both the regular
case and the exception handler, but then it'd be clearly associated with the
instant we move away from the EFI RT stack.

That would also hide this write from KCSAN; itherwise this'll need to be a
WRITE_ONCE() to pair with the (not necessariyl) locked read in current_in_efi()
below.

Sure.

quoted

      spin_unlock(&efi_rt_lock);                                      \
      __efi_fpsimd_end();                                             \
      efi_virtmap_unload();                                           \
 })

 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() && spin_is_locked(&efi_rt_lock) &&              \
+      on_task_stack(current, efi_rt_stack_top[-1], 1))

KCSAN is liable to complain about the access to efi_rt_stack_top[-1], since
that can race with another thread updating the value, and it's not necessarily
single-copy-atomic.

It's probably worth making this a READ_ONCE(), even if we move all the writes
to asm, to avoid tearing.

Aside from those points, this looks good to me.

ok

_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel