efi: Follow-up fixes for EFI runtime stack

Commit ff7a167961d1b ("arm64: efi: Execute runtime services from a dedicated stack") introduced a dedicated stack for EFI runtime services, in an attempt to make the execution of EFI runtime services more robust, given that they execute at the same privilege level as the kernel.

However, this stack needs to be declared to the stacktrace machinery, which is careful not to walk the stack when it leads into memory regions that are not known to be allocated for stack use.

Also, given that the ACPI code may invoke the low-level EFI runtime call wrapper without using the dedicated kernel thread and workqueue, we should take this into account when trying to gracefully handle synchronous exceptions.

Changes since v2:
- clear efi_rt_stack_top[-1] from asm code, and use READ_ONCE() to read its value when not holding the spinlock, to ensure that all accesses are safe under concurrency;
- add Mark's ack to patch #2

Ard Biesheuvel (2):
arm64: efi: Avoid workqueue to check whether EFI runtime is live
arm64: efi: Account for the EFI runtime stack in stack unwinder

 arch/arm64/include/asm/efi.h        |  9 +++++++++
 arch/arm64/include/asm/stacktrace.h | 15 +++++++++++++++
 arch/arm64/kernel/efi-rt-wrapper.S  |  6 ++++++
 arch/arm64/kernel/efi.c             |  3 ++-
 arch/arm64/kernel/stacktrace.c      | 12 ++++++++++++
 5 files changed, 44 insertions(+), 1 deletion(-)

2.39.0

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.

---
 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(-)

index 31d13a6001df49c4..f68d13c3a44e7bb2 100644
@@ -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)
 
 /*

index a00886410537d6a6..11f3ec9f09e86de6 100644
@@ -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]

index fab05de2e12dd5d8..b273900f45668587 100644
@@ -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

Thanks for this!

Mark.

The EFI runtime services run from a dedicated stack now, and so the stack unwinder needs to be informed about this.

---
 arch/arm64/include/asm/stacktrace.h | 15 +++++++++++++++
 arch/arm64/kernel/stacktrace.c      | 12 ++++++++++++
 2 files changed, 27 insertions(+)

index 4e5354beafb01bac..66ec8caa6ac07fa0 100644
@@ -106,4 +106,19 @@ static inline struct stack_info stackinfo_get_sdei_critical(void)
 #define stackinfo_get_sdei_critical()	stackinfo_get_unknown()
 #endif
 
+#ifdef CONFIG_EFI
+extern u64 *efi_rt_stack_top;
+
+static inline struct stack_info stackinfo_get_efi(void)
+{
+	unsigned long high = (u64)efi_rt_stack_top;
+	unsigned long low = high - THREAD_SIZE;
+
+	return (struct stack_info) {
+		.low = low,
+		.high = high,
+	};
+}
+#endif
+
 #endif	/* __ASM_STACKTRACE_H */

index 117e2c180f3c77d8..83154303e682c8b6 100644
@@ -5,6 +5,7 @@
  * Copyright (C) 2012 ARM Ltd.
  */
 #include <linux/kernel.h>
+#include <linux/efi.h>
 #include <linux/export.h>
 #include <linux/ftrace.h>
 #include <linux/sched.h>

@@ -12,6 +13,7 @@
 #include <linux/sched/task_stack.h>
 #include <linux/stacktrace.h>
 
+#include <asm/efi.h>
 #include <asm/irq.h>
 #include <asm/stack_pointer.h>
 #include <asm/stacktrace.h>

@@ -186,6 +188,13 @@ void show_stack(struct task_struct *tsk, unsigned long *sp, const char *loglvl)
 			: stackinfo_get_unknown();		\
 	})
 
+#define STACKINFO_EFI						\
+	({							\
+		((task == current) && current_in_efi())		\
+			? stackinfo_get_efi()			\
+			: stackinfo_get_unknown();		\
+	})
+
 noinline noinstr void arch_stack_walk(stack_trace_consume_fn consume_entry,
 			      void *cookie, struct task_struct *task,
 			      struct pt_regs *regs)

@@ -199,6 +208,9 @@ noinline noinstr void arch_stack_walk(stack_trace_consume_fn consume_entry,
 #if defined(CONFIG_VMAP_STACK) && defined(CONFIG_ARM_SDE_INTERFACE)
 		STACKINFO_SDEI(normal),
 		STACKINFO_SDEI(critical),
+#endif
+#ifdef CONFIG_EFI
+		STACKINFO_EFI,
 #endif
 	};
 	struct unwind_state state = {

2.39.0

Hi Ard,

Apologies if this has been reported and/or fixed already, I searched lore and did not find anything but I just bisected a QEMU boot hang [1] that we see in the ClangBuiltLinux CI with Fedora's configuration [2] to this change in next-20220110 as commit a7334dc70496 ("arm64: efi:
Account for the EFI runtime stack in stack unwinder").

I do not think our QEMU command is very exotic but we are not booting via EFI (although I do see it even if we do boot via EFI):

$ qemu-system-aarch64 \
-cpu max,pauth-impdef=true \
-machine virt,gic-version=max,virtualization=true \
-kernel arch/arm64/boot/Image.gz
-append "console=ttyAMA0 earlycon" \
-display none \
-initrd images/arm64/rootfs.cpio
-m 512m \
-nodefaults \
-no-reboot \
-serial mon:stdio
[    0.000000] Booting Linux on physical CPU 0x0000000000 [0x000f0510]
[    0.000000] Linux version 6.2.0-rc2+ (tuxmake@tuxmake) (aarch64-linux-gcc (GCC) 12.2.0, GNU ld (GNU Binutils) 2.39) #1 SMP PREEMPT_DYNAMIC @1673275520
[    0.000000] random: crng init done
[    0.000000] Machine model: linux,dummy-virt
[    0.000000] efi: UEFI not found.
[    0.000000] earlycon: pl11 at MMIO 0x0000000009000000 (options '')
[    0.000000] printk: bootconsole [pl11] enabled
[    0.000000] NUMA: No NUMA configuration found
[    0.000000] NUMA: Faking a node at [mem 0x0000000040000000-0x000000005fffffff]
[    0.000000] NUMA: NODE_DATA [mem 0x5fe9e3c0-0x5feb4fff]
[    0.000000] Zone ranges:
[    0.000000]   DMA      [mem 0x0000000040000000-0x000000005fffffff]
[    0.000000]   DMA32    empty
[    0.000000]   Normal   empty
[    0.000000]   Device   empty
[    0.000000] Movable zone start for each node
[    0.000000] Early memory node ranges
[    0.000000]   node   0: [mem 0x0000000040000000-0x000000005fffffff]
[    0.000000] Initmem setup node 0 [mem 0x0000000040000000-0x000000005fffffff]
[    0.000000] cma: Reserved 64 MiB at 0x000000005b600000
[    0.000000] psci: probing for conduit method from DT.
[    0.000000] psci: PSCIv1.1 detected in firmware.
[    0.000000] psci: Using standard PSCI v0.2 function IDs
[    0.000000] psci: Trusted OS migration not required
[    0.000000] psci: SMC Calling Convention v1.0
[    0.000000] percpu: Embedded 32 pages/cpu s90408 r8192 d32472 u131072
[    0.000000] Detected PIPT I-cache on CPU0
[    0.000000] CPU features: detected: Address authentication (IMP DEF algorithm)
[    0.000000] CPU features: detected: GIC system register CPU interface
[    0.000000] CPU features: detected: Virtualization Host Extensions
[    0.000000] CPU features: detected: Hardware dirty bit management
[    0.000000] CPU features: detected: Spectre-v4
[    0.000000] alternatives: applying boot alternatives
[    0.000000] Fallback order for Node 0: 0
[    0.000000] Built 1 zonelists, mobility grouping on.  Total pages: 129024
[    0.000000] Policy zone: DMA
[    0.000000] Kernel command line: console=ttyAMA0 earlycon
[    0.000000] Dentry cache hash table entries: 65536 (order: 7, 524288 bytes, linear)
[    0.000000] Inode-cache hash table entries: 32768 (order: 6, 262144 bytes, linear)
[    0.000000] mem auto-init: stack:all(zero), heap alloc:off, heap free:off
[    0.000000] Memory: 384052K/524288K available (17088K kernel code, 4208K rwdata, 15032K rodata, 9536K init, 10717K bss, 74700K reserved, 65536K cma-reserved)
[    0.000000] SLUB: HWalign=64, Order=0-3, MinObjects=0, CPUs=1, Nodes=1
[    0.000000] ftrace: allocating 58614 entries in 229 pages
[    0.000000] ftrace: allocated 229 pages with 5 groups
[    0.000000] trace event string verifier disabled
[    0.000000] Dynamic Preempt: voluntary
[    0.000000] rcu: Preemptible hierarchical RCU implementation.
[    0.000000] rcu:     RCU restricting CPUs from NR_CPUS=4096 to nr_cpu_ids=1.
[    0.000000]  Trampoline variant of Tasks RCU enabled.
[    0.000000]  Rude variant of Tasks RCU enabled.
[    0.000000]  Tracing variant of Tasks RCU enabled.
[    0.000000] rcu: RCU calculated value of scheduler-enlistment delay is 10 jiffies.
[    0.000000] rcu: Adjusting geometry for rcu_fanout_leaf=16, nr_cpu_ids=1
[    0.000000] NR_IRQS: 64, nr_irqs: 64, preallocated irqs: 0
[    0.000000] GICv3: GIC: Using split EOI/Deactivate mode
[    0.000000] GICv3: 256 SPIs implemented
[    0.000000] GICv3: 0 Extended SPIs implemented
[    0.000000] Root IRQ handler: gic_handle_irq
[    0.000000] GICv3: GICv3 features: 16 PPIs
[    0.000000] GICv3: GICv4 features:
[    0.000000] GICv3: CPU0: found redistributor 0 region 0:0x00000000080a0000
[    0.000000] ITS [mem 0x08080000-0x0809ffff]
[    0.000000] ITS@0x0000000008080000: Single VMOVP capable
[    0.000000] ITS@0x0000000008080000: allocated 8192 Devices @43de0000 (indirect, esz 8, psz 64K, shr 1)
[    0.000000] ITS@0x0000000008080000: allocated 8192 Interrupt Collections @43df0000 (flat, esz 8, psz 64K, shr 1)
[    0.000000] ITS@0x0000000008080000: allocated 8192 Virtual CPUs @43e00000 (indirect, esz 8, psz 64K, shr 1)
[    0.000000] GICv3: using LPI property table @0x0000000043e10000
[    0.000000] ITS: Allocated DevID ffff as GICv4 proxy device (2 slots)
[    0.000000] ITS: Enabling GICv4 support
[    0.000000] GICv3: CPU0: using allocated LPI pending table @0x0000000043e20000
[    0.000000] rcu: srcu_init: Setting srcu_struct sizes based on contention.
[    0.000000] arch_timer: cp15 timer(s) running at 62.50MHz (phys).
[    0.000000] clocksource: arch_sys_counter: mask: 0x1ffffffffffffff max_cycles: 0x1cd42e208c, max_idle_ns: 881590405314 ns
[    0.000069] sched_clock: 57 bits at 63MHz, resolution 16ns, wraps every 4398046511096ns
[    0.002785] kfence: initialized - using 2097152 bytes for 255 objects at 0x(____ptrval____)-0x(____ptrval____)
[    0.014680] Console: colour dummy device 80x25
[    0.022555] Calibrating delay loop (skipped), value calculated using timer frequency.. 125.00 BogoMIPS (lpj=625000)
[    0.023292] pid_max: default: 32768 minimum: 301
[    0.025346] LSM: initializing lsm=lockdown,capability,yama,integrity,selinux,bpf,landlock
[    0.026087] Yama: becoming mindful.
[    0.027053] SELinux:  Initializing.
[    0.027935] LSM support for eBPF active
[    0.028089] landlock: Up and running.
[    0.030514] Mount-cache hash table entries: 1024 (order: 1, 8192 bytes, linear)
[    0.030706] Mountpoint-cache hash table entries: 1024 (order: 1, 8192 bytes, linear)
qemu-system-aarch64: terminating on signal 15 from pid 4112158 (timeout)

This does not appear to be clang specific, as I can reproduce it with the same configuration using kernel.org's GCC 12.2.0 (as you'll see above). I unfortunately do not have time to bisect configurations to see if there is a more trivial reproducer on top of defconfig, so just this report.

If there is any additional information I can provide, please let me know.

Cheers,
Nathan

[1]: https://github.com/ClangBuiltLinux/continuous-integration2/actions/runs/3883388450/jobs/6628748170
[2]: https://src.fedoraproject.org/rpms/kernel/raw/rawhide/f/kernel-aarch64-fedora.config

# bad: [435bf71af3a0aa8067f3b87ff9febf68b564dbb6] Add linux-next specific files for 20230110
# good: [1fe4fd6f5cad346e598593af36caeadc4f5d4fa9] Merge tag 'xfs-6.2-fixes-2' of git://git.kernel.org/pub/scm/fs/xfs/xfs-linux
git bisect start '435bf71af3a0aa8067f3b87ff9febf68b564dbb6' '1fe4fd6f5cad346e598593af36caeadc4f5d4fa9'
# bad: [57aac56e8af1628ef96055820f88ca547233b310] Merge branch 'drm-next' of git://git.freedesktop.org/git/drm/drm.git
git bisect bad 57aac56e8af1628ef96055820f88ca547233b310
# bad: [68efa45676e8f0b73c896ee4f82996ac396455af] Merge branch 'riscv-soc-for-next' of git://git.kernel.org/pub/scm/linux/kernel/git/conor/linux.git
git bisect bad 68efa45676e8f0b73c896ee4f82996ac396455af
# bad: [154e09c58f5dcd853bd54ac8b725132126d0b640] Merge branch 'for-next' of https://git.kernel.org/pub/scm/linux/kernel/git/krzk/linux-mem-ctrl.git
git bisect bad 154e09c58f5dcd853bd54ac8b725132126d0b640
# good: [0680aa5d9dee0e17d169c022f29c53ec6ad3f69d] Merge branch 'fixes' of git://git.kernel.org/pub/scm/linux/kernel/git/xiang/erofs.git
git bisect good 0680aa5d9dee0e17d169c022f29c53ec6ad3f69d
# good: [76c76819b36d2433e2637c91dbdc4ec345bf7d92] ARM: remove CONFIG_UNUSED_BOARD_FILES
git bisect good 76c76819b36d2433e2637c91dbdc4ec345bf7d92
# bad: [d08fc57162414439a50e36eb5c4554a56eb8e121] Merge branch 'for-next' of git://git.armlinux.org.uk/~rmk/linux-arm.git
git bisect bad d08fc57162414439a50e36eb5c4554a56eb8e121
# bad: [3145d9dfed323d5a82d40670e333de58fb1a9e65] Merge branch 'for-linux-next-fixes' of git://anongit.freedesktop.org/drm/drm-misc
git bisect bad 3145d9dfed323d5a82d40670e333de58fb1a9e65
# good: [3b3d45c3b305edeb93053c05993bd7c58b9ceb94] Merge branch 'for-rc' of git://git.kernel.org/pub/scm/linux/kernel/git/cel/linux
git bisect good 3b3d45c3b305edeb93053c05993bd7c58b9ceb94
# bad: [33b73800589ae5a93e3b2faf2fbac7c30d6fdd1a] Merge branch 'urgent' of git://git.kernel.org/pub/scm/linux/kernel/git/efi/efi.git
git bisect bad 33b73800589ae5a93e3b2faf2fbac7c30d6fdd1a
# good: [18bba1843fc7f264f58c9345d00827d082f9c558] efi: rt-wrapper: Add missing include
git bisect good 18bba1843fc7f264f58c9345d00827d082f9c558
# bad: [a7334dc70496bb0cec7f1d3b1f148855951f41c5] arm64: efi: Account for the EFI runtime stack in stack unwinder
git bisect bad a7334dc70496bb0cec7f1d3b1f148855951f41c5
# good: [4a70773264fd7f00b8401b5f9addb72b58678fdf] arm64: efi: Avoid workqueue to check whether EFI runtime is live
git bisect good 4a70773264fd7f00b8401b5f9addb72b58678fdf

# first bad commit: [a7334dc70496bb0cec7f1d3b1f148855951f41c5] arm64: efi: Account for the EFI runtime stack in stack unwinder

Thanks for the report. This is due to an oversight on my part: we removed a spin_is_locked() check, and the lock in question can only be in the locked state when EFI runtime services are enabled to begin with.

Without the lock check, we may end up dereferencing the uninitialized efi_rt_stack_top on non-EFI boots.

I've fixed this up in the EFI fixes tree, so the issue should disappear once -next is updated. (We just missed 20230111 unfortunately)

Hi Ard,

Thank you for the quick response! That issue appears to be fixed.

Unfortunately, I am still seeing a hang while booting via EFI on either bare metal or KVM when CONFIG_DEBUG_PREEMPT is enabled (Fedora's rawhide config appears to enable several debugging options), so it appears I was seeing two distinct issues :/ defconfig + CONFIG_DEBUG_PREEMPT=y is enough for me to reproduce this problem.

I see

  [    0.015382] Remapping and enabling EFI services.

as the last line in the console (via earlycon) with the bad kernel and nothing after it (I assume we deadlock somewhere or hit a BUG_ON()?), vs

  [    0.015191] Remapping and enabling EFI services.
  [    0.016725] smp: Bringing up secondary CPUs ...

on the good kernel, followed by a normal boot.

Sorry for not noticing this sooner! It should be pretty easy to reproduce but if there is any other information I can provide, I am more than happy to do so.

Cheers,
Nathan

Yeah, this is the same issue, essentially.

I have added back the spin_is_locked() check, which is a better indicator of whether the EFI runtime stack is actually in use or not.

Should either / both of these be routed to Stable?

If so, we should probably Cc: them.

Lee Jones [李琼斯]

I'll forward them to -stable by hand once all the bits and pieces land in Linus's tree.

Feel free to remind in a week or so, though.

Thanks,
Ard.