[PATCH 0/2] arm64: efi: Robustify EFI runtime wrapper code

[PATCH 0/2] arm64: efi: Robustify EFI runtime wrapper code

[Ard Biesheuvel]

Make the EFI runtime wrapper code more robust, by switching to a
dedicated stack and dealing with sync exceptions occurring in firmware
by unwinding it. While at it, move the backup copy of register X18 onto
the base of that stack too so we can can restore it if needed without
reloading it from the ordinary stack.

Patch #2 is a v2 of a patch that got merged and reverted again in
v6.1-rc.

This supersedes 'arm64: efi: Move runtime services asm wrapper out of
.text'

Cc: Sami Tolvanen <samitolvanen@google.com>
Cc: Kees Cook <keescook@chromium.org>

Ard Biesheuvel (2):
  arm64: efi: Execute runtime services from a dedicated stack
  arm64: efi: Recover from synchronous exceptions occurring in firmware

 arch/arm64/include/asm/efi.h            | 11 +++++
 arch/arm64/kernel/efi-rt-wrapper.S      | 38 ++++++++++++++--
 arch/arm64/kernel/efi.c                 | 47 ++++++++++++++++++++
 arch/arm64/mm/fault.c                   |  4 ++
 drivers/firmware/efi/runtime-wrappers.c |  1 +
 5 files changed, 98 insertions(+), 3 deletions(-)

-- 
2.35.1

[PATCH 0/2] arm64: efi: Robustify EFI runtime wrapper code

[Ard Biesheuvel]

Make the EFI runtime wrapper code more robust, by switching to a
dedicated stack and dealing with sync exceptions occurring in firmware
by unwinding it. While at it, move the backup copy of register X18 onto
the base of that stack too so we can can restore it if needed without
reloading it from the ordinary stack.

Patch #2 is a v2 of a patch that got merged and reverted again in
v6.1-rc.

This supersedes 'arm64: efi: Move runtime services asm wrapper out of
.text'

Cc: Sami Tolvanen <samitolvanen@google.com>
Cc: Kees Cook <keescook@chromium.org>

Ard Biesheuvel (2):
  arm64: efi: Execute runtime services from a dedicated stack
  arm64: efi: Recover from synchronous exceptions occurring in firmware

 arch/arm64/include/asm/efi.h            | 11 +++++
 arch/arm64/kernel/efi-rt-wrapper.S      | 38 ++++++++++++++--
 arch/arm64/kernel/efi.c                 | 47 ++++++++++++++++++++
 arch/arm64/mm/fault.c                   |  4 ++
 drivers/firmware/efi/runtime-wrappers.c |  1 +
 5 files changed, 98 insertions(+), 3 deletions(-)

-- 
2.35.1


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

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

[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.

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

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

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

[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.

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

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


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

[PATCH 2/2] arm64: efi: Recover from synchronous exceptions occurring in firmware

[Ard Biesheuvel]

Unlike x86, which has machinery to deal with page faults that occur
during the execution of EFI runtime services, arm64 has nothing like
that, and a synchronous exception raised by firmware code brings down
the whole system.

With more EFI based systems appearing that were not built to run Linux
(such as the Windows-on-ARM laptops based on Qualcomm SOCs), as well as
the introduction of PRM (platform specific firmware routines that are
callable just like EFI runtime services), we are more likely to run into
issues of this sort, and it is much more likely that we can identify and
work around such issues if they don't bring down the system entirely.

Since we already use a EFI runtime services call wrapper in assembler,
we can quite easily add some code that captures the execution state at
the point where the call is made, allowing us to revert to this state
and proceed execution if the call triggered a synchronous exception.

Given that the kernel and the firmware don't share any data structures
that could end up in an indeterminate state, we can happily continue
running, as long as we mark the EFI runtime services as unavailable from
that point on.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
Acked-by: Catalin Marinas <catalin.marinas@arm.com>
---
 arch/arm64/include/asm/efi.h            |  8 +++++
 arch/arm64/kernel/efi-rt-wrapper.S      | 31 ++++++++++++++++----
 arch/arm64/kernel/efi.c                 | 22 ++++++++++++++
 arch/arm64/mm/fault.c                   |  4 +++
 drivers/firmware/efi/runtime-wrappers.c |  1 +
 5 files changed, 61 insertions(+), 5 deletions(-)

diff --git a/arch/arm64/include/asm/efi.h b/arch/arm64/include/asm/efi.h
index 1c408ec3c8b3a883..31d13a6001df49c4 100644
--- a/arch/arm64/include/asm/efi.h
+++ b/arch/arm64/include/asm/efi.h
@@ -14,8 +14,16 @@
 
 #ifdef CONFIG_EFI
 extern void efi_init(void);
+
+bool efi_runtime_fixup_exception(struct pt_regs *regs, const char *msg);
 #else
 #define efi_init()
+
+static inline
+bool efi_runtime_fixup_exception(struct pt_regs *regs, const char *msg)
+{
+	return false;
+}
 #endif
 
 int efi_create_mapping(struct mm_struct *mm, efi_memory_desc_t *md);
diff --git a/arch/arm64/kernel/efi-rt-wrapper.S b/arch/arm64/kernel/efi-rt-wrapper.S
index b2786b968fee68dd..ee0c6c719ab573b7 100644
--- a/arch/arm64/kernel/efi-rt-wrapper.S
+++ b/arch/arm64/kernel/efi-rt-wrapper.S
@@ -6,7 +6,7 @@
 #include <linux/linkage.h>
 
 SYM_FUNC_START(__efi_rt_asm_wrapper)
-	stp	x29, x30, [sp, #-32]!
+	stp	x29, x30, [sp, #-112]!
 	mov	x29, sp
 
 	/*
@@ -16,11 +16,20 @@ SYM_FUNC_START(__efi_rt_asm_wrapper)
 	 */
 	stp	x1, x18, [sp, #16]
 
+	/*
+	 * Preserve all callee saved registers and record the stack pointer
+	 * value in a per-CPU variable so we can recover from synchronous
+	 * exceptions occurring while running the firmware routines.
+	 */
+	stp	x19, x20, [sp, #32]
+	stp	x21, x22, [sp, #48]
+	stp	x23, x24, [sp, #64]
+	stp	x25, x26, [sp, #80]
+	stp	x27, x28, [sp, #96]
+
 	ldr_l	x16, efi_rt_stack_top
 	mov	sp, x16
-#ifdef CONFIG_SHADOW_CALL_STACK
-	str	x18, [sp, #-16]!
-#endif
+	stp	x18, x29, [sp, #-16]!
 
 	/*
 	 * We are lucky enough that no EFI runtime services take more than
@@ -38,7 +47,7 @@ SYM_FUNC_START(__efi_rt_asm_wrapper)
 	mov	sp, x29
 	ldp	x1, x2, [sp, #16]
 	cmp	x2, x18
-	ldp	x29, x30, [sp], #32
+	ldp	x29, x30, [sp], #112
 	b.ne	0f
 	ret
 0:
@@ -56,3 +65,15 @@ SYM_FUNC_START(__efi_rt_asm_wrapper)
 
 	b	efi_handle_corrupted_x18	// tail call
 SYM_FUNC_END(__efi_rt_asm_wrapper)
+
+SYM_CODE_START(__efi_rt_asm_recover)
+	mov	sp, x30
+
+	ldp	x19, x20, [sp, #32]
+	ldp	x21, x22, [sp, #48]
+	ldp	x23, x24, [sp, #64]
+	ldp	x25, x26, [sp, #80]
+	ldp	x27, x28, [sp, #96]
+	ldp	x29, x30, [sp], #112
+	ret
+SYM_CODE_END(__efi_rt_asm_recover)
diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
index 8cb2e005f8aca589..0169a669fde7544f 100644
--- a/arch/arm64/kernel/efi.c
+++ b/arch/arm64/kernel/efi.c
@@ -149,6 +149,28 @@ DEFINE_SPINLOCK(efi_rt_lock);
 
 asmlinkage u64 *efi_rt_stack_top __ro_after_init;
 
+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)
+		return false;
+
+	pr_err(FW_BUG "Unable to handle %s in EFI runtime service\n", msg);
+	add_taint(TAINT_FIRMWARE_WORKAROUND, LOCKDEP_STILL_OK);
+	clear_bit(EFI_RUNTIME_SERVICES, &efi.flags);
+
+	regs->regs[0]	= EFI_ABORTED;
+	regs->regs[30]	= efi_rt_stack_top[-1];
+	regs->pc	= (u64)__efi_rt_asm_recover;
+
+	if (IS_ENABLED(CONFIG_SHADOW_CALL_STACK))
+		regs->regs[18] = efi_rt_stack_top[-2];
+
+	return true;
+}
+
 /* required by the EFI spec */
 static_assert(THREAD_SIZE >= SZ_8K);
 
diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
index 5b391490e045be91..3e9cf9826417a434 100644
--- a/arch/arm64/mm/fault.c
+++ b/arch/arm64/mm/fault.c
@@ -30,6 +30,7 @@
 #include <asm/bug.h>
 #include <asm/cmpxchg.h>
 #include <asm/cpufeature.h>
+#include <asm/efi.h>
 #include <asm/exception.h>
 #include <asm/daifflags.h>
 #include <asm/debug-monitors.h>
@@ -391,6 +392,9 @@ static void __do_kernel_fault(unsigned long addr, unsigned long esr,
 		msg = "paging request";
 	}
 
+	if (efi_runtime_fixup_exception(regs, msg))
+		return;
+
 	die_kernel_fault(msg, addr, esr, regs);
 }
 
diff --git a/drivers/firmware/efi/runtime-wrappers.c b/drivers/firmware/efi/runtime-wrappers.c
index f3e54f6616f02475..7feee3d9c2bfbeec 100644
--- a/drivers/firmware/efi/runtime-wrappers.c
+++ b/drivers/firmware/efi/runtime-wrappers.c
@@ -83,6 +83,7 @@ struct efi_runtime_work efi_rts_work;
 	else								\
 		pr_err("Failed to queue work to efi_rts_wq.\n");	\
 									\
+	WARN_ON_ONCE(efi_rts_work.status == EFI_ABORTED);		\
 exit:									\
 	efi_rts_work.efi_rts_id = EFI_NONE;				\
 	efi_rts_work.status;						\
-- 
2.35.1

[PATCH 2/2] arm64: efi: Recover from synchronous exceptions occurring in firmware

[Ard Biesheuvel]

Unlike x86, which has machinery to deal with page faults that occur
during the execution of EFI runtime services, arm64 has nothing like
that, and a synchronous exception raised by firmware code brings down
the whole system.

With more EFI based systems appearing that were not built to run Linux
(such as the Windows-on-ARM laptops based on Qualcomm SOCs), as well as
the introduction of PRM (platform specific firmware routines that are
callable just like EFI runtime services), we are more likely to run into
issues of this sort, and it is much more likely that we can identify and
work around such issues if they don't bring down the system entirely.

Since we already use a EFI runtime services call wrapper in assembler,
we can quite easily add some code that captures the execution state at
the point where the call is made, allowing us to revert to this state
and proceed execution if the call triggered a synchronous exception.

Given that the kernel and the firmware don't share any data structures
that could end up in an indeterminate state, we can happily continue
running, as long as we mark the EFI runtime services as unavailable from
that point on.

Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
Acked-by: Catalin Marinas <catalin.marinas@arm.com>
---
 arch/arm64/include/asm/efi.h            |  8 +++++
 arch/arm64/kernel/efi-rt-wrapper.S      | 31 ++++++++++++++++----
 arch/arm64/kernel/efi.c                 | 22 ++++++++++++++
 arch/arm64/mm/fault.c                   |  4 +++
 drivers/firmware/efi/runtime-wrappers.c |  1 +
 5 files changed, 61 insertions(+), 5 deletions(-)

diff --git a/arch/arm64/include/asm/efi.h b/arch/arm64/include/asm/efi.h
index 1c408ec3c8b3a883..31d13a6001df49c4 100644
--- a/arch/arm64/include/asm/efi.h
+++ b/arch/arm64/include/asm/efi.h
@@ -14,8 +14,16 @@
 
 #ifdef CONFIG_EFI
 extern void efi_init(void);
+
+bool efi_runtime_fixup_exception(struct pt_regs *regs, const char *msg);
 #else
 #define efi_init()
+
+static inline
+bool efi_runtime_fixup_exception(struct pt_regs *regs, const char *msg)
+{
+	return false;
+}
 #endif
 
 int efi_create_mapping(struct mm_struct *mm, efi_memory_desc_t *md);
diff --git a/arch/arm64/kernel/efi-rt-wrapper.S b/arch/arm64/kernel/efi-rt-wrapper.S
index b2786b968fee68dd..ee0c6c719ab573b7 100644
--- a/arch/arm64/kernel/efi-rt-wrapper.S
+++ b/arch/arm64/kernel/efi-rt-wrapper.S
@@ -6,7 +6,7 @@
 #include <linux/linkage.h>
 
 SYM_FUNC_START(__efi_rt_asm_wrapper)
-	stp	x29, x30, [sp, #-32]!
+	stp	x29, x30, [sp, #-112]!
 	mov	x29, sp
 
 	/*
@@ -16,11 +16,20 @@ SYM_FUNC_START(__efi_rt_asm_wrapper)
 	 */
 	stp	x1, x18, [sp, #16]
 
+	/*
+	 * Preserve all callee saved registers and record the stack pointer
+	 * value in a per-CPU variable so we can recover from synchronous
+	 * exceptions occurring while running the firmware routines.
+	 */
+	stp	x19, x20, [sp, #32]
+	stp	x21, x22, [sp, #48]
+	stp	x23, x24, [sp, #64]
+	stp	x25, x26, [sp, #80]
+	stp	x27, x28, [sp, #96]
+
 	ldr_l	x16, efi_rt_stack_top
 	mov	sp, x16
-#ifdef CONFIG_SHADOW_CALL_STACK
-	str	x18, [sp, #-16]!
-#endif
+	stp	x18, x29, [sp, #-16]!
 
 	/*
 	 * We are lucky enough that no EFI runtime services take more than
@@ -38,7 +47,7 @@ SYM_FUNC_START(__efi_rt_asm_wrapper)
 	mov	sp, x29
 	ldp	x1, x2, [sp, #16]
 	cmp	x2, x18
-	ldp	x29, x30, [sp], #32
+	ldp	x29, x30, [sp], #112
 	b.ne	0f
 	ret
 0:
@@ -56,3 +65,15 @@ SYM_FUNC_START(__efi_rt_asm_wrapper)
 
 	b	efi_handle_corrupted_x18	// tail call
 SYM_FUNC_END(__efi_rt_asm_wrapper)
+
+SYM_CODE_START(__efi_rt_asm_recover)
+	mov	sp, x30
+
+	ldp	x19, x20, [sp, #32]
+	ldp	x21, x22, [sp, #48]
+	ldp	x23, x24, [sp, #64]
+	ldp	x25, x26, [sp, #80]
+	ldp	x27, x28, [sp, #96]
+	ldp	x29, x30, [sp], #112
+	ret
+SYM_CODE_END(__efi_rt_asm_recover)
diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
index 8cb2e005f8aca589..0169a669fde7544f 100644
--- a/arch/arm64/kernel/efi.c
+++ b/arch/arm64/kernel/efi.c
@@ -149,6 +149,28 @@ DEFINE_SPINLOCK(efi_rt_lock);
 
 asmlinkage u64 *efi_rt_stack_top __ro_after_init;
 
+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)
+		return false;
+
+	pr_err(FW_BUG "Unable to handle %s in EFI runtime service\n", msg);
+	add_taint(TAINT_FIRMWARE_WORKAROUND, LOCKDEP_STILL_OK);
+	clear_bit(EFI_RUNTIME_SERVICES, &efi.flags);
+
+	regs->regs[0]	= EFI_ABORTED;
+	regs->regs[30]	= efi_rt_stack_top[-1];
+	regs->pc	= (u64)__efi_rt_asm_recover;
+
+	if (IS_ENABLED(CONFIG_SHADOW_CALL_STACK))
+		regs->regs[18] = efi_rt_stack_top[-2];
+
+	return true;
+}
+
 /* required by the EFI spec */
 static_assert(THREAD_SIZE >= SZ_8K);
 
diff --git a/arch/arm64/mm/fault.c b/arch/arm64/mm/fault.c
index 5b391490e045be91..3e9cf9826417a434 100644
--- a/arch/arm64/mm/fault.c
+++ b/arch/arm64/mm/fault.c
@@ -30,6 +30,7 @@
 #include <asm/bug.h>
 #include <asm/cmpxchg.h>
 #include <asm/cpufeature.h>
+#include <asm/efi.h>
 #include <asm/exception.h>
 #include <asm/daifflags.h>
 #include <asm/debug-monitors.h>
@@ -391,6 +392,9 @@ static void __do_kernel_fault(unsigned long addr, unsigned long esr,
 		msg = "paging request";
 	}
 
+	if (efi_runtime_fixup_exception(regs, msg))
+		return;
+
 	die_kernel_fault(msg, addr, esr, regs);
 }
 
diff --git a/drivers/firmware/efi/runtime-wrappers.c b/drivers/firmware/efi/runtime-wrappers.c
index f3e54f6616f02475..7feee3d9c2bfbeec 100644
--- a/drivers/firmware/efi/runtime-wrappers.c
+++ b/drivers/firmware/efi/runtime-wrappers.c
@@ -83,6 +83,7 @@ struct efi_runtime_work efi_rts_work;
 	else								\
 		pr_err("Failed to queue work to efi_rts_wq.\n");	\
 									\
+	WARN_ON_ONCE(efi_rts_work.status == EFI_ABORTED);		\
 exit:									\
 	efi_rts_work.efi_rts_id = EFI_NONE;				\
 	efi_rts_work.status;						\
-- 
2.35.1


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

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

[Mark Rutland]

Hi Ard,

One drive-by comment below...

On Mon, Dec 05, 2022 at 09:12:09PM +0100, 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 ++++++++++++++++++++

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.

Thanks,
Mark.

>  3 files changed, 40 insertions(+), 1 deletion(-)
> 
> 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
> 

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

[Mark Rutland]

Hi Ard,

One drive-by comment below...

On Mon, Dec 05, 2022 at 09:12:09PM +0100, 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 ++++++++++++++++++++

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.

Thanks,
Mark.

>  3 files changed, 40 insertions(+), 1 deletion(-)
> 
> 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
> 

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

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

[Ard Biesheuvel]

On Fri, 9 Dec 2022 at 11:51, Mark Rutland <mark.rutland@arm.com> wrote:
>
> Hi Ard,
>
> One drive-by comment below...
>
> On Mon, Dec 05, 2022 at 09:12:09PM +0100, 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 ++++++++++++++++++++
>
> 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.
>

Thanks, I'll look into that.

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

[Ard Biesheuvel]

On Fri, 9 Dec 2022 at 11:51, Mark Rutland <mark.rutland@arm.com> wrote:
>
> Hi Ard,
>
> One drive-by comment below...
>
> On Mon, Dec 05, 2022 at 09:12:09PM +0100, 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 ++++++++++++++++++++
>
> 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.
>

Thanks, I'll look into that.

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

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

[Lee Jones]

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 [李琼斯]

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

[Lee Jones]

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 [李琼斯]

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

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

[Ard Biesheuvel]

On Wed, 4 Jan 2023 at 11:40, Lee Jones <lee@kernel.org> wrote:
>
> 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?
>

Thanks for the reminder.

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

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

[Ard Biesheuvel]

On Wed, 4 Jan 2023 at 11:40, Lee Jones <lee@kernel.org> wrote:
>
> 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?
>

Thanks for the reminder.

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

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

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

[Lee Jones]

On Wed, 04 Jan 2023, Ard Biesheuvel wrote:

> On Wed, 4 Jan 2023 at 11:40, Lee Jones <lee@kernel.org> wrote:
> >
> > 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?
> >
> 
> Thanks for the reminder.
> 
> Only patch #1 is needed. It should be applied to v5.10 and later.

Perfect, thanks Ard.

-- 
Lee Jones [李琼斯]

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

[Lee Jones]

On Wed, 04 Jan 2023, Ard Biesheuvel wrote:

> On Wed, 4 Jan 2023 at 11:40, Lee Jones <lee@kernel.org> wrote:
> >
> > 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?
> >
> 
> Thanks for the reminder.
> 
> Only patch #1 is needed. It should be applied to v5.10 and later.

Perfect, thanks Ard.

-- 
Lee Jones [李琼斯]

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

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

[Greg KH]

On Wed, Jan 04, 2023 at 02:42:30PM +0000, Lee Jones wrote:
> On Wed, 04 Jan 2023, Ard Biesheuvel wrote:
> 
> > On Wed, 4 Jan 2023 at 11:40, Lee Jones <lee@kernel.org> wrote:
> > >
> > > 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?
> > >
> > 
> > Thanks for the reminder.
> > 
> > Only patch #1 is needed. It should be applied to v5.10 and later.
> 
> Perfect, thanks Ard.

Now queued up, thanks.

greg k-h

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

[Greg KH]

On Wed, Jan 04, 2023 at 02:42:30PM +0000, Lee Jones wrote:
> On Wed, 04 Jan 2023, Ard Biesheuvel wrote:
> 
> > On Wed, 4 Jan 2023 at 11:40, Lee Jones <lee@kernel.org> wrote:
> > >
> > > 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?
> > >
> > 
> > Thanks for the reminder.
> > 
> > Only patch #1 is needed. It should be applied to v5.10 and later.
> 
> Perfect, thanks Ard.

Now queued up, thanks.

greg k-h

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

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

[Mark Rutland]

On Wed, Jan 04, 2023 at 02:56:19PM +0100, Ard Biesheuvel wrote:
> On Wed, 4 Jan 2023 at 11:40, Lee Jones <lee@kernel.org> wrote:
> >
> > 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?
> >
> 
> 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.

Mark.

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

[Mark Rutland]

On Wed, Jan 04, 2023 at 02:56:19PM +0100, Ard Biesheuvel wrote:
> On Wed, 4 Jan 2023 at 11:40, Lee Jones <lee@kernel.org> wrote:
> >
> > 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?
> >
> 
> 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.

Mark.

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

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

[Ard Biesheuvel]

On Wed, 4 Jan 2023 at 17:13, Mark Rutland <mark.rutland@arm.com> wrote:
>
> On Wed, Jan 04, 2023 at 02:56:19PM +0100, Ard Biesheuvel wrote:
> > On Wed, 4 Jan 2023 at 11:40, Lee Jones <lee@kernel.org> wrote:
> > >
> > > 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?
> > >
> >
> > 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

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

[Ard Biesheuvel]

On Wed, 4 Jan 2023 at 17:13, Mark Rutland <mark.rutland@arm.com> wrote:
>
> On Wed, Jan 04, 2023 at 02:56:19PM +0100, Ard Biesheuvel wrote:
> > On Wed, 4 Jan 2023 at 11:40, Lee Jones <lee@kernel.org> wrote:
> > >
> > > 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?
> > >
> >
> > 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

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

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

[Mark Rutland]

On Wed, Jan 04, 2023 at 05:15:34PM +0100, Ard Biesheuvel wrote:
> On Wed, 4 Jan 2023 at 17:13, Mark Rutland <mark.rutland@arm.com> wrote:
> >
> > On Wed, Jan 04, 2023 at 02:56:19PM +0100, Ard Biesheuvel wrote:
> > > On Wed, 4 Jan 2023 at 11:40, Lee Jones <lee@kernel.org> wrote:
> > > >
> > > > 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?
> > > >
> > >
> > > 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

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.

Thanks,
Mark.

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

[Mark Rutland]

On Wed, Jan 04, 2023 at 05:15:34PM +0100, Ard Biesheuvel wrote:
> On Wed, 4 Jan 2023 at 17:13, Mark Rutland <mark.rutland@arm.com> wrote:
> >
> > On Wed, Jan 04, 2023 at 02:56:19PM +0100, Ard Biesheuvel wrote:
> > > On Wed, 4 Jan 2023 at 11:40, Lee Jones <lee@kernel.org> wrote:
> > > >
> > > > 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?
> > > >
> > >
> > > 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

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.

Thanks,
Mark.

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

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

[Ard Biesheuvel]

On Wed, 4 Jan 2023 at 17:30, Mark Rutland <mark.rutland@arm.com> wrote:
>
> On Wed, Jan 04, 2023 at 05:15:34PM +0100, Ard Biesheuvel wrote:
> > On Wed, 4 Jan 2023 at 17:13, Mark Rutland <mark.rutland@arm.com> wrote:
> > >
> > > On Wed, Jan 04, 2023 at 02:56:19PM +0100, Ard Biesheuvel wrote:
> > > > On Wed, 4 Jan 2023 at 11:40, Lee Jones <lee@kernel.org> wrote:
> > > > >
> > > > > 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?
> > > > >
> > > >
> > > > 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
>
> 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.
>

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.

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

[Ard Biesheuvel]

On Wed, 4 Jan 2023 at 17:30, Mark Rutland <mark.rutland@arm.com> wrote:
>
> On Wed, Jan 04, 2023 at 05:15:34PM +0100, Ard Biesheuvel wrote:
> > On Wed, 4 Jan 2023 at 17:13, Mark Rutland <mark.rutland@arm.com> wrote:
> > >
> > > On Wed, Jan 04, 2023 at 02:56:19PM +0100, Ard Biesheuvel wrote:
> > > > On Wed, 4 Jan 2023 at 11:40, Lee Jones <lee@kernel.org> wrote:
> > > > >
> > > > > 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?
> > > > >
> > > >
> > > > 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
>
> 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.
>

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.

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

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

[Greg KH]

On Wed, Jan 04, 2023 at 05:32:18PM +0100, Ard Biesheuvel wrote:
> On Wed, 4 Jan 2023 at 17:30, Mark Rutland <mark.rutland@arm.com> wrote:
> >
> > On Wed, Jan 04, 2023 at 05:15:34PM +0100, Ard Biesheuvel wrote:
> > > On Wed, 4 Jan 2023 at 17:13, Mark Rutland <mark.rutland@arm.com> wrote:
> > > >
> > > > On Wed, Jan 04, 2023 at 02:56:19PM +0100, Ard Biesheuvel wrote:
> > > > > On Wed, 4 Jan 2023 at 11:40, Lee Jones <lee@kernel.org> wrote:
> > > > > >
> > > > > > 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?
> > > > > >
> > > > >
> > > > > 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
> >
> > 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.
> >
> 
> 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.

greg k-h

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

[Greg KH]

On Wed, Jan 04, 2023 at 05:32:18PM +0100, Ard Biesheuvel wrote:
> On Wed, 4 Jan 2023 at 17:30, Mark Rutland <mark.rutland@arm.com> wrote:
> >
> > On Wed, Jan 04, 2023 at 05:15:34PM +0100, Ard Biesheuvel wrote:
> > > On Wed, 4 Jan 2023 at 17:13, Mark Rutland <mark.rutland@arm.com> wrote:
> > > >
> > > > On Wed, Jan 04, 2023 at 02:56:19PM +0100, Ard Biesheuvel wrote:
> > > > > On Wed, 4 Jan 2023 at 11:40, Lee Jones <lee@kernel.org> wrote:
> > > > > >
> > > > > > 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?
> > > > > >
> > > > >
> > > > > 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
> >
> > 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.
> >
> 
> 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.

greg k-h

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

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

[Mark Rutland]

On Wed, Jan 04, 2023 at 05:32:18PM +0100, Ard Biesheuvel wrote:
> On Wed, 4 Jan 2023 at 17:30, Mark Rutland <mark.rutland@arm.com> wrote:
> >
> > On Wed, Jan 04, 2023 at 05:15:34PM +0100, Ard Biesheuvel wrote:
> > > On Wed, 4 Jan 2023 at 17:13, Mark Rutland <mark.rutland@arm.com> wrote:
> > > >
> > > > On Wed, Jan 04, 2023 at 02:56:19PM +0100, Ard Biesheuvel wrote:
> > > > > On Wed, 4 Jan 2023 at 11:40, Lee Jones <lee@kernel.org> wrote:
> > > > > >
> > > > > > 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?
> > > > > >
> > > > >
> > > > > 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
> >
> > 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.
> 
> 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/20230104174433.1259428-1-ardb@kernel.org

   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?

Thanks,
Mark.

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

[Mark Rutland]

On Wed, Jan 04, 2023 at 05:32:18PM +0100, Ard Biesheuvel wrote:
> On Wed, 4 Jan 2023 at 17:30, Mark Rutland <mark.rutland@arm.com> wrote:
> >
> > On Wed, Jan 04, 2023 at 05:15:34PM +0100, Ard Biesheuvel wrote:
> > > On Wed, 4 Jan 2023 at 17:13, Mark Rutland <mark.rutland@arm.com> wrote:
> > > >
> > > > On Wed, Jan 04, 2023 at 02:56:19PM +0100, Ard Biesheuvel wrote:
> > > > > On Wed, 4 Jan 2023 at 11:40, Lee Jones <lee@kernel.org> wrote:
> > > > > >
> > > > > > 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?
> > > > > >
> > > > >
> > > > > 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
> >
> > 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.
> 
> 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/20230104174433.1259428-1-ardb@kernel.org

   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?

Thanks,
Mark.

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

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

[Ard Biesheuvel]

On Thu, 5 Jan 2023 at 13:56, Mark Rutland <mark.rutland@arm.com> wrote:
>
> On Wed, Jan 04, 2023 at 05:32:18PM +0100, Ard Biesheuvel wrote:
> > On Wed, 4 Jan 2023 at 17:30, Mark Rutland <mark.rutland@arm.com> wrote:
> > >
> > > On Wed, Jan 04, 2023 at 05:15:34PM +0100, Ard Biesheuvel wrote:
> > > > On Wed, 4 Jan 2023 at 17:13, Mark Rutland <mark.rutland@arm.com> wrote:
> > > > >
> > > > > On Wed, Jan 04, 2023 at 02:56:19PM +0100, Ard Biesheuvel wrote:
> > > > > > On Wed, 4 Jan 2023 at 11:40, Lee Jones <lee@kernel.org> wrote:
> > > > > > >
> > > > > > > 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?
> > > > > > >
> > > > > >
> > > > > > 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
> > >
> > > 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.
> >
> > 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.
>

The lock by itself does nothing useful - it is only needed because
there is now a single stack that is shared by all callers of EFI
runtime code. The existing ones are serialized already, but ACPI may
invoke efi_call_virt_pointer() as well, which is why the additional
spinlock is required for completeness. However, that ACPI feature is
relatively recent (and I am not aware of any arm64 systems that
actually implement it in their firmware)

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

The reason for the backport is that it also allows us to stash the
shadow call stack pointer elsewhere, as storing it on the normal stack
as we do currently defeats the purpose.

> 2) Backport this *but* also backport the follow-up fixes from your other series:
>    https://lore.kernel.org/r/20230104174433.1259428-1-ardb@kernel.org
>
>    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?
>

This is when the shadow call stack was introduced. Apologies for not
making this clearer in the commit log.

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

What I had in mind (but did not communicate clearly) is to backport
the patch that introduces the new stack to v5.10 and later, and to
backport the patch that adds the stacktrace declaration as well, but
separately (and probably not all the way back to v5.10)

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

[Ard Biesheuvel]

On Thu, 5 Jan 2023 at 13:56, Mark Rutland <mark.rutland@arm.com> wrote:
>
> On Wed, Jan 04, 2023 at 05:32:18PM +0100, Ard Biesheuvel wrote:
> > On Wed, 4 Jan 2023 at 17:30, Mark Rutland <mark.rutland@arm.com> wrote:
> > >
> > > On Wed, Jan 04, 2023 at 05:15:34PM +0100, Ard Biesheuvel wrote:
> > > > On Wed, 4 Jan 2023 at 17:13, Mark Rutland <mark.rutland@arm.com> wrote:
> > > > >
> > > > > On Wed, Jan 04, 2023 at 02:56:19PM +0100, Ard Biesheuvel wrote:
> > > > > > On Wed, 4 Jan 2023 at 11:40, Lee Jones <lee@kernel.org> wrote:
> > > > > > >
> > > > > > > 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?
> > > > > > >
> > > > > >
> > > > > > 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
> > >
> > > 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.
> >
> > 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.
>

The lock by itself does nothing useful - it is only needed because
there is now a single stack that is shared by all callers of EFI
runtime code. The existing ones are serialized already, but ACPI may
invoke efi_call_virt_pointer() as well, which is why the additional
spinlock is required for completeness. However, that ACPI feature is
relatively recent (and I am not aware of any arm64 systems that
actually implement it in their firmware)

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

The reason for the backport is that it also allows us to stash the
shadow call stack pointer elsewhere, as storing it on the normal stack
as we do currently defeats the purpose.

> 2) Backport this *but* also backport the follow-up fixes from your other series:
>    https://lore.kernel.org/r/20230104174433.1259428-1-ardb@kernel.org
>
>    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?
>

This is when the shadow call stack was introduced. Apologies for not
making this clearer in the commit log.

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

What I had in mind (but did not communicate clearly) is to backport
the patch that introduces the new stack to v5.10 and later, and to
backport the patch that adds the stacktrace declaration as well, but
separately (and probably not all the way back to v5.10)

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

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

[Lee Jones]

On Thu, 05 Jan 2023, Greg KH wrote:

> On Wed, Jan 04, 2023 at 05:32:18PM +0100, Ard Biesheuvel wrote:
> > On Wed, 4 Jan 2023 at 17:30, Mark Rutland <mark.rutland@arm.com> wrote:
> > >
> > > On Wed, Jan 04, 2023 at 05:15:34PM +0100, Ard Biesheuvel wrote:
> > > > On Wed, 4 Jan 2023 at 17:13, Mark Rutland <mark.rutland@arm.com> wrote:
> > > > >
> > > > > On Wed, Jan 04, 2023 at 02:56:19PM +0100, Ard Biesheuvel wrote:
> > > > > > On Wed, 4 Jan 2023 at 11:40, Lee Jones <lee@kernel.org> wrote:
> > > > > > >
> > > > > > > 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?
> > > > > > >
> > > > > >
> > > > > > 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
> > >
> > > 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.
> > >
> > 
> > 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.

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?

Thank you.

-- 
Lee Jones [李琼斯]

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

[Lee Jones]

On Thu, 05 Jan 2023, Greg KH wrote:

> On Wed, Jan 04, 2023 at 05:32:18PM +0100, Ard Biesheuvel wrote:
> > On Wed, 4 Jan 2023 at 17:30, Mark Rutland <mark.rutland@arm.com> wrote:
> > >
> > > On Wed, Jan 04, 2023 at 05:15:34PM +0100, Ard Biesheuvel wrote:
> > > > On Wed, 4 Jan 2023 at 17:13, Mark Rutland <mark.rutland@arm.com> wrote:
> > > > >
> > > > > On Wed, Jan 04, 2023 at 02:56:19PM +0100, Ard Biesheuvel wrote:
> > > > > > On Wed, 4 Jan 2023 at 11:40, Lee Jones <lee@kernel.org> wrote:
> > > > > > >
> > > > > > > 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?
> > > > > > >
> > > > > >
> > > > > > 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
> > >
> > > 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.
> > >
> > 
> > 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.

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?

Thank you.

-- 
Lee Jones [李琼斯]

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

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

[Greg KH]

On Tue, Jan 17, 2023 at 04:56:19PM +0000, Lee Jones wrote:
> On Thu, 05 Jan 2023, Greg KH wrote:
> 
> > On Wed, Jan 04, 2023 at 05:32:18PM +0100, Ard Biesheuvel wrote:
> > > On Wed, 4 Jan 2023 at 17:30, Mark Rutland <mark.rutland@arm.com> wrote:
> > > >
> > > > On Wed, Jan 04, 2023 at 05:15:34PM +0100, Ard Biesheuvel wrote:
> > > > > On Wed, 4 Jan 2023 at 17:13, Mark Rutland <mark.rutland@arm.com> wrote:
> > > > > >
> > > > > > On Wed, Jan 04, 2023 at 02:56:19PM +0100, Ard Biesheuvel wrote:
> > > > > > > On Wed, 4 Jan 2023 at 11:40, Lee Jones <lee@kernel.org> wrote:
> > > > > > > >
> > > > > > > > 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?
> > > > > > > >
> > > > > > >
> > > > > > > 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
> > > >
> > > > 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.
> > > >
> > > 
> > > 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.
> 
> 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?

Now queued up for 5.10.y and newer.

greg k-h

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

[Greg KH]

On Tue, Jan 17, 2023 at 04:56:19PM +0000, Lee Jones wrote:
> On Thu, 05 Jan 2023, Greg KH wrote:
> 
> > On Wed, Jan 04, 2023 at 05:32:18PM +0100, Ard Biesheuvel wrote:
> > > On Wed, 4 Jan 2023 at 17:30, Mark Rutland <mark.rutland@arm.com> wrote:
> > > >
> > > > On Wed, Jan 04, 2023 at 05:15:34PM +0100, Ard Biesheuvel wrote:
> > > > > On Wed, 4 Jan 2023 at 17:13, Mark Rutland <mark.rutland@arm.com> wrote:
> > > > > >
> > > > > > On Wed, Jan 04, 2023 at 02:56:19PM +0100, Ard Biesheuvel wrote:
> > > > > > > On Wed, 4 Jan 2023 at 11:40, Lee Jones <lee@kernel.org> wrote:
> > > > > > > >
> > > > > > > > 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?
> > > > > > > >
> > > > > > >
> > > > > > > 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
> > > >
> > > > 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.
> > > >
> > > 
> > > 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.
> 
> 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?

Now queued up for 5.10.y and newer.

greg k-h

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