arm64: efi: Robustify EFI runtime wrapper code

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 <[email protected]>

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

arch/arm64/include/asm/efi.h +3 -0

index 7c12e01c2b312e7b..1c408ec3c8b3a883 100644
@@ -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)

arch/arm64/kernel/efi-rt-wrapper.S +12 -1

index 75691a2641c1c0f8..b2786b968fee68dd 100644
@@ -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)

arch/arm64/kernel/efi.c +25 -0

index a908a37f03678b6b..8cb2e005f8aca589 100644
@@ -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);

Signature · 1 line

2.35.1

Hi Ard,

One drive-by comment below...

Quoted 17 lines · Ard Biesheuvel

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.

Quoted 5 lines

Signed-off-by Ard Biesheuvel <[email protected]>

---
 arch/arm64/include/asm/efi.h       |  3 +++
 arch/arm64/kernel/efi-rt-wrapper.S | 13 +++++++++-
 arch/arm64/kernel/efi.c            | 25 ++++++++++++++++++++

We'll need to teach the stack unwinder about this, or if we take an exception from the EFI stack, the backtrace will terminate as soon as it hits a frame record on the EFI stack.

In arch/arm64/kernel/stacktrace.c's arch_stack_walk(), that'll need to be added to the array of stack bounds. Ideally we'd only add that when a thread is making an EFI call.

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 <[email protected]>

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

arch/arm64/include/asm/efi.h +3 -0

index 7c12e01c2b312e7b..1c408ec3c8b3a883 100644
@@ -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)

arch/arm64/kernel/efi-rt-wrapper.S +12 -1

index 75691a2641c1c0f8..b2786b968fee68dd 100644
@@ -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)

arch/arm64/kernel/efi.c +25 -0

index a908a37f03678b6b..8cb2e005f8aca589 100644
@@ -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);

Signature · 1 line

2.35.1

Quoted 17 lines · Ard Biesheuvel

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.

Quoted 6 lines

Signed-off-by Ard Biesheuvel <[email protected]>

---
 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?

Quoted 29 lines · Lee Jones

Quoted 17 lines · Ard Biesheuvel

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.

Quoted 6 lines

Signed-off-by Ard Biesheuvel <[email protected]>

---
 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?

Quoted 4 lines

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?

Thanks for the reminder.

Only patch #1 is needed. It should be applied to v5.10 and later.

Quoted 32 lines · Ard Biesheuvel

Quoted 31 lines · Lee Jones

Quoted 17 lines · Ard Biesheuvel

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.

Quoted 6 lines

Signed-off-by Ard Biesheuvel <[email protected]>

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

Quoted 6 lines

Quoted 2 lines

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

Thanks for the reminder.

Only patch #1 is needed. It should be applied to v5.10 and later.

Perfect, thanks Ard.

Quoted 29 lines · Lee Jones

Quoted 17 lines · Ard Biesheuvel

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.

Quoted 6 lines

Signed-off-by Ard Biesheuvel <[email protected]>

---
 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?

Quoted 4 lines

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?

Thanks for the reminder.

Only patch #1 is needed. It should be applied to v5.10 and later.

Quoted 35 lines · Ard Biesheuvel

Quoted 33 lines · Lee Jones

Quoted 17 lines · Ard Biesheuvel

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.

Quoted 6 lines

Signed-off-by Ard Biesheuvel <[email protected]>

---
 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?

Quoted 3 lines

Thanks for the reminder.

Only patch #1 is needed. It should be applied to v5.10 and later.

Hold on, why did this go into mainline when I had an outstanding comment w.r.t. the stack unwinder?

From your last reply to me there I was expecting a respin with that fixed.

Quoted 37 lines · Mark Rutland

Quoted 35 lines · Ard Biesheuvel

Quoted 33 lines · Lee Jones

Quoted 17 lines · Ard Biesheuvel

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.

Quoted 6 lines

Signed-off-by Ard Biesheuvel <[email protected]>

---
 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?

Quoted 9 lines

Quoted 3 lines

Thanks for the reminder.

Only patch #1 is needed. It should be applied to v5.10 and later.

Hold on, why did this go into mainline when I had an outstanding comment w.r.t. the stack unwinder?

From your last reply to me there I was expecting a respin with that fixed.

Apologies for the confusion.

I have a patch for this queued up, but AIUI, that cannot be merged all the way back to v5.10, so these need to remain separate changes in any case.

https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=c2530a04a73e6b75ed71ed14d09d7b42d6300013

Quoted 48 lines · Ard Biesheuvel

Quoted 37 lines · Mark Rutland

Quoted 35 lines · Ard Biesheuvel

Quoted 33 lines · Lee Jones

Quoted 17 lines · Ard Biesheuvel

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.

Quoted 6 lines

Signed-off-by Ard Biesheuvel <[email protected]>

---
 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?

Quoted 9 lines

Quoted 3 lines

Thanks for the reminder.

Only patch #1 is needed. It should be applied to v5.10 and later.

Hold on, why did this go into mainline when I had an outstanding comment w.r.t. the stack unwinder?

From your last reply to me there I was expecting a respin with that fixed.

Quoted 7 lines

Apologies for the confusion.

I have a patch for this queued up, but AIUI, that cannot be merged all the way back to v5.10, so these need to remain separate changes in any case.

https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=c2530a04a73e6b75ed71ed14d09d7b42d6300013

Ah, ok, thanks for the pointer!

I'm a little uneasy here, still.

By backporting this we're also backporting the new breakage of the stack unwinder, and the minimal change for backports would be to add the lock and not the new stack (which was added for additinoal robustness, not to fix the bug the lock fixes).

I do appreciate that the additional stack is likely more useful than the occasional diagnostic output from the kernel, but it does seem like this has traded off one bug for another, and I'm just a little annoyed because I pointed that out before the first pull request was made.

I do know that this isn't malicious, and I'm not trying to start a fight, but now we have to consider whether we want/need to backport a stack unwinder fix to account for this, and we hadn't had that discussion before.

Quoted 67 lines · Mark Rutland

Quoted 48 lines · Ard Biesheuvel

Quoted 37 lines · Mark Rutland

Quoted 35 lines · Ard Biesheuvel

Quoted 33 lines · Lee Jones

Quoted 17 lines · Ard Biesheuvel

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.

Quoted 6 lines

Signed-off-by Ard Biesheuvel <[email protected]>

---
 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?

Quoted 9 lines

Quoted 3 lines

Thanks for the reminder.

Only patch #1 is needed. It should be applied to v5.10 and later.

Hold on, why did this go into mainline when I had an outstanding comment w.r.t. the stack unwinder?

From your last reply to me there I was expecting a respin with that fixed.

Quoted 7 lines

Apologies for the confusion.

I have a patch for this queued up, but AIUI, that cannot be merged all the way back to v5.10, so these need to remain separate changes in any case.

https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=c2530a04a73e6b75ed71ed14d09d7b42d6300013

Ah, ok, thanks for the pointer!

I'm a little uneasy here, still.

By backporting this we're also backporting the new breakage of the stack unwinder, and the minimal change for backports would be to add the lock and not the new stack (which was added for additinoal robustness, not to fix the bug the lock fixes).

Quoted 9 lines

I do appreciate that the additional stack is likely more useful than the occasional diagnostic output from the kernel, but it does seem like this has traded off one bug for another, and I'm just a little annoyed because I pointed that out before the first pull request was made.

I do know that this isn't malicious, and I'm not trying to start a fight, but now we have to consider whether we want/need to backport a stack unwinder fix to account for this, and we hadn't had that discussion before.

In that case, let's drop these backports for the time being, and collaborate on a solution that works for all of us.

Greg, could you please drop these again? Thanks.

Quoted 78 lines · Ard Biesheuvel

Quoted 76 lines · Mark Rutland

Quoted 48 lines · Ard Biesheuvel

Quoted 37 lines · Mark Rutland

Quoted 35 lines · Ard Biesheuvel

Quoted 33 lines · Lee Jones

Quoted 17 lines · Ard Biesheuvel

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.

Quoted 6 lines

Signed-off-by Ard Biesheuvel <[email protected]>

---
 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?

Quoted 9 lines

Quoted 3 lines

Thanks for the reminder.

Only patch #1 is needed. It should be applied to v5.10 and later.

Hold on, why did this go into mainline when I had an outstanding comment w.r.t. the stack unwinder?

From your last reply to me there I was expecting a respin with that fixed.

Quoted 7 lines

Apologies for the confusion.

I have a patch for this queued up, but AIUI, that cannot be merged all the way back to v5.10, so these need to remain separate changes in any case.

https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=c2530a04a73e6b75ed71ed14d09d7b42d6300013

Ah, ok, thanks for the pointer!

I'm a little uneasy here, still.

By backporting this we're also backporting the new breakage of the stack unwinder, and the minimal change for backports would be to add the lock and not the new stack (which was added for additinoal robustness, not to fix the bug the lock fixes).

I do appreciate that the additional stack is likely more useful than the occasional diagnostic output from the kernel, but it does seem like this has traded off one bug for another, and I'm just a little annoyed because I pointed that out before the first pull request was made.

I do know that this isn't malicious, and I'm not trying to start a fight, but now we have to consider whether we want/need to backport a stack unwinder fix to account for this, and we hadn't had that discussion before.

Quoted 4 lines

In that case, let's drop these backports for the time being, and collaborate on a solution that works for all of us.

Greg, could you please drop these again? Thanks.

Dropped now from all queues, thanks.

Quoted 84 lines · Greg KH

Quoted 82 lines · Ard Biesheuvel

Quoted 67 lines · Mark Rutland

Quoted 48 lines · Ard Biesheuvel

Quoted 37 lines · Mark Rutland

Quoted 35 lines · Ard Biesheuvel

Quoted 33 lines · Lee Jones

Quoted 17 lines · Ard Biesheuvel

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.

Quoted 6 lines

Signed-off-by Ard Biesheuvel <[email protected]>

---
 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?

Quoted 9 lines

Quoted 3 lines

Thanks for the reminder.

Only patch #1 is needed. It should be applied to v5.10 and later.

Hold on, why did this go into mainline when I had an outstanding comment w.r.t. the stack unwinder?

From your last reply to me there I was expecting a respin with that fixed.

Quoted 7 lines

Apologies for the confusion.

I have a patch for this queued up, but AIUI, that cannot be merged all the way back to v5.10, so these need to remain separate changes in any case.

https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=c2530a04a73e6b75ed71ed14d09d7b42d6300013

Ah, ok, thanks for the pointer!

I'm a little uneasy here, still.

By backporting this we're also backporting the new breakage of the stack unwinder, and the minimal change for backports would be to add the lock and not the new stack (which was added for additinoal robustness, not to fix the bug the lock fixes).

Quoted 9 lines

I do appreciate that the additional stack is likely more useful than the occasional diagnostic output from the kernel, but it does seem like this has traded off one bug for another, and I'm just a little annoyed because I pointed that out before the first pull request was made.

I do know that this isn't malicious, and I'm not trying to start a fight, but now we have to consider whether we want/need to backport a stack unwinder fix to account for this, and we hadn't had that discussion before.

In that case, let's drop these backports for the time being, and collaborate on a solution that works for all of us.

Greg, could you please drop these again? Thanks.

Quoted 1 line

Dropped now from all queues, thanks.

Now in Mainline as:

18bba1843fc7f efi: rt-wrapper: Add missing include ff7a167961d1b arm64: efi: Execute runtime services from a dedicated stack

Would you be kind enough to re-collect them please?

Quoted 72 lines · Mark Rutland

Quoted 48 lines · Ard Biesheuvel

Quoted 37 lines · Mark Rutland

Quoted 35 lines · Ard Biesheuvel

Quoted 33 lines · Lee Jones

Quoted 17 lines · Ard Biesheuvel

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.

Quoted 6 lines

Signed-off-by Ard Biesheuvel <[email protected]>

---
 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?

Quoted 9 lines

Quoted 3 lines

Thanks for the reminder.

Only patch #1 is needed. It should be applied to v5.10 and later.

Hold on, why did this go into mainline when I had an outstanding comment w.r.t. the stack unwinder?

From your last reply to me there I was expecting a respin with that fixed.

Quoted 7 lines

Apologies for the confusion.

I have a patch for this queued up, but AIUI, that cannot be merged all the way back to v5.10, so these need to remain separate changes in any case.

https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=c2530a04a73e6b75ed71ed14d09d7b42d6300013

Ah, ok, thanks for the pointer!

I'm a little uneasy here, still.

By backporting this we're also backporting the new breakage of the stack unwinder, and the minimal change for backports would be to add the lock and not the new stack (which was added for additinoal robustness, not to fix the bug the lock fixes).

I do appreciate that the additional stack is likely more useful than the occasional diagnostic output from the kernel, but it does seem like this has traded off one bug for another, and I'm just a little annoyed because I pointed that out before the first pull request was made.

Quoted 3 lines

I do know that this isn't malicious, and I'm not trying to start a fight, but now we have to consider whether we want/need to backport a stack unwinder fix to account for this, and we hadn't had that discussion before.

In that case, let's drop these backports for the time being, and collaborate on a solution that works for all of us.

Quoted 73 lines · Ard Biesheuvel

Quoted 72 lines · Mark Rutland

Quoted 48 lines · Ard Biesheuvel

Quoted 37 lines · Mark Rutland

Quoted 35 lines · Ard Biesheuvel

Quoted 33 lines · Lee Jones

Quoted 17 lines · Ard Biesheuvel

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.

Quoted 6 lines

Signed-off-by Ard Biesheuvel <[email protected]>

---
 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?

Quoted 9 lines

Quoted 3 lines

Thanks for the reminder.

Only patch #1 is needed. It should be applied to v5.10 and later.

Hold on, why did this go into mainline when I had an outstanding comment w.r.t. the stack unwinder?

From your last reply to me there I was expecting a respin with that fixed.

Quoted 7 lines

Apologies for the confusion.

I have a patch for this queued up, but AIUI, that cannot be merged all the way back to v5.10, so these need to remain separate changes in any case.

https://git.kernel.org/pub/scm/linux/kernel/git/next/linux-next.git/commit/?id=c2530a04a73e6b75ed71ed14d09d7b42d6300013

Ah, ok, thanks for the pointer!

I'm a little uneasy here, still.

By backporting this we're also backporting the new breakage of the stack unwinder, and the minimal change for backports would be to add the lock and not the new stack (which was added for additinoal robustness, not to fix the bug the lock fixes).

I do appreciate that the additional stack is likely more useful than the occasional diagnostic output from the kernel, but it does seem like this has traded off one bug for another, and I'm just a little annoyed because I pointed that out before the first pull request was made.

Quoted 6 lines

Quoted 3 lines

I do know that this isn't malicious, and I'm not trying to start a fight, but now we have to consider whether we want/need to backport a stack unwinder fix to account for this, and we hadn't had that discussion before.

In that case, let's drop these backports for the time being, and collaborate on a solution that works for all of us.

Thanks!

IIUC our options here are:

1) Create a cut-down patch for stable that just adds the new lock but leaves out the new stack.

I may be missing a reason why that's insufficient or painful.

2) Backport this *but* also backport the follow-up fixes from your other series:
https://lore.kernel.org/r/[email protected]

Above you mentioned something about v5.10, was that just to say that some manual backporting was required, or that there was a structural problem that would require more invasive changes / prerequisites?

3) Something else?

My preference would be (1), but if we are encountering issue with stack size on stable kernels, then I'd be happy to help with manual backporting effort for (2), as long as we backported all the relevant bits in one go.

Does that make sense, and does that sound reasonable to you?