[PATCH] arm64: efi: Make runtime service wrapper more robust

[PATCH] arm64: efi: Make runtime service wrapper more robust

[Ard Biesheuvel]

Prevent abuse of the runtime service wrapper code by avoiding restoring
the shadow call stack pointer from the ordinary stack, or the stack
pointer itself from a GPR. Also, given that the exception recovery
routine is never called in an ordinary way, it doesn't need BTI landing
pads so it can be SYM_CODE rather than SYM_FUNC.

Cc: Sami Tolvanen <samitolvanen@google.com>
Cc: Kees Cook <keescook@chromium.org>
Signed-off-by: Ard Biesheuvel <ardb@kernel.org>
---
 arch/arm64/kernel/efi-rt-wrapper.S | 16 +++++++++-------
 arch/arm64/kernel/efi.c            |  6 +++++-
 2 files changed, 14 insertions(+), 8 deletions(-)

diff --git a/arch/arm64/kernel/efi-rt-wrapper.S b/arch/arm64/kernel/efi-rt-wrapper.S
index 67babd5f04c27c7a..afd3e81e1b627b87 100644
--- a/arch/arm64/kernel/efi-rt-wrapper.S
+++ b/arch/arm64/kernel/efi-rt-wrapper.S
@@ -28,7 +28,7 @@ SYM_FUNC_START(__efi_rt_asm_wrapper)
 	stp	x27, x28, [sp, #96]
 
 	adr_this_cpu	x8, __efi_rt_asm_recover_sp, x9
-	str		x29, [x8]
+	stp		x29, x18, [x8]
 
 	/*
 	 * We are lucky enough that no EFI runtime services take more than
@@ -56,15 +56,17 @@ 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_this_cpu	x18, __efi_rt_asm_recover_sp + 8, x9
+#endif
+
 	b	efi_handle_corrupted_x18	// tail call
 SYM_FUNC_END(__efi_rt_asm_wrapper)
 
-SYM_FUNC_START(__efi_rt_asm_recover)
-	ldr_this_cpu	x8, __efi_rt_asm_recover_sp, x9
-	mov		sp, x8
+SYM_CODE_START(__efi_rt_asm_recover)
+	mov	sp, x30
 
-	ldp	x0,  x18, [sp, #16]
+	ldr	x0, [sp, #16]
 	ldp	x19, x20, [sp, #32]
 	ldp	x21, x22, [sp, #48]
 	ldp	x23, x24, [sp, #64]
@@ -73,4 +75,4 @@ SYM_FUNC_START(__efi_rt_asm_recover)
 	ldp	x29, x30, [sp], #112
 
 	b	efi_handle_runtime_exception
-SYM_FUNC_END(__efi_rt_asm_recover)
+SYM_CODE_END(__efi_rt_asm_recover)
diff --git a/arch/arm64/kernel/efi.c b/arch/arm64/kernel/efi.c
index 8d36e66a6e64cdaa..db7bdce1c7da578b 100644
--- a/arch/arm64/kernel/efi.c
+++ b/arch/arm64/kernel/efi.c
@@ -130,7 +130,7 @@ asmlinkage efi_status_t efi_handle_corrupted_x18(efi_status_t s, const char *f)
 	return s;
 }
 
-asmlinkage DEFINE_PER_CPU(u64, __efi_rt_asm_recover_sp);
+asmlinkage DEFINE_PER_CPU(u64[2], __efi_rt_asm_recover_sp);
 
 asmlinkage efi_status_t __efi_rt_asm_recover(void);
 
@@ -151,6 +151,10 @@ bool efi_runtime_fixup_exception(struct pt_regs *regs, const char *msg)
 	add_taint(TAINT_FIRMWARE_WORKAROUND, LOCKDEP_STILL_OK);
 	dump_stack();
 
+	regs->regs[30] = __this_cpu_read(__efi_rt_asm_recover_sp[0]);
+#ifdef CONFIG_SHADOW_CALL_STACK
+	regs->regs[18] = __this_cpu_read(__efi_rt_asm_recover_sp[1]);
+#endif
 	regs->pc = (u64)__efi_rt_asm_recover;
 	return true;
 }
-- 
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] arm64: efi: Make runtime service wrapper more robust

[Kees Cook]

On Mon, Nov 28, 2022 at 10:49:39AM +0100, Ard Biesheuvel wrote:
> Prevent abuse of the runtime service wrapper code by avoiding restoring
> the shadow call stack pointer from the ordinary stack, or the stack
> pointer itself from a GPR. Also, given that the exception recovery
> routine is never called in an ordinary way, it doesn't need BTI landing
> pads so it can be SYM_CODE rather than SYM_FUNC.

Does this mean x18 is now being spilled to the stack? (Do we already
spill it in other places?)

-- 
Kees Cook

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

Re: [PATCH] arm64: efi: Make runtime service wrapper more robust

[Ard Biesheuvel]

On Fri, 2 Dec 2022 at 00:45, Kees Cook <keescook@chromium.org> wrote:
>
> On Mon, Nov 28, 2022 at 10:49:39AM +0100, Ard Biesheuvel wrote:
> > Prevent abuse of the runtime service wrapper code by avoiding restoring
> > the shadow call stack pointer from the ordinary stack, or the stack
> > pointer itself from a GPR. Also, given that the exception recovery
> > routine is never called in an ordinary way, it doesn't need BTI landing
> > pads so it can be SYM_CODE rather than SYM_FUNC.
>
> Does this mean x18 is now being spilled to the stack? (Do we already
> spill it in other places?)
>

I've found a better way of addressing this, by moving this code out of
the kernel .text mapping entirely, and only mapping it executable in
the EFI page tables (which are only active while a runtime service
call is in progress, and only on a single CPU running with preemption
disabled)

https://git.kernel.org/pub/scm/linux/kernel/git/efi/efi.git/commit/?id=47f68266d6ad94860c6cd9d2145cb91350b47e43

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

Re: [PATCH] arm64: efi: Make runtime service wrapper more robust

[Ard Biesheuvel]

On Fri, 2 Dec 2022 at 00:47, Ard Biesheuvel <ardb@kernel.org> wrote:
>
> On Fri, 2 Dec 2022 at 00:45, Kees Cook <keescook@chromium.org> wrote:
> >
> > On Mon, Nov 28, 2022 at 10:49:39AM +0100, Ard Biesheuvel wrote:
> > > Prevent abuse of the runtime service wrapper code by avoiding restoring
> > > the shadow call stack pointer from the ordinary stack, or the stack
> > > pointer itself from a GPR. Also, given that the exception recovery
> > > routine is never called in an ordinary way, it doesn't need BTI landing
> > > pads so it can be SYM_CODE rather than SYM_FUNC.
> >
> > Does this mean x18 is now being spilled to the stack? (Do we already
> > spill it in other places?)
> >
>
> I've found a better way of addressing this, by moving this code out of
> the kernel .text mapping entirely, and only mapping it executable in
> the EFI page tables (which are only active while a runtime service
> call is in progress, and only on a single CPU running with preemption
> disabled)
>
> https://git.kernel.org/pub/scm/linux/kernel/git/efi/efi.git/commit/?id=47f68266d6ad94860c6cd9d2145cb91350b47e43

And to answer your question: yes, x18 is currently spllled to the
stack in both of those routines. I've reverted the patch that added
the second one (which was only added this cycle). The other one needs
a fix going to -stable, so I'll backport the patch I quoted above once
it hits linus's tree.

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