diff mbox series

[v4,11/13] KVM: arm64: Improve CONFIG_CFI_CLANG error message

Message ID 20240529121251.1993135-12-ptosi@google.com (mailing list archive)
State New, archived
Headers show
Series KVM: arm64: Add support for hypervisor kCFI | expand

Commit Message

Pierre-Clément Tosi May 29, 2024, 12:12 p.m. UTC
For kCFI, the compiler encodes in the immediate of the BRK (which the
CPU places in ESR_ELx) the indices of the two registers it used to hold
(resp.) the function pointer and expected type. Therefore, the kCFI
handler must be able to parse the contents of the register file at the
point where the exception was triggered.

To achieve this, introduce a new hypervisor panic path that first stores
the CPU context in the per-CPU kvm_hyp_ctxt before calling (directly or
indirectly) hyp_panic() and execute it from all EL2 synchronous
exception handlers i.e.

- call it directly in host_el2_sync_vect (__kvm_hyp_host_vector, EL2t&h)
- call it directly in el2t_sync_invalid (__kvm_hyp_vector, EL2t)
- set ELR_EL2 to it in el2_sync (__kvm_hyp_vector, EL2h), which ERETs

Teach hyp_panic() to decode the kCFI ESR and extract the target and type
from the saved CPU context. In VHE, use that information to panic() with
a specialized error message. In nVHE, only report it if the host (EL1)
has access to the saved CPU context i.e. iff CONFIG_NVHE_EL2_DEBUG=y,
which aligns with the behavior of CONFIG_PROTECTED_NVHE_STACKTRACE.

Signed-off-by: Pierre-Clément Tosi <ptosi@google.com>
---
 arch/arm64/kvm/handle_exit.c            | 30 +++++++++++++++++++++++--
 arch/arm64/kvm/hyp/entry.S              | 24 +++++++++++++++++++-
 arch/arm64/kvm/hyp/hyp-entry.S          |  2 +-
 arch/arm64/kvm/hyp/include/hyp/switch.h |  4 ++--
 arch/arm64/kvm/hyp/nvhe/host.S          |  2 +-
 arch/arm64/kvm/hyp/vhe/switch.c         | 26 +++++++++++++++++++--
 6 files changed, 79 insertions(+), 9 deletions(-)

Comments

Will Deacon June 3, 2024, 2:48 p.m. UTC | #1
On Wed, May 29, 2024 at 01:12:17PM +0100, Pierre-Clément Tosi wrote:
> For kCFI, the compiler encodes in the immediate of the BRK (which the
> CPU places in ESR_ELx) the indices of the two registers it used to hold
> (resp.) the function pointer and expected type. Therefore, the kCFI
> handler must be able to parse the contents of the register file at the
> point where the exception was triggered.
> 
> To achieve this, introduce a new hypervisor panic path that first stores
> the CPU context in the per-CPU kvm_hyp_ctxt before calling (directly or
> indirectly) hyp_panic() and execute it from all EL2 synchronous
> exception handlers i.e.
> 
> - call it directly in host_el2_sync_vect (__kvm_hyp_host_vector, EL2t&h)
> - call it directly in el2t_sync_invalid (__kvm_hyp_vector, EL2t)
> - set ELR_EL2 to it in el2_sync (__kvm_hyp_vector, EL2h), which ERETs
> 
> Teach hyp_panic() to decode the kCFI ESR and extract the target and type
> from the saved CPU context. In VHE, use that information to panic() with
> a specialized error message. In nVHE, only report it if the host (EL1)
> has access to the saved CPU context i.e. iff CONFIG_NVHE_EL2_DEBUG=y,
> which aligns with the behavior of CONFIG_PROTECTED_NVHE_STACKTRACE.
> 
> Signed-off-by: Pierre-Clément Tosi <ptosi@google.com>
> ---
>  arch/arm64/kvm/handle_exit.c            | 30 +++++++++++++++++++++++--
>  arch/arm64/kvm/hyp/entry.S              | 24 +++++++++++++++++++-
>  arch/arm64/kvm/hyp/hyp-entry.S          |  2 +-
>  arch/arm64/kvm/hyp/include/hyp/switch.h |  4 ++--
>  arch/arm64/kvm/hyp/nvhe/host.S          |  2 +-
>  arch/arm64/kvm/hyp/vhe/switch.c         | 26 +++++++++++++++++++--
>  6 files changed, 79 insertions(+), 9 deletions(-)

This quite a lot of work just to print out some opaque type numbers
when CONFIG_NVHE_EL2_DEBUG=y. Is it really worth it? How would I use
this information to debug an otherwise undebuggable kcfi failure at EL2?

Will
Pierre-Clément Tosi June 4, 2024, 4:05 p.m. UTC | #2
On Mon, Jun 03, 2024 at 03:48:08PM +0100, Will Deacon wrote:
> On Wed, May 29, 2024 at 01:12:17PM +0100, Pierre-Clément Tosi wrote:
> > For kCFI, the compiler encodes in the immediate of the BRK (which the
> > CPU places in ESR_ELx) the indices of the two registers it used to hold
> > (resp.) the function pointer and expected type. Therefore, the kCFI
> > handler must be able to parse the contents of the register file at the
> > point where the exception was triggered.
> > 
> > To achieve this, introduce a new hypervisor panic path that first stores
> > the CPU context in the per-CPU kvm_hyp_ctxt before calling (directly or
> > indirectly) hyp_panic() and execute it from all EL2 synchronous
> > exception handlers i.e.
> > 
> > - call it directly in host_el2_sync_vect (__kvm_hyp_host_vector, EL2t&h)
> > - call it directly in el2t_sync_invalid (__kvm_hyp_vector, EL2t)
> > - set ELR_EL2 to it in el2_sync (__kvm_hyp_vector, EL2h), which ERETs
> > 
> > Teach hyp_panic() to decode the kCFI ESR and extract the target and type
> > from the saved CPU context. In VHE, use that information to panic() with
> > a specialized error message. In nVHE, only report it if the host (EL1)
> > has access to the saved CPU context i.e. iff CONFIG_NVHE_EL2_DEBUG=y,
> > which aligns with the behavior of CONFIG_PROTECTED_NVHE_STACKTRACE.
> > 
> > Signed-off-by: Pierre-Clément Tosi <ptosi@google.com>
> > ---
> >  arch/arm64/kvm/handle_exit.c            | 30 +++++++++++++++++++++++--
> >  arch/arm64/kvm/hyp/entry.S              | 24 +++++++++++++++++++-
> >  arch/arm64/kvm/hyp/hyp-entry.S          |  2 +-
> >  arch/arm64/kvm/hyp/include/hyp/switch.h |  4 ++--
> >  arch/arm64/kvm/hyp/nvhe/host.S          |  2 +-
> >  arch/arm64/kvm/hyp/vhe/switch.c         | 26 +++++++++++++++++++--
> >  6 files changed, 79 insertions(+), 9 deletions(-)
> 
> This quite a lot of work just to print out some opaque type numbers
> when CONFIG_NVHE_EL2_DEBUG=y. Is it really worth it? How would I use
> this information to debug an otherwise undebuggable kcfi failure at EL2?

The type ID alone might not be worth it but what about the target?

In my experience, non-malicious kCFI failures are often caused by an issue with
the callee, not the caller. Without this patch, only the source of the exception
is reported but, with it, the panic handler also prints the kCFI target (i.e.
value of the function pointer) as a symbol.

With the infrastructure for the target in place, it isn't much more work to also
report the type ID. Although it is rarely informative (as you noted), there are
some situations where it can still be useful e.g. if reported as zero and/or has
been corrupted and does not match its value from the ELF.
Will Deacon June 6, 2024, 4:22 p.m. UTC | #3
On Tue, Jun 04, 2024 at 05:05:59PM +0100, Pierre-Clément Tosi wrote:
> On Mon, Jun 03, 2024 at 03:48:08PM +0100, Will Deacon wrote:
> > On Wed, May 29, 2024 at 01:12:17PM +0100, Pierre-Clément Tosi wrote:
> > > For kCFI, the compiler encodes in the immediate of the BRK (which the
> > > CPU places in ESR_ELx) the indices of the two registers it used to hold
> > > (resp.) the function pointer and expected type. Therefore, the kCFI
> > > handler must be able to parse the contents of the register file at the
> > > point where the exception was triggered.
> > > 
> > > To achieve this, introduce a new hypervisor panic path that first stores
> > > the CPU context in the per-CPU kvm_hyp_ctxt before calling (directly or
> > > indirectly) hyp_panic() and execute it from all EL2 synchronous
> > > exception handlers i.e.
> > > 
> > > - call it directly in host_el2_sync_vect (__kvm_hyp_host_vector, EL2t&h)
> > > - call it directly in el2t_sync_invalid (__kvm_hyp_vector, EL2t)
> > > - set ELR_EL2 to it in el2_sync (__kvm_hyp_vector, EL2h), which ERETs
> > > 
> > > Teach hyp_panic() to decode the kCFI ESR and extract the target and type
> > > from the saved CPU context. In VHE, use that information to panic() with
> > > a specialized error message. In nVHE, only report it if the host (EL1)
> > > has access to the saved CPU context i.e. iff CONFIG_NVHE_EL2_DEBUG=y,
> > > which aligns with the behavior of CONFIG_PROTECTED_NVHE_STACKTRACE.
> > > 
> > > Signed-off-by: Pierre-Clément Tosi <ptosi@google.com>
> > > ---
> > >  arch/arm64/kvm/handle_exit.c            | 30 +++++++++++++++++++++++--
> > >  arch/arm64/kvm/hyp/entry.S              | 24 +++++++++++++++++++-
> > >  arch/arm64/kvm/hyp/hyp-entry.S          |  2 +-
> > >  arch/arm64/kvm/hyp/include/hyp/switch.h |  4 ++--
> > >  arch/arm64/kvm/hyp/nvhe/host.S          |  2 +-
> > >  arch/arm64/kvm/hyp/vhe/switch.c         | 26 +++++++++++++++++++--
> > >  6 files changed, 79 insertions(+), 9 deletions(-)
> > 
> > This quite a lot of work just to print out some opaque type numbers
> > when CONFIG_NVHE_EL2_DEBUG=y. Is it really worth it? How would I use
> > this information to debug an otherwise undebuggable kcfi failure at EL2?
> 
> The type ID alone might not be worth it but what about the target?
> 
> In my experience, non-malicious kCFI failures are often caused by an issue with
> the callee, not the caller. Without this patch, only the source of the exception
> is reported but, with it, the panic handler also prints the kCFI target (i.e.
> value of the function pointer) as a symbol.

I think it's less of an issue for EL2, as we don't have tonnes of
indirections, but I agree that the target is nice to have.

> With the infrastructure for the target in place, it isn't much more work to also
> report the type ID. Although it is rarely informative (as you noted), there are
> some situations where it can still be useful e.g. if reported as zero and/or has
> been corrupted and does not match its value from the ELF.

So looking at the implementation, I'm not a huge fan of saving off all
the GPRs and then relying on the stage-2 being disabled so that the host
can fish out the registers it cares about. I think I'd prefer to provide
the target as an additional argument to nvhe_hyp_panic_handler(), meaning
that we could even print the VA when CONFIG_NVHE_EL2_DEBUG is disabled.

But for now, I suggest we drop this patch along with the testing patches
because I think the rest of the series is nearly there and it's a useful
change on its own.

Will
diff mbox series

Patch

diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
index 69b08ac7322d..2fac3be3db00 100644
--- a/arch/arm64/kvm/handle_exit.c
+++ b/arch/arm64/kvm/handle_exit.c
@@ -26,6 +26,8 @@ 
 #define CREATE_TRACE_POINTS
 #include "trace_handle_exit.h"
 
+DECLARE_KVM_NVHE_PER_CPU(struct kvm_cpu_context, kvm_hyp_ctxt);
+
 typedef int (*exit_handle_fn)(struct kvm_vcpu *);
 
 static void kvm_handle_guest_serror(struct kvm_vcpu *vcpu, u64 esr)
@@ -417,10 +419,34 @@  static void print_nvhe_hyp_panic(const char *name, u64 panic_addr)
 		(void *)(panic_addr + kaslr_offset()));
 }
 
-static void kvm_nvhe_report_cfi_failure(u64 panic_addr)
+static void kvm_nvhe_report_cfi_target(struct user_pt_regs *regs, u64 esr,
+				       u64 hyp_offset)
+{
+	u64 va_mask = GENMASK_ULL(vabits_actual - 1, 0);
+	u8 type_idx = FIELD_GET(CFI_BRK_IMM_TYPE, esr);
+	u8 target_idx = FIELD_GET(CFI_BRK_IMM_TARGET, esr);
+	u32 expected_type = (u32)regs->regs[type_idx];
+	u64 target_addr = (regs->regs[target_idx] & va_mask) + hyp_offset;
+
+	kvm_err(" (target: [<%016llx>] %ps, expected type: 0x%08x)\n",
+		target_addr, (void *)(target_addr + kaslr_offset()),
+		expected_type);
+}
+
+static void kvm_nvhe_report_cfi_failure(u64 panic_addr, u64 esr, u64 hyp_offset)
 {
+	struct user_pt_regs *regs = NULL;
+
 	print_nvhe_hyp_panic("CFI failure", panic_addr);
 
+	if (IS_ENABLED(CONFIG_NVHE_EL2_DEBUG) || !is_protected_kvm_enabled())
+		regs = &this_cpu_ptr_nvhe_sym(kvm_hyp_ctxt)->regs;
+
+	if (regs)
+		kvm_nvhe_report_cfi_target(regs, esr, hyp_offset);
+	else
+		kvm_err(" (no target information: !CONFIG_NVHE_EL2_DEBUG)\n");
+
 	if (IS_ENABLED(CONFIG_CFI_PERMISSIVE))
 		kvm_err(" (CONFIG_CFI_PERMISSIVE ignored for hyp failures)\n");
 }
@@ -455,7 +481,7 @@  void __noreturn __cold nvhe_hyp_panic_handler(u64 esr, u64 spsr,
 		else
 			print_nvhe_hyp_panic("BUG", panic_addr);
 	} else if (IS_ENABLED(CONFIG_CFI_CLANG) && esr_is_cfi_brk(esr)) {
-		kvm_nvhe_report_cfi_failure(panic_addr);
+		kvm_nvhe_report_cfi_failure(panic_addr, esr, hyp_offset);
 	} else {
 		print_nvhe_hyp_panic("panic", panic_addr);
 	}
diff --git a/arch/arm64/kvm/hyp/entry.S b/arch/arm64/kvm/hyp/entry.S
index 343851c17373..8965dbc75972 100644
--- a/arch/arm64/kvm/hyp/entry.S
+++ b/arch/arm64/kvm/hyp/entry.S
@@ -83,7 +83,7 @@  alternative_else_nop_endif
 	eret
 	sb
 
-SYM_INNER_LABEL(__hyp_restore_elr_and_panic, SYM_L_GLOBAL)
+SYM_INNER_LABEL(__hyp_restore_elr_save_context_and_panic, SYM_L_GLOBAL)
 	// x2-x29,lr: vcpu regs
 	// vcpu x0-x1 on the stack
 
@@ -91,6 +91,28 @@  SYM_INNER_LABEL(__hyp_restore_elr_and_panic, SYM_L_GLOBAL)
 	ldr	x0, [x0, #CPU_ELR_EL2]
 	msr	elr_el2, x0
 
+SYM_INNER_LABEL(__hyp_save_context_and_panic, SYM_L_GLOBAL)
+	// x0-x29,lr: hyp regs
+
+	stp	x0, x1, [sp, #-16]!
+
+	adr_this_cpu x0, kvm_hyp_ctxt, x1
+
+	stp	x2, x3,   [x0, #CPU_XREG_OFFSET(2)]
+
+	ldp	x2, x3, [sp], #16
+
+	stp	x2, x3,   [x0, #CPU_XREG_OFFSET(0)]
+	stp	x4, x5,   [x0, #CPU_XREG_OFFSET(4)]
+	stp	x6, x7,   [x0, #CPU_XREG_OFFSET(6)]
+	stp	x8, x9,   [x0, #CPU_XREG_OFFSET(8)]
+	stp	x10, x11, [x0, #CPU_XREG_OFFSET(10)]
+	stp	x12, x13, [x0, #CPU_XREG_OFFSET(12)]
+	stp	x14, x15, [x0, #CPU_XREG_OFFSET(14)]
+	stp	x16, x17, [x0, #CPU_XREG_OFFSET(16)]
+
+	save_callee_saved_regs x0
+
 SYM_INNER_LABEL(__hyp_panic, SYM_L_GLOBAL)
 	// x2-x29,lr: vcpu regs
 	// vcpu x0-x1 on the stack
diff --git a/arch/arm64/kvm/hyp/hyp-entry.S b/arch/arm64/kvm/hyp/hyp-entry.S
index 7e65ef738ec9..d0d90d598338 100644
--- a/arch/arm64/kvm/hyp/hyp-entry.S
+++ b/arch/arm64/kvm/hyp/hyp-entry.S
@@ -130,7 +130,7 @@  SYM_CODE_END(\label)
 .endm
 
 	/* None of these should ever happen */
-	invalid_vector	el2t_sync_invalid
+	invalid_vector	el2t_sync_invalid, __hyp_save_context_and_panic
 	invalid_vector	el2t_irq_invalid
 	invalid_vector	el2t_fiq_invalid
 	invalid_vector	el2t_error_invalid
diff --git a/arch/arm64/kvm/hyp/include/hyp/switch.h b/arch/arm64/kvm/hyp/include/hyp/switch.h
index d9931abf14c2..77783dbc1833 100644
--- a/arch/arm64/kvm/hyp/include/hyp/switch.h
+++ b/arch/arm64/kvm/hyp/include/hyp/switch.h
@@ -689,7 +689,7 @@  static inline bool fixup_guest_exit(struct kvm_vcpu *vcpu, u64 *exit_code)
 
 static inline void __kvm_unexpected_el2_exception(void)
 {
-	extern char __hyp_restore_elr_and_panic[];
+	extern char __hyp_restore_elr_save_context_and_panic[];
 	unsigned long addr, fixup;
 	struct kvm_exception_table_entry *entry, *end;
 	unsigned long elr_el2 = read_sysreg(elr_el2);
@@ -712,7 +712,7 @@  static inline void __kvm_unexpected_el2_exception(void)
 
 	/* Trigger a panic after restoring the hyp context. */
 	this_cpu_ptr(&kvm_hyp_ctxt)->sys_regs[ELR_EL2] = elr_el2;
-	write_sysreg(__hyp_restore_elr_and_panic, elr_el2);
+	write_sysreg(__hyp_restore_elr_save_context_and_panic, elr_el2);
 }
 
 #endif /* __ARM64_KVM_HYP_SWITCH_H__ */
diff --git a/arch/arm64/kvm/hyp/nvhe/host.S b/arch/arm64/kvm/hyp/nvhe/host.S
index a7db40a51e4a..9343160f5357 100644
--- a/arch/arm64/kvm/hyp/nvhe/host.S
+++ b/arch/arm64/kvm/hyp/nvhe/host.S
@@ -214,7 +214,7 @@  SYM_FUNC_END(__host_hvc)
 .endm
 
 .macro host_el2_sync_vect
-	__host_el2_vect __hyp_panic
+	__host_el2_vect __hyp_save_context_and_panic
 .endm
 
 .macro invalid_host_el1_vect
diff --git a/arch/arm64/kvm/hyp/vhe/switch.c b/arch/arm64/kvm/hyp/vhe/switch.c
index 0550b9f6317f..6c64783c3e00 100644
--- a/arch/arm64/kvm/hyp/vhe/switch.c
+++ b/arch/arm64/kvm/hyp/vhe/switch.c
@@ -17,6 +17,7 @@ 
 
 #include <asm/barrier.h>
 #include <asm/cpufeature.h>
+#include <asm/esr.h>
 #include <asm/kprobes.h>
 #include <asm/kvm_asm.h>
 #include <asm/kvm_emulate.h>
@@ -384,7 +385,24 @@  int __kvm_vcpu_run(struct kvm_vcpu *vcpu)
 	return ret;
 }
 
-static void __noreturn __hyp_call_panic(u64 spsr, u64 elr, u64 par)
+static void __noreturn __hyp_call_panic_for_cfi(u64 elr, u64 esr)
+{
+	struct user_pt_regs *regs = &this_cpu_ptr(&kvm_hyp_ctxt)->regs;
+	u8 type_idx = FIELD_GET(CFI_BRK_IMM_TYPE, esr);
+	u8 target_idx = FIELD_GET(CFI_BRK_IMM_TARGET, esr);
+	u32 expected_type = (u32)regs->regs[type_idx];
+	u64 target = regs->regs[target_idx];
+
+	panic("VHE hyp CFI failure at: [<%016llx>] %pB (target: [<%016llx>] %ps, expected type: 0x%08x)\n"
+#ifdef CONFIG_CFI_PERMISSIVE
+	      " (CONFIG_CFI_PERMISSIVE ignored for hyp failures)\n"
+#endif
+	      ,
+	      elr, (void *)elr, target, (void *)target, expected_type);
+}
+NOKPROBE_SYMBOL(__hyp_call_panic_for_cfi);
+
+static void __noreturn __hyp_call_panic(u64 spsr, u64 elr, u64 par, u64 esr)
 {
 	struct kvm_cpu_context *host_ctxt;
 	struct kvm_vcpu *vcpu;
@@ -395,6 +413,9 @@  static void __noreturn __hyp_call_panic(u64 spsr, u64 elr, u64 par)
 	__deactivate_traps(vcpu);
 	sysreg_restore_host_state_vhe(host_ctxt);
 
+	if (IS_ENABLED(CONFIG_CFI_CLANG) && esr_is_cfi_brk(esr))
+		__hyp_call_panic_for_cfi(elr, esr);
+
 	panic("HYP panic:\nPS:%08llx PC:%016llx ESR:%08llx\nFAR:%016llx HPFAR:%016llx PAR:%016llx\nVCPU:%p\n",
 	      spsr, elr,
 	      read_sysreg_el2(SYS_ESR), read_sysreg_el2(SYS_FAR),
@@ -407,8 +428,9 @@  void __noreturn hyp_panic(void)
 	u64 spsr = read_sysreg_el2(SYS_SPSR);
 	u64 elr = read_sysreg_el2(SYS_ELR);
 	u64 par = read_sysreg_par();
+	u64 esr = read_sysreg_el2(SYS_ESR);
 
-	__hyp_call_panic(spsr, elr, par);
+	__hyp_call_panic(spsr, elr, par, esr);
 }
 
 asmlinkage void kvm_unexpected_el2_exception(void)