[PATCH v4 00/13] KVM: arm64: Add support for hypervisor kCFI

[PATCH v4 00/13] KVM: arm64: Add support for hypervisor kCFI

[Pierre-Clément Tosi]

CONFIG_CFI_CLANG ("kernel Control Flow Integrity") makes the compiler inject
runtime type checks before any indirect function call. On AArch64, it generates
a BRK instruction to be executed on type mismatch and encodes the indices of the
registers holding the branch target and expected type in the immediate of the
instruction. As a result, a synchronous exception gets triggered on kCFI failure
and the fault handler can retrieve the immediate (and indices) from ESR_ELx.

This feature has been supported at EL1 ("host") since it was introduced by
b26e484b8bb3 ("arm64: Add CFI error handling"), where cfi_handler() decodes
ESR_EL1, giving informative panic messages such as

  [   21.885179] CFI failure at lkdtm_indirect_call+0x2c/0x44 [lkdtm]
  (target: lkdtm_increment_int+0x0/0x1c [lkdtm]; expected type: 0x7e0c52a)
  [   21.886593] Internal error: Oops - CFI: 0 [#1] PREEMPT SMP

However, it is not or only partially supported at EL2: in nVHE (or pKVM),
CONFIG_CFI_CLANG gets filtered out at build time, preventing the compiler from
injecting the checks. In VHE, EL2 code gets compiled with the checks but the
handlers in VBAR_EL2 are not aware of kCFI and will produce a generic and
not-so-helpful panic message such as

  [   36.456088][  T200] Kernel panic - not syncing: HYP panic:
  [   36.456088][  T200] PS:204003c9 PC:ffffffc080092310 ESR:f2008228
  [   36.456088][  T200] FAR:0000000081a50000 HPFAR:000000000081a500 PAR:1de7ec7edbadc0de
  [   36.456088][  T200] VCPU:00000000e189c7cf

To address this,

- [01/13] fixes an existing bug where the ELR_EL2 was getting clobbered on
  synchronous exceptions, causing the wrong "PC" to be reported by
  nvhe_hyp_panic_handler() or __hyp_call_panic(). This is particularly limiting
  for kCFI, as it would mask the location of the failed type check.
- [02/13] fixes a minor C/asm ABI mismatch which would trigger a kCFI failure
- [03/13] to [09/13] prepare nVHE for CONFIG_CFI_CLANG and [10/13] enables it
- [11/13] improves kCFI error messages by saving then parsing the CPU context
- [12/13] adds a kCFI test module for VHE and [13/13] extends it to nVHE & pKVM

As a result, an informative kCFI panic message is printed by or on behalf of EL2
giving the expected type and target address (possibly resolved to a symbol) for
VHE, nVHE, and pKVM (iff CONFIG_NVHE_EL2_DEBUG=y).

Note that kCFI errors remain fatal at EL2, even when CONFIG_CFI_PERMISSIVE=y.

Changes in v4:
  - Addressed Will's comments on v3:
  - Removed save/restore of x0-x1 & used __guest_exit_panic ABI for new routines
  - Reworked __pkvm_init_switch_pgd new API with separate args
  - Moved cosmetic changes (renaming to __hyp_panic) into dedicated commit
  - Further clarified the commit message regarding R_AARCH64_ABS32
  - early_brk64() uses esr_is_cfi_brk() (now introduced along esr_brk_comment())
  - Added helper to display nvHE panic banner
  - Moved the test module to the end of the series

Pierre-Clément Tosi (13):
  KVM: arm64: Fix clobbered ELR in sync abort/SError
  KVM: arm64: Fix __pkvm_init_switch_pgd call ABI
  KVM: arm64: nVHE: Simplify __guest_exit_panic path
  KVM: arm64: nVHE: Add EL2h sync exception handler
  KVM: arm64: Rename __guest_exit_panic __hyp_panic
  KVM: arm64: nVHE: gen-hyprel: Skip R_AARCH64_ABS32
  KVM: arm64: VHE: Mark __hyp_call_panic __noreturn
  arm64: Introduce esr_comment() & esr_is_cfi_brk()
  KVM: arm64: Introduce print_nvhe_hyp_panic helper
  KVM: arm64: nVHE: Support CONFIG_CFI_CLANG at EL2
  KVM: arm64: Improve CONFIG_CFI_CLANG error message
  KVM: arm64: VHE: Add test module for hyp kCFI
  KVM: arm64: nVHE: Support test module for hyp kCFI

 arch/arm64/include/asm/esr.h            |  11 ++
 arch/arm64/include/asm/kvm_asm.h        |   3 +
 arch/arm64/include/asm/kvm_cfi.h        |  38 +++++++
 arch/arm64/include/asm/kvm_hyp.h        |   3 +-
 arch/arm64/kernel/asm-offsets.c         |   1 +
 arch/arm64/kernel/debug-monitors.c      |   4 +-
 arch/arm64/kernel/traps.c               |   8 +-
 arch/arm64/kvm/Kconfig                  |  20 ++++
 arch/arm64/kvm/Makefile                 |   3 +
 arch/arm64/kvm/handle_exit.c            |  50 ++++++++-
 arch/arm64/kvm/hyp/cfi.c                |  37 +++++++
 arch/arm64/kvm/hyp/entry.S              |  34 +++++-
 arch/arm64/kvm/hyp/hyp-entry.S          |   4 +-
 arch/arm64/kvm/hyp/include/hyp/cfi.h    |  47 +++++++++
 arch/arm64/kvm/hyp/include/hyp/switch.h |   5 +-
 arch/arm64/kvm/hyp/nvhe/Makefile        |   7 +-
 arch/arm64/kvm/hyp/nvhe/gen-hyprel.c    |   6 ++
 arch/arm64/kvm/hyp/nvhe/host.S          |  21 ++--
 arch/arm64/kvm/hyp/nvhe/hyp-init.S      |  23 ++--
 arch/arm64/kvm/hyp/nvhe/hyp-main.c      |  19 ++++
 arch/arm64/kvm/hyp/nvhe/setup.c         |   4 +-
 arch/arm64/kvm/hyp/nvhe/switch.c        |   7 ++
 arch/arm64/kvm/hyp/vhe/Makefile         |   1 +
 arch/arm64/kvm/hyp/vhe/switch.c         |  34 +++++-
 arch/arm64/kvm/hyp_cfi_test.c           |  75 +++++++++++++
 arch/arm64/kvm/hyp_cfi_test_module.c    | 135 ++++++++++++++++++++++++
 26 files changed, 553 insertions(+), 47 deletions(-)
 create mode 100644 arch/arm64/include/asm/kvm_cfi.h
 create mode 100644 arch/arm64/kvm/hyp/cfi.c
 create mode 100644 arch/arm64/kvm/hyp/include/hyp/cfi.h
 create mode 100644 arch/arm64/kvm/hyp_cfi_test.c
 create mode 100644 arch/arm64/kvm/hyp_cfi_test_module.c

-- 
2.45.1.288.g0e0cd299f1-goog


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

[PATCH v4 01/13] KVM: arm64: Fix clobbered ELR in sync abort/SError

[Pierre-Clément Tosi]

When the hypervisor receives a SError or synchronous exception (EL2h)
while running with the __kvm_hyp_vector and if ELR_EL2 doesn't point to
an extable entry, it panics indirectly by overwriting ELR with the
address of a panic handler in order for the asm routine it returns to to
ERET into the handler.

However, this clobbers ELR_EL2 for the handler itself. As a result,
hyp_panic(), when retrieving what it believes to be the PC where the
exception happened, actually ends up reading the address of the panic
handler that called it! This results in an erroneous and confusing panic
message where the source of any synchronous exception (e.g. BUG() or
kCFI) appears to be __guest_exit_panic, making it hard to locate the
actual BRK instruction.

Therefore, store the original ELR_EL2 in the per-CPU kvm_hyp_ctxt and
point the sysreg to a routine that first restores it to its previous
value before running __guest_exit_panic.

Fixes: 7db21530479f ("KVM: arm64: Restore hyp when panicking in guest context")
Signed-off-by: Pierre-Clément Tosi <ptosi@google.com>
---
 arch/arm64/kernel/asm-offsets.c         | 1 +
 arch/arm64/kvm/hyp/entry.S              | 8 ++++++++
 arch/arm64/kvm/hyp/include/hyp/switch.h | 5 +++--
 3 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/arch/arm64/kernel/asm-offsets.c b/arch/arm64/kernel/asm-offsets.c
index 81496083c041..27de1dddb0ab 100644
--- a/arch/arm64/kernel/asm-offsets.c
+++ b/arch/arm64/kernel/asm-offsets.c
@@ -128,6 +128,7 @@ int main(void)
   DEFINE(VCPU_FAULT_DISR,	offsetof(struct kvm_vcpu, arch.fault.disr_el1));
   DEFINE(VCPU_HCR_EL2,		offsetof(struct kvm_vcpu, arch.hcr_el2));
   DEFINE(CPU_USER_PT_REGS,	offsetof(struct kvm_cpu_context, regs));
+  DEFINE(CPU_ELR_EL2,		offsetof(struct kvm_cpu_context, sys_regs[ELR_EL2]));
   DEFINE(CPU_RGSR_EL1,		offsetof(struct kvm_cpu_context, sys_regs[RGSR_EL1]));
   DEFINE(CPU_GCR_EL1,		offsetof(struct kvm_cpu_context, sys_regs[GCR_EL1]));
   DEFINE(CPU_APIAKEYLO_EL1,	offsetof(struct kvm_cpu_context, sys_regs[APIAKEYLO_EL1]));
diff --git a/arch/arm64/kvm/hyp/entry.S b/arch/arm64/kvm/hyp/entry.S
index f3aa7738b477..4433a234aa9b 100644
--- a/arch/arm64/kvm/hyp/entry.S
+++ b/arch/arm64/kvm/hyp/entry.S
@@ -83,6 +83,14 @@ alternative_else_nop_endif
 	eret
 	sb
 
+SYM_INNER_LABEL(__guest_exit_restore_elr_and_panic, SYM_L_GLOBAL)
+	// x2-x29,lr: vcpu regs
+	// vcpu x0-x1 on the stack
+
+	adr_this_cpu x0, kvm_hyp_ctxt, x1
+	ldr	x0, [x0, #CPU_ELR_EL2]
+	msr	elr_el2, x0
+
 SYM_INNER_LABEL(__guest_exit_panic, SYM_L_GLOBAL)
 	// x2-x29,lr: vcpu regs
 	// vcpu x0-x1 on the stack
diff --git a/arch/arm64/kvm/hyp/include/hyp/switch.h b/arch/arm64/kvm/hyp/include/hyp/switch.h
index a92566f36022..ed9a63f1f7bf 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 __guest_exit_panic[];
+	extern char __guest_exit_restore_elr_and_panic[];
 	unsigned long addr, fixup;
 	struct kvm_exception_table_entry *entry, *end;
 	unsigned long elr_el2 = read_sysreg(elr_el2);
@@ -711,7 +711,8 @@ static inline void __kvm_unexpected_el2_exception(void)
 	}
 
 	/* Trigger a panic after restoring the hyp context. */
-	write_sysreg(__guest_exit_panic, elr_el2);
+	this_cpu_ptr(&kvm_hyp_ctxt)->sys_regs[ELR_EL2] = elr_el2;
+	write_sysreg(__guest_exit_restore_elr_and_panic, elr_el2);
 }
 
 #endif /* __ARM64_KVM_HYP_SWITCH_H__ */
-- 
2.45.1.288.g0e0cd299f1-goog


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

[PATCH v4 02/13] KVM: arm64: Fix __pkvm_init_switch_pgd call ABI

[Pierre-Clément Tosi]

Fix the mismatch between the (incorrect) C signature, C call site, and
asm implementation by aligning all three on an API passing the
parameters (pgd and SP) separately, instead of as a bundled struct.

Remove the now unnecessary memory accesses while the MMU is off from the
asm, which simplifies the C caller (as it does not need to convert a VA
struct pointer to PA) and makes the code slightly more robust by
offsetting the struct fields from C and properly expressing the call to
the C compiler (e.g. type checker and kCFI).

Fixes: f320bc742bc2 ("KVM: arm64: Prepare the creation of s1 mappings at EL2")
Signed-off-by: Pierre-Clément Tosi <ptosi@google.com>
---
 arch/arm64/include/asm/kvm_hyp.h   |  3 +--
 arch/arm64/kvm/hyp/nvhe/hyp-init.S | 17 +++++++++--------
 arch/arm64/kvm/hyp/nvhe/setup.c    |  4 ++--
 3 files changed, 12 insertions(+), 12 deletions(-)

diff --git a/arch/arm64/include/asm/kvm_hyp.h b/arch/arm64/include/asm/kvm_hyp.h
index 3e80464f8953..58b5a2b14d88 100644
--- a/arch/arm64/include/asm/kvm_hyp.h
+++ b/arch/arm64/include/asm/kvm_hyp.h
@@ -123,8 +123,7 @@ void __noreturn __hyp_do_panic(struct kvm_cpu_context *host_ctxt, u64 spsr,
 #endif
 
 #ifdef __KVM_NVHE_HYPERVISOR__
-void __pkvm_init_switch_pgd(phys_addr_t phys, unsigned long size,
-			    phys_addr_t pgd, void *sp, void *cont_fn);
+void __pkvm_init_switch_pgd(phys_addr_t pgd, void *sp, void (*fn)(void));
 int __pkvm_init(phys_addr_t phys, unsigned long size, unsigned long nr_cpus,
 		unsigned long *per_cpu_base, u32 hyp_va_bits);
 void __noreturn __host_enter(struct kvm_cpu_context *host_ctxt);
diff --git a/arch/arm64/kvm/hyp/nvhe/hyp-init.S b/arch/arm64/kvm/hyp/nvhe/hyp-init.S
index 2994878d68ea..d859c4de06b6 100644
--- a/arch/arm64/kvm/hyp/nvhe/hyp-init.S
+++ b/arch/arm64/kvm/hyp/nvhe/hyp-init.S
@@ -265,33 +265,34 @@ alternative_else_nop_endif
 
 SYM_CODE_END(__kvm_handle_stub_hvc)
 
+/*
+ * void __pkvm_init_switch_pgd(phys_addr_t pgd, void *sp, void (*fn)(void));
+ */
 SYM_FUNC_START(__pkvm_init_switch_pgd)
 	/* Turn the MMU off */
 	pre_disable_mmu_workaround
-	mrs	x2, sctlr_el2
-	bic	x3, x2, #SCTLR_ELx_M
+	mrs	x9, sctlr_el2
+	bic	x3, x9, #SCTLR_ELx_M
 	msr	sctlr_el2, x3
 	isb
 
 	tlbi	alle2
 
 	/* Install the new pgtables */
-	ldr	x3, [x0, #NVHE_INIT_PGD_PA]
-	phys_to_ttbr x4, x3
+	phys_to_ttbr x4, x0
 alternative_if ARM64_HAS_CNP
 	orr	x4, x4, #TTBR_CNP_BIT
 alternative_else_nop_endif
 	msr	ttbr0_el2, x4
 
 	/* Set the new stack pointer */
-	ldr	x0, [x0, #NVHE_INIT_STACK_HYP_VA]
-	mov	sp, x0
+	mov	sp, x1
 
 	/* And turn the MMU back on! */
 	dsb	nsh
 	isb
-	set_sctlr_el2	x2
-	ret	x1
+	set_sctlr_el2	x9
+	ret	x2
 SYM_FUNC_END(__pkvm_init_switch_pgd)
 
 	.popsection
diff --git a/arch/arm64/kvm/hyp/nvhe/setup.c b/arch/arm64/kvm/hyp/nvhe/setup.c
index 859f22f754d3..1cbd2c78f7a1 100644
--- a/arch/arm64/kvm/hyp/nvhe/setup.c
+++ b/arch/arm64/kvm/hyp/nvhe/setup.c
@@ -316,7 +316,7 @@ int __pkvm_init(phys_addr_t phys, unsigned long size, unsigned long nr_cpus,
 {
 	struct kvm_nvhe_init_params *params;
 	void *virt = hyp_phys_to_virt(phys);
-	void (*fn)(phys_addr_t params_pa, void *finalize_fn_va);
+	typeof(__pkvm_init_switch_pgd) *fn;
 	int ret;
 
 	BUG_ON(kvm_check_pvm_sysreg_table());
@@ -340,7 +340,7 @@ int __pkvm_init(phys_addr_t phys, unsigned long size, unsigned long nr_cpus,
 	/* Jump in the idmap page to switch to the new page-tables */
 	params = this_cpu_ptr(&kvm_init_params);
 	fn = (typeof(fn))__hyp_pa(__pkvm_init_switch_pgd);
-	fn(__hyp_pa(params), __pkvm_init_finalise);
+	fn(params->pgd_pa, (void *)params->stack_hyp_va, __pkvm_init_finalise);
 
 	unreachable();
 }
-- 
2.45.1.288.g0e0cd299f1-goog


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

[PATCH v4 03/13] KVM: arm64: nVHE: Simplify __guest_exit_panic path

[Pierre-Clément Tosi]

In invalid_host_el2_vect (i.e. EL2{t,h} handlers in nVHE guest context),
remove the duplicate vCPU context check that __guest_exit_panic also
performs, allowing an unconditional branch to it.

Signed-off-by: Pierre-Clément Tosi <ptosi@google.com>
---
 arch/arm64/kvm/hyp/nvhe/host.S | 9 ++-------
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/arch/arm64/kvm/hyp/nvhe/host.S b/arch/arm64/kvm/hyp/nvhe/host.S
index 135cfb294ee5..71fb311b4c0e 100644
--- a/arch/arm64/kvm/hyp/nvhe/host.S
+++ b/arch/arm64/kvm/hyp/nvhe/host.S
@@ -197,18 +197,13 @@ SYM_FUNC_END(__host_hvc)
 	sub	x0, sp, x0			// x0'' = sp' - x0' = (sp + x0) - sp = x0
 	sub	sp, sp, x0			// sp'' = sp' - x0 = (sp + x0) - x0 = sp
 
-	/* If a guest is loaded, panic out of it. */
-	stp	x0, x1, [sp, #-16]!
-	get_loaded_vcpu x0, x1
-	cbnz	x0, __guest_exit_panic
-	add	sp, sp, #16
-
 	/*
 	 * The panic may not be clean if the exception is taken before the host
 	 * context has been saved by __host_exit or after the hyp context has
 	 * been partially clobbered by __host_enter.
 	 */
-	b	hyp_panic
+	stp	x0, x1, [sp, #-16]!
+	b	__guest_exit_panic
 
 .L__hyp_sp_overflow\@:
 	/* Switch to the overflow stack */
-- 
2.45.1.288.g0e0cd299f1-goog


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

[PATCH v4 04/13] KVM: arm64: nVHE: Add EL2h sync exception handler

[Pierre-Clément Tosi]

Introduce a handler for EL2h synchronous exceptions distinct from
handlers for other "invalid" exceptions when running with the nVHE host
vector. This will allow a future patch to handle kCFI (synchronous)
errors without affecting other classes of exceptions.

Signed-off-by: Pierre-Clément Tosi <ptosi@google.com>
---
 arch/arm64/kvm/hyp/nvhe/host.S | 14 +++++++++++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/arch/arm64/kvm/hyp/nvhe/host.S b/arch/arm64/kvm/hyp/nvhe/host.S
index 71fb311b4c0e..bc0a73d9fcd0 100644
--- a/arch/arm64/kvm/hyp/nvhe/host.S
+++ b/arch/arm64/kvm/hyp/nvhe/host.S
@@ -183,7 +183,7 @@ SYM_FUNC_END(__host_hvc)
 .endif
 .endm
 
-.macro invalid_host_el2_vect
+.macro __host_el2_vect handler:req
 	.align 7
 
 	/*
@@ -203,7 +203,7 @@ SYM_FUNC_END(__host_hvc)
 	 * been partially clobbered by __host_enter.
 	 */
 	stp	x0, x1, [sp, #-16]!
-	b	__guest_exit_panic
+	b	\handler
 
 .L__hyp_sp_overflow\@:
 	/* Switch to the overflow stack */
@@ -213,6 +213,10 @@ SYM_FUNC_END(__host_hvc)
 	ASM_BUG()
 .endm
 
+.macro host_el2_sync_vect
+	__host_el2_vect __guest_exit_panic
+.endm
+
 .macro invalid_host_el1_vect
 	.align 7
 	mov	x0, xzr		/* restore_host = false */
@@ -222,6 +226,10 @@ SYM_FUNC_END(__host_hvc)
 	b	__hyp_do_panic
 .endm
 
+.macro invalid_host_el2_vect
+	__host_el2_vect __guest_exit_panic
+.endm
+
 /*
  * The host vector does not use an ESB instruction in order to avoid consuming
  * SErrors that should only be consumed by the host. Guest entry is deferred by
@@ -239,7 +247,7 @@ SYM_CODE_START(__kvm_hyp_host_vector)
 	invalid_host_el2_vect			// FIQ EL2t
 	invalid_host_el2_vect			// Error EL2t
 
-	invalid_host_el2_vect			// Synchronous EL2h
+	host_el2_sync_vect			// Synchronous EL2h
 	invalid_host_el2_vect			// IRQ EL2h
 	invalid_host_el2_vect			// FIQ EL2h
 	invalid_host_el2_vect			// Error EL2h
-- 
2.45.1.288.g0e0cd299f1-goog


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

[PATCH v4 05/13] KVM: arm64: Rename __guest_exit_panic __hyp_panic

[Pierre-Clément Tosi]

Use a name that expresses the fact that the routine might not exit
through the guest but will always (directly or indirectly) end up
executing hyp_panic().

Use CPU_LR_OFFSET to clarify that the routine returns to hyp_panic().

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

diff --git a/arch/arm64/kvm/hyp/entry.S b/arch/arm64/kvm/hyp/entry.S
index 4433a234aa9b..343851c17373 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(__guest_exit_restore_elr_and_panic, SYM_L_GLOBAL)
+SYM_INNER_LABEL(__hyp_restore_elr_and_panic, SYM_L_GLOBAL)
 	// x2-x29,lr: vcpu regs
 	// vcpu x0-x1 on the stack
 
@@ -91,7 +91,7 @@ SYM_INNER_LABEL(__guest_exit_restore_elr_and_panic, SYM_L_GLOBAL)
 	ldr	x0, [x0, #CPU_ELR_EL2]
 	msr	elr_el2, x0
 
-SYM_INNER_LABEL(__guest_exit_panic, SYM_L_GLOBAL)
+SYM_INNER_LABEL(__hyp_panic, SYM_L_GLOBAL)
 	// x2-x29,lr: vcpu regs
 	// vcpu x0-x1 on the stack
 
@@ -109,7 +109,7 @@ SYM_INNER_LABEL(__guest_exit_panic, SYM_L_GLOBAL)
 	// accurate if the guest had been completely restored.
 	adr_this_cpu x0, kvm_hyp_ctxt, x1
 	adr_l	x1, hyp_panic
-	str	x1, [x0, #CPU_XREG_OFFSET(30)]
+	str	x1, [x0, #CPU_LR_OFFSET]
 
 	get_vcpu_ptr	x1, x0
 
diff --git a/arch/arm64/kvm/hyp/hyp-entry.S b/arch/arm64/kvm/hyp/hyp-entry.S
index 03f97d71984c..7e65ef738ec9 100644
--- a/arch/arm64/kvm/hyp/hyp-entry.S
+++ b/arch/arm64/kvm/hyp/hyp-entry.S
@@ -122,7 +122,7 @@ el2_error:
 	eret
 	sb
 
-.macro invalid_vector	label, target = __guest_exit_panic
+.macro invalid_vector	label, target = __hyp_panic
 	.align	2
 SYM_CODE_START_LOCAL(\label)
 	b \target
diff --git a/arch/arm64/kvm/hyp/include/hyp/switch.h b/arch/arm64/kvm/hyp/include/hyp/switch.h
index ed9a63f1f7bf..d9931abf14c2 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 __guest_exit_restore_elr_and_panic[];
+	extern char __hyp_restore_elr_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(__guest_exit_restore_elr_and_panic, elr_el2);
+	write_sysreg(__hyp_restore_elr_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 bc0a73d9fcd0..a7db40a51e4a 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 __guest_exit_panic
+	__host_el2_vect __hyp_panic
 .endm
 
 .macro invalid_host_el1_vect
@@ -227,7 +227,7 @@ SYM_FUNC_END(__host_hvc)
 .endm
 
 .macro invalid_host_el2_vect
-	__host_el2_vect __guest_exit_panic
+	__host_el2_vect __hyp_panic
 .endm
 
 /*
-- 
2.45.1.288.g0e0cd299f1-goog


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

[PATCH v4 06/13] KVM: arm64: nVHE: gen-hyprel: Skip R_AARCH64_ABS32

[Pierre-Clément Tosi]

Ignore R_AARCH64_ABS32 relocations, instead of panicking, when emitting
the relocation table of the hypervisor. The toolchain might produce them
when generating function calls with kCFI to represent the 32-bit type ID
which can then be resolved across compilation units at link time.  These
are NOT actual 32-bit addresses and are therefore not needed in the
final (runtime) relocation table (which is unlikely to use 32-bit
absolute addresses for arm64 anyway).

Signed-off-by: Pierre-Clément Tosi <ptosi@google.com>
---
 arch/arm64/kvm/hyp/nvhe/gen-hyprel.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/arch/arm64/kvm/hyp/nvhe/gen-hyprel.c b/arch/arm64/kvm/hyp/nvhe/gen-hyprel.c
index 6bc88a756cb7..b63f4e1c1033 100644
--- a/arch/arm64/kvm/hyp/nvhe/gen-hyprel.c
+++ b/arch/arm64/kvm/hyp/nvhe/gen-hyprel.c
@@ -50,6 +50,9 @@
 #ifndef R_AARCH64_ABS64
 #define R_AARCH64_ABS64			257
 #endif
+#ifndef R_AARCH64_ABS32
+#define R_AARCH64_ABS32			258
+#endif
 #ifndef R_AARCH64_PREL64
 #define R_AARCH64_PREL64		260
 #endif
@@ -383,6 +386,9 @@ static void emit_rela_section(Elf64_Shdr *sh_rela)
 		case R_AARCH64_ABS64:
 			emit_rela_abs64(rela, sh_orig_name);
 			break;
+		/* Allow 32-bit absolute relocation, for kCFI type hashes. */
+		case R_AARCH64_ABS32:
+			break;
 		/* Allow position-relative data relocations. */
 		case R_AARCH64_PREL64:
 		case R_AARCH64_PREL32:
-- 
2.45.1.288.g0e0cd299f1-goog


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

[PATCH v4 07/13] KVM: arm64: VHE: Mark __hyp_call_panic __noreturn

[Pierre-Clément Tosi]

Given that the sole purpose of __hyp_call_panic() is to call panic(), a
__noreturn function, give it the __noreturn attribute, removing the need
for its caller to use unreachable().

Signed-off-by: Pierre-Clément Tosi <ptosi@google.com>
---
 arch/arm64/kvm/hyp/vhe/switch.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/arch/arm64/kvm/hyp/vhe/switch.c b/arch/arm64/kvm/hyp/vhe/switch.c
index d7af5f46f22a..0550b9f6317f 100644
--- a/arch/arm64/kvm/hyp/vhe/switch.c
+++ b/arch/arm64/kvm/hyp/vhe/switch.c
@@ -384,7 +384,7 @@ int __kvm_vcpu_run(struct kvm_vcpu *vcpu)
 	return ret;
 }
 
-static void __hyp_call_panic(u64 spsr, u64 elr, u64 par)
+static void __noreturn __hyp_call_panic(u64 spsr, u64 elr, u64 par)
 {
 	struct kvm_cpu_context *host_ctxt;
 	struct kvm_vcpu *vcpu;
@@ -409,7 +409,6 @@ void __noreturn hyp_panic(void)
 	u64 par = read_sysreg_par();
 
 	__hyp_call_panic(spsr, elr, par);
-	unreachable();
 }
 
 asmlinkage void kvm_unexpected_el2_exception(void)
-- 
2.45.1.288.g0e0cd299f1-goog


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

[PATCH v4 08/13] arm64: Introduce esr_comment() & esr_is_cfi_brk()

[Pierre-Clément Tosi]

As it is already used in two places, move esr_comment() to a header for
re-use, with a clearer name.

Introduce esr_is_cfi_brk() to detect kCFI BRK syndromes, currently used
by early_brk64() but soon to be also used by hypervisor code.

Signed-off-by: Pierre-Clément Tosi <ptosi@google.com>
---
 arch/arm64/include/asm/esr.h       | 11 +++++++++++
 arch/arm64/kernel/debug-monitors.c |  4 +---
 arch/arm64/kernel/traps.c          |  8 +++-----
 arch/arm64/kvm/handle_exit.c       |  2 +-
 4 files changed, 16 insertions(+), 9 deletions(-)

diff --git a/arch/arm64/include/asm/esr.h b/arch/arm64/include/asm/esr.h
index 7abf09df7033..77569d207ecf 100644
--- a/arch/arm64/include/asm/esr.h
+++ b/arch/arm64/include/asm/esr.h
@@ -379,6 +379,11 @@
 #ifndef __ASSEMBLY__
 #include <asm/types.h>
 
+static inline unsigned long esr_brk_comment(unsigned long esr)
+{
+	return esr & ESR_ELx_BRK64_ISS_COMMENT_MASK;
+}
+
 static inline bool esr_is_data_abort(unsigned long esr)
 {
 	const unsigned long ec = ESR_ELx_EC(esr);
@@ -386,6 +391,12 @@ static inline bool esr_is_data_abort(unsigned long esr)
 	return ec == ESR_ELx_EC_DABT_LOW || ec == ESR_ELx_EC_DABT_CUR;
 }
 
+static inline bool esr_is_cfi_brk(unsigned long esr)
+{
+	return ESR_ELx_EC(esr) == ESR_ELx_EC_BRK64 &&
+	       (esr_brk_comment(esr) & ~CFI_BRK_IMM_MASK) == CFI_BRK_IMM_BASE;
+}
+
 static inline bool esr_fsc_is_translation_fault(unsigned long esr)
 {
 	/* Translation fault, level -1 */
diff --git a/arch/arm64/kernel/debug-monitors.c b/arch/arm64/kernel/debug-monitors.c
index 64f2ecbdfe5c..024a7b245056 100644
--- a/arch/arm64/kernel/debug-monitors.c
+++ b/arch/arm64/kernel/debug-monitors.c
@@ -312,9 +312,7 @@ static int call_break_hook(struct pt_regs *regs, unsigned long esr)
 	 * entirely not preemptible, and we can use rcu list safely here.
 	 */
 	list_for_each_entry_rcu(hook, list, node) {
-		unsigned long comment = esr & ESR_ELx_BRK64_ISS_COMMENT_MASK;
-
-		if ((comment & ~hook->mask) == hook->imm)
+		if ((esr_brk_comment(esr) & ~hook->mask) == hook->imm)
 			fn = hook->fn;
 	}
 
diff --git a/arch/arm64/kernel/traps.c b/arch/arm64/kernel/traps.c
index 215e6d7f2df8..9e22683aa921 100644
--- a/arch/arm64/kernel/traps.c
+++ b/arch/arm64/kernel/traps.c
@@ -1105,8 +1105,6 @@ static struct break_hook ubsan_break_hook = {
 };
 #endif
 
-#define esr_comment(esr) ((esr) & ESR_ELx_BRK64_ISS_COMMENT_MASK)
-
 /*
  * Initial handler for AArch64 BRK exceptions
  * This handler only used until debug_traps_init().
@@ -1115,15 +1113,15 @@ int __init early_brk64(unsigned long addr, unsigned long esr,
 		struct pt_regs *regs)
 {
 #ifdef CONFIG_CFI_CLANG
-	if ((esr_comment(esr) & ~CFI_BRK_IMM_MASK) == CFI_BRK_IMM_BASE)
+	if (esr_is_cfi_brk(esr))
 		return cfi_handler(regs, esr) != DBG_HOOK_HANDLED;
 #endif
 #ifdef CONFIG_KASAN_SW_TAGS
-	if ((esr_comment(esr) & ~KASAN_BRK_MASK) == KASAN_BRK_IMM)
+	if ((esr_brk_comment(esr) & ~KASAN_BRK_MASK) == KASAN_BRK_IMM)
 		return kasan_handler(regs, esr) != DBG_HOOK_HANDLED;
 #endif
 #ifdef CONFIG_UBSAN_TRAP
-	if ((esr_comment(esr) & ~UBSAN_BRK_MASK) == UBSAN_BRK_IMM)
+	if ((esr_brk_comment(esr) & ~UBSAN_BRK_MASK) == UBSAN_BRK_IMM)
 		return ubsan_handler(regs, esr) != DBG_HOOK_HANDLED;
 #endif
 	return bug_handler(regs, esr) != DBG_HOOK_HANDLED;
diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
index b037f0a0e27e..d41447193e13 100644
--- a/arch/arm64/kvm/handle_exit.c
+++ b/arch/arm64/kvm/handle_exit.c
@@ -423,7 +423,7 @@ void __noreturn __cold nvhe_hyp_panic_handler(u64 esr, u64 spsr,
 	if (mode != PSR_MODE_EL2t && mode != PSR_MODE_EL2h) {
 		kvm_err("Invalid host exception to nVHE hyp!\n");
 	} else if (ESR_ELx_EC(esr) == ESR_ELx_EC_BRK64 &&
-		   (esr & ESR_ELx_BRK64_ISS_COMMENT_MASK) == BUG_BRK_IMM) {
+		   esr_brk_comment(esr) == BUG_BRK_IMM) {
 		const char *file = NULL;
 		unsigned int line = 0;
 
-- 
2.45.1.288.g0e0cd299f1-goog


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

[PATCH v4 09/13] KVM: arm64: Introduce print_nvhe_hyp_panic helper

[Pierre-Clément Tosi]

Add a helper to display a panic banner soon to also be used for kCFI
failures, to ensure that we remain consistent.

Signed-off-by: Pierre-Clément Tosi <ptosi@google.com>
---
 arch/arm64/kvm/handle_exit.c | 12 ++++++++----
 1 file changed, 8 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
index d41447193e13..b3d6657a259d 100644
--- a/arch/arm64/kvm/handle_exit.c
+++ b/arch/arm64/kvm/handle_exit.c
@@ -411,6 +411,12 @@ void handle_exit_early(struct kvm_vcpu *vcpu, int exception_index)
 		kvm_handle_guest_serror(vcpu, kvm_vcpu_get_esr(vcpu));
 }
 
+static void print_nvhe_hyp_panic(const char *name, u64 panic_addr)
+{
+	kvm_err("nVHE hyp %s at: [<%016llx>] %pB!\n", name, panic_addr,
+		(void *)(panic_addr + kaslr_offset()));
+}
+
 void __noreturn __cold nvhe_hyp_panic_handler(u64 esr, u64 spsr,
 					      u64 elr_virt, u64 elr_phys,
 					      u64 par, uintptr_t vcpu,
@@ -439,11 +445,9 @@ void __noreturn __cold nvhe_hyp_panic_handler(u64 esr, u64 spsr,
 		if (file)
 			kvm_err("nVHE hyp BUG at: %s:%u!\n", file, line);
 		else
-			kvm_err("nVHE hyp BUG at: [<%016llx>] %pB!\n", panic_addr,
-					(void *)(panic_addr + kaslr_offset()));
+			print_nvhe_hyp_panic("BUG", panic_addr);
 	} else {
-		kvm_err("nVHE hyp panic at: [<%016llx>] %pB!\n", panic_addr,
-				(void *)(panic_addr + kaslr_offset()));
+		print_nvhe_hyp_panic("panic", panic_addr);
 	}
 
 	/* Dump the nVHE hypervisor backtrace */
-- 
2.45.1.288.g0e0cd299f1-goog


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

[PATCH v4 10/13] KVM: arm64: nVHE: Support CONFIG_CFI_CLANG at EL2

[Pierre-Clément Tosi]

The compiler implements kCFI by adding type information (u32) above
every function that might be indirectly called and, whenever a function
pointer is called, injects a read-and-compare of that u32 against the
value corresponding to the expected type. In case of a mismatch, a BRK
instruction gets executed. When the hypervisor triggers such an
exception in nVHE, it panics and triggers and exception return to EL1.

Therefore, teach nvhe_hyp_panic_handler() to detect kCFI errors from the
ESR and report them. If necessary, remind the user that EL2 kCFI is not
affected by CONFIG_CFI_PERMISSIVE.

Pass $(CC_FLAGS_CFI) to the compiler when building the nVHE hyp code.

Use SYM_TYPED_FUNC_START() for __pkvm_init_switch_pgd, as nVHE can't
call it directly and must use a PA function pointer from C (because it
is part of the idmap page), which would trigger a kCFI failure if the
type ID wasn't present.

Signed-off-by: Pierre-Clément Tosi <ptosi@google.com>
---
 arch/arm64/kvm/handle_exit.c       | 10 ++++++++++
 arch/arm64/kvm/hyp/nvhe/Makefile   |  6 +++---
 arch/arm64/kvm/hyp/nvhe/hyp-init.S |  6 +++++-
 3 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
index b3d6657a259d..69b08ac7322d 100644
--- a/arch/arm64/kvm/handle_exit.c
+++ b/arch/arm64/kvm/handle_exit.c
@@ -417,6 +417,14 @@ 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)
+{
+	print_nvhe_hyp_panic("CFI failure", panic_addr);
+
+	if (IS_ENABLED(CONFIG_CFI_PERMISSIVE))
+		kvm_err(" (CONFIG_CFI_PERMISSIVE ignored for hyp failures)\n");
+}
+
 void __noreturn __cold nvhe_hyp_panic_handler(u64 esr, u64 spsr,
 					      u64 elr_virt, u64 elr_phys,
 					      u64 par, uintptr_t vcpu,
@@ -446,6 +454,8 @@ void __noreturn __cold nvhe_hyp_panic_handler(u64 esr, u64 spsr,
 			kvm_err("nVHE hyp BUG at: %s:%u!\n", file, line);
 		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);
 	} else {
 		print_nvhe_hyp_panic("panic", panic_addr);
 	}
diff --git a/arch/arm64/kvm/hyp/nvhe/Makefile b/arch/arm64/kvm/hyp/nvhe/Makefile
index 50fa0ffb6b7e..782b34b004be 100644
--- a/arch/arm64/kvm/hyp/nvhe/Makefile
+++ b/arch/arm64/kvm/hyp/nvhe/Makefile
@@ -89,9 +89,9 @@ quiet_cmd_hyprel = HYPREL  $@
 quiet_cmd_hypcopy = HYPCOPY $@
       cmd_hypcopy = $(OBJCOPY) --prefix-symbols=__kvm_nvhe_ $< $@
 
-# Remove ftrace, Shadow Call Stack, and CFI CFLAGS.
-# This is equivalent to the 'notrace', '__noscs', and '__nocfi' annotations.
-KBUILD_CFLAGS := $(filter-out $(CC_FLAGS_FTRACE) $(CC_FLAGS_SCS) $(CC_FLAGS_CFI), $(KBUILD_CFLAGS))
+# Remove ftrace and Shadow Call Stack CFLAGS.
+# This is equivalent to the 'notrace' and '__noscs' annotations.
+KBUILD_CFLAGS := $(filter-out $(CC_FLAGS_FTRACE) $(CC_FLAGS_SCS), $(KBUILD_CFLAGS))
 # Starting from 13.0.0 llvm emits SHT_REL section '.llvm.call-graph-profile'
 # when profile optimization is applied. gen-hyprel does not support SHT_REL and
 # causes a build failure. Remove profile optimization flags.
diff --git a/arch/arm64/kvm/hyp/nvhe/hyp-init.S b/arch/arm64/kvm/hyp/nvhe/hyp-init.S
index d859c4de06b6..b1c8977e2812 100644
--- a/arch/arm64/kvm/hyp/nvhe/hyp-init.S
+++ b/arch/arm64/kvm/hyp/nvhe/hyp-init.S
@@ -5,6 +5,7 @@
  */
 
 #include <linux/arm-smccc.h>
+#include <linux/cfi_types.h>
 #include <linux/linkage.h>
 
 #include <asm/alternative.h>
@@ -267,8 +268,11 @@ SYM_CODE_END(__kvm_handle_stub_hvc)
 
 /*
  * void __pkvm_init_switch_pgd(phys_addr_t pgd, void *sp, void (*fn)(void));
+ *
+ * SYM_TYPED_FUNC_START() allows C to call this ID-mapped function indirectly
+ * using a physical pointer without triggering a kCFI failure.
  */
-SYM_FUNC_START(__pkvm_init_switch_pgd)
+SYM_TYPED_FUNC_START(__pkvm_init_switch_pgd)
 	/* Turn the MMU off */
 	pre_disable_mmu_workaround
 	mrs	x9, sctlr_el2
-- 
2.45.1.288.g0e0cd299f1-goog


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

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

[Pierre-Clément Tosi]

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

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)
-- 
2.45.1.288.g0e0cd299f1-goog


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

[PATCH v4 12/13] KVM: arm64: VHE: Add test module for hyp kCFI

[Pierre-Clément Tosi]

In order to easily periodically (and potentially automatically) validate
that the hypervisor kCFI feature doesn't bitrot, introduce a way to
trigger hypervisor kCFI faults from userspace on test builds of KVM.

Add hooks in the hypervisor code to call registered callbacks (intended
to trigger kCFI faults either for the callback call itself of from
within the callback function) when running with guest or host VBAR_EL2.
As the calls are issued from the KVM_RUN ioctl handling path, userspace
gains control over when the actual triggering of the fault happens
without needing to modify the KVM uAPI.

Export kernel functions to register these callbacks from modules and
introduce a kernel module intended to contain any testing logic. By
limiting the changes to the core kernel to a strict minimum, this
architectural split allows tests to be updated (within the module)
without the need to redeploy (or recompile) the kernel (hyp) under test.

Use the module parameters as the uAPI for configuring the fault
condition being tested (i.e. either at insertion or post-insertion
using /sys/module/.../parameters), which naturally makes it impossible
for userspace to test kCFI without the module (and, inversely, makes
the module only - not KVM - responsible for exposing said uAPI).

As kCFI is implemented with a caller-side check of a callee-side value,
make the module support 4 tests based on the location of the caller and
callee (built-in or in-module), for each of the 2 hypervisor contexts
(host & guest), selected by userspace using the 'guest' or 'host' module
parameter. For this purpose, export symbols which the module can use to
configure the callbacks for in-kernel and module-to-built-in kCFI
faulting calls.

Define the module-to-kernel API to allow the module to detect that it
was loaded on a kernel built with support for it but which is running
without a hypervisor (-ENXIO) or with one that doesn't use the VHE CPU
feature (-EOPNOTSUPP), which is currently the only mode for which KVM
supports hypervisor kCFI.

Allow kernel build configs to set CONFIG_HYP_CFI_TEST to only support
the in-kernel hooks (=y) or also build the test module (=m). Use
intermediate internal Kconfig flags (CONFIG_HYP_SUPPORTS_CFI_TEST and
CONFIG_HYP_CFI_TEST_MODULE) to simplify the Makefiles and #ifdefs. As
the symbols for callback registration are only exported to modules when
CONFIG_HYP_CFI_TEST != n, it is impossible for the test module to be
non-forcefully inserted on a kernel that doesn't support it.

Note that this feature must NOT result in any noticeable change
(behavioral or binary size) when HYP_CFI_TEST_MODULE = n.

CONFIG_HYP_CFI_TEST is intentionally independent of CONFIG_CFI_CLANG, to
avoid arbitrarily limiting the number of flag combinations that can be
tested with the module.

Also note that, as VHE aliases VBAR_EL1 to VBAR_EL2 for the host,
testing hypervisor kCFI in VHE and in host context is equivalent to
testing kCFI support of the kernel itself i.e. EL1 in non-VHE and/or in
non-virtualized environments. For this reason, CONFIG_CFI_PERMISSIVE
**will** prevent the test module from triggering a hyp panic (although a
warning still gets printed) in that context.

Signed-off-by: Pierre-Clément Tosi <ptosi@google.com>
---
 arch/arm64/include/asm/kvm_cfi.h     |  36 ++++++++
 arch/arm64/kvm/Kconfig               |  22 +++++
 arch/arm64/kvm/Makefile              |   3 +
 arch/arm64/kvm/hyp/include/hyp/cfi.h |  47 ++++++++++
 arch/arm64/kvm/hyp/vhe/Makefile      |   1 +
 arch/arm64/kvm/hyp/vhe/cfi.c         |  37 ++++++++
 arch/arm64/kvm/hyp/vhe/switch.c      |   7 ++
 arch/arm64/kvm/hyp_cfi_test.c        |  43 +++++++++
 arch/arm64/kvm/hyp_cfi_test_module.c | 133 +++++++++++++++++++++++++++
 9 files changed, 329 insertions(+)
 create mode 100644 arch/arm64/include/asm/kvm_cfi.h
 create mode 100644 arch/arm64/kvm/hyp/include/hyp/cfi.h
 create mode 100644 arch/arm64/kvm/hyp/vhe/cfi.c
 create mode 100644 arch/arm64/kvm/hyp_cfi_test.c
 create mode 100644 arch/arm64/kvm/hyp_cfi_test_module.c

diff --git a/arch/arm64/include/asm/kvm_cfi.h b/arch/arm64/include/asm/kvm_cfi.h
new file mode 100644
index 000000000000..13cc7b19d838
--- /dev/null
+++ b/arch/arm64/include/asm/kvm_cfi.h
@@ -0,0 +1,36 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (C) 2024 - Google Inc
+ * Author: Pierre-Clément Tosi <ptosi@google.com>
+ */
+
+#ifndef __ARM64_KVM_CFI_H__
+#define __ARM64_KVM_CFI_H__
+
+#include <asm/kvm_asm.h>
+#include <linux/errno.h>
+
+#ifdef CONFIG_HYP_SUPPORTS_CFI_TEST
+
+int kvm_cfi_test_register_host_ctxt_cb(void (*cb)(void));
+int kvm_cfi_test_register_guest_ctxt_cb(void (*cb)(void));
+
+#else
+
+static inline int kvm_cfi_test_register_host_ctxt_cb(void (*cb)(void))
+{
+	return -EOPNOTSUPP;
+}
+
+static inline int kvm_cfi_test_register_guest_ctxt_cb(void (*cb)(void))
+{
+	return -EOPNOTSUPP;
+}
+
+#endif /* CONFIG_HYP_SUPPORTS_CFI_TEST */
+
+/* Symbols which the host can register as hyp callbacks; see <hyp/cfi.h>. */
+void hyp_trigger_builtin_cfi_fault(void);
+void hyp_builtin_cfi_fault_target(int unused);
+
+#endif /* __ARM64_KVM_CFI_H__ */
diff --git a/arch/arm64/kvm/Kconfig b/arch/arm64/kvm/Kconfig
index 58f09370d17e..5daa8079a120 100644
--- a/arch/arm64/kvm/Kconfig
+++ b/arch/arm64/kvm/Kconfig
@@ -65,4 +65,26 @@ config PROTECTED_NVHE_STACKTRACE
 
 	  If unsure, or not using protected nVHE (pKVM), say N.
 
+config HYP_CFI_TEST
+	tristate "KVM hypervisor kCFI test support"
+	depends on KVM
+	help
+	  Say Y or M here to build KVM with test hooks to support intentionally
+	  triggering hypervisor kCFI faults in guest or host context.
+
+	  Say M here to also build a module which registers callbacks triggering
+	  faults and selected by userspace through its parameters.
+
+	  Note that this feature is currently only supported in VHE mode.
+
+	  If unsure, say N.
+
+config HYP_SUPPORTS_CFI_TEST
+	def_bool y
+	depends on HYP_CFI_TEST
+
+config HYP_CFI_TEST_MODULE
+	def_tristate m if HYP_CFI_TEST = m
+	depends on HYP_CFI_TEST
+
 endif # VIRTUALIZATION
diff --git a/arch/arm64/kvm/Makefile b/arch/arm64/kvm/Makefile
index a6497228c5a8..303be42ad90a 100644
--- a/arch/arm64/kvm/Makefile
+++ b/arch/arm64/kvm/Makefile
@@ -22,6 +22,7 @@ kvm-y += arm.o mmu.o mmio.o psci.o hypercalls.o pvtime.o \
 	 vgic/vgic-mmio-v3.o vgic/vgic-kvm-device.o \
 	 vgic/vgic-its.o vgic/vgic-debug.o
 
+kvm-$(CONFIG_HYP_SUPPORTS_CFI_TEST) += hyp_cfi_test.o
 kvm-$(CONFIG_HW_PERF_EVENTS)  += pmu-emul.o pmu.o
 kvm-$(CONFIG_ARM64_PTR_AUTH)  += pauth.o
 
@@ -40,3 +41,5 @@ $(obj)/hyp_constants.h: $(obj)/hyp-constants.s FORCE
 
 obj-kvm := $(addprefix $(obj)/, $(kvm-y))
 $(obj-kvm): $(obj)/hyp_constants.h
+
+obj-$(CONFIG_HYP_CFI_TEST_MODULE) += hyp_cfi_test_module.o
diff --git a/arch/arm64/kvm/hyp/include/hyp/cfi.h b/arch/arm64/kvm/hyp/include/hyp/cfi.h
new file mode 100644
index 000000000000..c6536040bc06
--- /dev/null
+++ b/arch/arm64/kvm/hyp/include/hyp/cfi.h
@@ -0,0 +1,47 @@
+/* SPDX-License-Identifier: GPL-2.0-only */
+/*
+ * Copyright (C) 2024 - Google Inc
+ * Author: Pierre-Clément Tosi <ptosi@google.com>
+ */
+
+#ifndef __ARM64_KVM_HYP_CFI_H__
+#define __ARM64_KVM_HYP_CFI_H__
+
+#include <asm/bug.h>
+#include <asm/errno.h>
+
+#include <linux/compiler.h>
+
+#ifdef CONFIG_HYP_SUPPORTS_CFI_TEST
+
+int __kvm_register_cfi_test_cb(void (*cb)(void), bool in_host_ctxt);
+
+extern void (*hyp_test_host_ctxt_cfi)(void);
+extern void (*hyp_test_guest_ctxt_cfi)(void);
+
+/* Hypervisor callbacks for the host to register. */
+void hyp_trigger_builtin_cfi_fault(void);
+void hyp_builtin_cfi_fault_target(int unused);
+
+#else
+
+static inline
+int __kvm_register_cfi_test_cb(void (*cb)(void), bool in_host_ctxt)
+{
+	return -EOPNOTSUPP;
+}
+
+#define hyp_test_host_ctxt_cfi ((void(*)(void))(NULL))
+#define hyp_test_guest_ctxt_cfi ((void(*)(void))(NULL))
+
+static inline void hyp_trigger_builtin_cfi_fault(void)
+{
+}
+
+static inline void hyp_builtin_cfi_fault_target(int __always_unused unused)
+{
+}
+
+#endif /* CONFIG_HYP_SUPPORTS_CFI_TEST */
+
+#endif /* __ARM64_KVM_HYP_CFI_H__ */
diff --git a/arch/arm64/kvm/hyp/vhe/Makefile b/arch/arm64/kvm/hyp/vhe/Makefile
index 3b9e5464b5b3..19ca584cc21e 100644
--- a/arch/arm64/kvm/hyp/vhe/Makefile
+++ b/arch/arm64/kvm/hyp/vhe/Makefile
@@ -9,3 +9,4 @@ ccflags-y := -D__KVM_VHE_HYPERVISOR__
 obj-y := timer-sr.o sysreg-sr.o debug-sr.o switch.o tlb.o
 obj-y += ../vgic-v3-sr.o ../aarch32.o ../vgic-v2-cpuif-proxy.o ../entry.o \
 	 ../fpsimd.o ../hyp-entry.o ../exception.o
+obj-$(CONFIG_HYP_SUPPORTS_CFI_TEST) += cfi.o
diff --git a/arch/arm64/kvm/hyp/vhe/cfi.c b/arch/arm64/kvm/hyp/vhe/cfi.c
new file mode 100644
index 000000000000..5849f239e27f
--- /dev/null
+++ b/arch/arm64/kvm/hyp/vhe/cfi.c
@@ -0,0 +1,37 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2024 - Google Inc
+ * Author: Pierre-Clément Tosi <ptosi@google.com>
+ */
+#include <asm/rwonce.h>
+
+#include <hyp/cfi.h>
+
+void (*hyp_test_host_ctxt_cfi)(void);
+void (*hyp_test_guest_ctxt_cfi)(void);
+
+int __kvm_register_cfi_test_cb(void (*cb)(void), bool in_host_ctxt)
+{
+	if (in_host_ctxt)
+		hyp_test_host_ctxt_cfi = cb;
+	else
+		hyp_test_guest_ctxt_cfi = cb;
+
+	return 0;
+}
+
+void hyp_builtin_cfi_fault_target(int __always_unused unused)
+{
+}
+
+void hyp_trigger_builtin_cfi_fault(void)
+{
+	/* Intentional UB cast & dereference, to trigger a kCFI fault. */
+	void (*target)(void) = (void *)&hyp_builtin_cfi_fault_target;
+
+	/*
+	 * READ_ONCE() prevents this indirect call from being optimized out,
+	 * forcing the compiler to generate the kCFI check before the branch.
+	 */
+	READ_ONCE(target)();
+}
diff --git a/arch/arm64/kvm/hyp/vhe/switch.c b/arch/arm64/kvm/hyp/vhe/switch.c
index 6c64783c3e00..fe70220876b4 100644
--- a/arch/arm64/kvm/hyp/vhe/switch.c
+++ b/arch/arm64/kvm/hyp/vhe/switch.c
@@ -4,6 +4,7 @@
  * Author: Marc Zyngier <marc.zyngier@arm.com>
  */
 
+#include <hyp/cfi.h>
 #include <hyp/switch.h>
 
 #include <linux/arm-smccc.h>
@@ -311,6 +312,9 @@ static int __kvm_vcpu_run_vhe(struct kvm_vcpu *vcpu)
 	struct kvm_cpu_context *guest_ctxt;
 	u64 exit_code;
 
+	if (IS_ENABLED(CONFIG_HYP_SUPPORTS_CFI_TEST) && unlikely(hyp_test_host_ctxt_cfi))
+		hyp_test_host_ctxt_cfi();
+
 	host_ctxt = host_data_ptr(host_ctxt);
 	guest_ctxt = &vcpu->arch.ctxt;
 
@@ -329,6 +333,9 @@ static int __kvm_vcpu_run_vhe(struct kvm_vcpu *vcpu)
 	sysreg_restore_guest_state_vhe(guest_ctxt);
 	__debug_switch_to_guest(vcpu);
 
+	if (IS_ENABLED(CONFIG_HYP_SUPPORTS_CFI_TEST) && unlikely(hyp_test_guest_ctxt_cfi))
+		hyp_test_guest_ctxt_cfi();
+
 	do {
 		/* Jump in the fire! */
 		exit_code = __guest_enter(vcpu);
diff --git a/arch/arm64/kvm/hyp_cfi_test.c b/arch/arm64/kvm/hyp_cfi_test.c
new file mode 100644
index 000000000000..da7b25ca1b1f
--- /dev/null
+++ b/arch/arm64/kvm/hyp_cfi_test.c
@@ -0,0 +1,43 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2024 - Google Inc
+ * Author: Pierre-Clément Tosi <ptosi@google.com>
+ */
+#include <asm/kvm_asm.h>
+#include <asm/kvm_cfi.h>
+#include <asm/kvm_host.h>
+#include <asm/virt.h>
+
+#include <linux/export.h>
+#include <linux/stddef.h>
+#include <linux/types.h>
+
+/* For calling directly into the VHE hypervisor; see <hyp/cfi.h>. */
+int __kvm_register_cfi_test_cb(void (*)(void), bool);
+
+static int kvm_register_cfi_test_cb(void (*vhe_cb)(void), bool in_host_ctxt)
+{
+	if (!is_hyp_mode_available())
+		return -ENXIO;
+
+	if (is_hyp_nvhe())
+		return -EOPNOTSUPP;
+
+	return __kvm_register_cfi_test_cb(vhe_cb, in_host_ctxt);
+}
+
+int kvm_cfi_test_register_host_ctxt_cb(void (*cb)(void))
+{
+	return kvm_register_cfi_test_cb(cb, true);
+}
+EXPORT_SYMBOL(kvm_cfi_test_register_host_ctxt_cb);
+
+int kvm_cfi_test_register_guest_ctxt_cb(void (*cb)(void))
+{
+	return kvm_register_cfi_test_cb(cb, false);
+}
+EXPORT_SYMBOL(kvm_cfi_test_register_guest_ctxt_cb);
+
+/* Hypervisor callbacks for the test module to register. */
+EXPORT_SYMBOL(hyp_trigger_builtin_cfi_fault);
+EXPORT_SYMBOL(hyp_builtin_cfi_fault_target);
diff --git a/arch/arm64/kvm/hyp_cfi_test_module.c b/arch/arm64/kvm/hyp_cfi_test_module.c
new file mode 100644
index 000000000000..eeda4be4d3ef
--- /dev/null
+++ b/arch/arm64/kvm/hyp_cfi_test_module.c
@@ -0,0 +1,133 @@
+// SPDX-License-Identifier: GPL-2.0-only
+/*
+ * Copyright (C) 2024 - Google Inc
+ * Author: Pierre-Clément Tosi <ptosi@google.com>
+ */
+#define pr_fmt(fmt) KBUILD_MODNAME ": " fmt
+
+#include <asm/kvm_asm.h>
+#include <asm/kvm_cfi.h>
+#include <asm/rwonce.h>
+
+#include <linux/init.h>
+#include <linux/kstrtox.h>
+#include <linux/module.h>
+#include <linux/printk.h>
+
+static int set_host_mode(const char *val, const struct kernel_param *kp);
+static int set_guest_mode(const char *val, const struct kernel_param *kp);
+
+#define M_DESC \
+	"\n\t0: none" \
+	"\n\t1: built-in caller & built-in callee" \
+	"\n\t2: built-in caller & module callee" \
+	"\n\t3: module caller & built-in callee" \
+	"\n\t4: module caller & module callee"
+
+static unsigned int host_mode;
+module_param_call(host, set_host_mode, param_get_uint, &host_mode, 0644);
+MODULE_PARM_DESC(host,
+		 "Hypervisor kCFI fault test case in host context:" M_DESC);
+
+static unsigned int guest_mode;
+module_param_call(guest, set_guest_mode, param_get_uint, &guest_mode, 0644);
+MODULE_PARM_DESC(guest,
+		 "Hypervisor kCFI fault test case in guest context:" M_DESC);
+
+static void trigger_module2module_cfi_fault(void);
+static void trigger_module2builtin_cfi_fault(void);
+static void hyp_cfi_module2module_test_target(int);
+static void hyp_cfi_builtin2module_test_target(int);
+
+static int set_param_mode(const char *val, const struct kernel_param *kp,
+			 int (*register_cb)(void (*)(void)))
+{
+	unsigned int *mode = kp->arg;
+	int err;
+
+	err = param_set_uint(val, kp);
+	if (err)
+		return err;
+
+	switch (*mode) {
+	case 0:
+		return register_cb(NULL);
+	case 1:
+		return register_cb(hyp_trigger_builtin_cfi_fault);
+	case 2:
+		return register_cb((void *)hyp_cfi_builtin2module_test_target);
+	case 3:
+		return register_cb(trigger_module2builtin_cfi_fault);
+	case 4:
+		return register_cb(trigger_module2module_cfi_fault);
+	default:
+		return -EINVAL;
+	}
+}
+
+static int set_host_mode(const char *val, const struct kernel_param *kp)
+{
+	return set_param_mode(val, kp, kvm_cfi_test_register_host_ctxt_cb);
+}
+
+static int set_guest_mode(const char *val, const struct kernel_param *kp)
+{
+	return set_param_mode(val, kp, kvm_cfi_test_register_guest_ctxt_cb);
+}
+
+static void __exit exit_hyp_cfi_test(void)
+{
+	int err;
+
+	err = kvm_cfi_test_register_host_ctxt_cb(NULL);
+	if (err)
+		pr_err("Failed to unregister host context trigger: %d\n", err);
+
+	err = kvm_cfi_test_register_guest_ctxt_cb(NULL);
+	if (err)
+		pr_err("Failed to unregister guest context trigger: %d\n", err);
+}
+module_exit(exit_hyp_cfi_test);
+
+static void trigger_module2builtin_cfi_fault(void)
+{
+	/* Intentional UB cast & dereference, to trigger a kCFI fault. */
+	void (*target)(void) = (void *)&hyp_builtin_cfi_fault_target;
+
+	/*
+	 * READ_ONCE() prevents this indirect call from being optimized out,
+	 * forcing the compiler to generate the kCFI check before the branch.
+	 */
+	READ_ONCE(target)();
+
+	pr_err_ratelimited("%s: Survived a kCFI violation\n", __func__);
+}
+
+static void trigger_module2module_cfi_fault(void)
+{
+	/* Intentional UB cast & dereference, to trigger a kCFI fault. */
+	void (*target)(void) = (void *)&hyp_cfi_module2module_test_target;
+
+	/*
+	 * READ_ONCE() prevents this indirect call from being optimized out,
+	 * forcing the compiler to generate the kCFI check before the branch.
+	 */
+	READ_ONCE(target)();
+
+	pr_err_ratelimited("%s: Survived a kCFI violation\n", __func__);
+}
+
+/* Use different functions, for clearer symbols in kCFI panic reports. */
+static noinline
+void hyp_cfi_module2module_test_target(int __always_unused unused)
+{
+}
+
+static noinline
+void hyp_cfi_builtin2module_test_target(int __always_unused unused)
+{
+}
+
+MODULE_LICENSE("GPL");
+MODULE_AUTHOR("Pierre-Clément Tosi <ptosi@google.com>");
+MODULE_DESCRIPTION("KVM hypervisor kCFI test module");
-- 
2.45.1.288.g0e0cd299f1-goog


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

[PATCH v4 13/13] KVM: arm64: nVHE: Support test module for hyp kCFI

[Pierre-Clément Tosi]

Extend support for the kCFI test module to nVHE by replicating the hooks
on the KVM_RUN handler path currently existing in VHE in the nVHE code,
exporting the equivalent callback targets for triggering built-in hyp
kCFI faults, and exposing a new CONFIG_HYP_CFI_TEST-only host HVC to
implement callback registration.

Update the test module to register the nVHE equivalent callback for test
case '1' (i.e. both EL2 hyp caller and callee are built-in) and document
that other cases are not supported outside of VHE, as they require EL2
symbols in the module, which is not currently supported for nVHE.

Note that a kernel in protected mode that doesn't support HYP_CFI_TEST
will prevent the module from registering nVHE callbacks both by not
exporting the necessary symbols (similar to VHE) but also by rejecting
the corresponding HVC, if the module tries to issue it directly.

Also note that the test module will run in pKVM (with HYP_CFI_TEST)
independently of other debug Kconfig flags but that not stacktrace will
be printed without PROTECTED_NVHE_STACKTRACE. This allows testing kCFI
under conditions closer to release builds, if desired.

Signed-off-by: Pierre-Clément Tosi <ptosi@google.com>
---
 arch/arm64/include/asm/kvm_asm.h     |  3 ++
 arch/arm64/include/asm/kvm_cfi.h     |  6 ++--
 arch/arm64/kvm/Kconfig               |  2 --
 arch/arm64/kvm/hyp/{vhe => }/cfi.c   |  0
 arch/arm64/kvm/hyp/nvhe/Makefile     |  1 +
 arch/arm64/kvm/hyp/nvhe/hyp-main.c   | 19 ++++++++++++
 arch/arm64/kvm/hyp/nvhe/switch.c     |  7 +++++
 arch/arm64/kvm/hyp/vhe/Makefile      |  2 +-
 arch/arm64/kvm/hyp_cfi_test.c        | 44 ++++++++++++++++++++++++----
 arch/arm64/kvm/hyp_cfi_test_module.c | 24 ++++++++-------
 10 files changed, 86 insertions(+), 22 deletions(-)
 rename arch/arm64/kvm/hyp/{vhe => }/cfi.c (100%)

diff --git a/arch/arm64/include/asm/kvm_asm.h b/arch/arm64/include/asm/kvm_asm.h
index a6330460d9e5..791054492920 100644
--- a/arch/arm64/include/asm/kvm_asm.h
+++ b/arch/arm64/include/asm/kvm_asm.h
@@ -79,6 +79,9 @@ enum __kvm_host_smccc_func {
 	__KVM_HOST_SMCCC_FUNC___pkvm_init_vm,
 	__KVM_HOST_SMCCC_FUNC___pkvm_init_vcpu,
 	__KVM_HOST_SMCCC_FUNC___pkvm_teardown_vm,
+#ifdef CONFIG_HYP_SUPPORTS_CFI_TEST
+	__KVM_HOST_SMCCC_FUNC___kvm_register_cfi_test_cb,
+#endif
 };
 
 #define DECLARE_KVM_VHE_SYM(sym)	extern char sym[]
diff --git a/arch/arm64/include/asm/kvm_cfi.h b/arch/arm64/include/asm/kvm_cfi.h
index 13cc7b19d838..ed6422eebce5 100644
--- a/arch/arm64/include/asm/kvm_cfi.h
+++ b/arch/arm64/include/asm/kvm_cfi.h
@@ -12,8 +12,8 @@
 
 #ifdef CONFIG_HYP_SUPPORTS_CFI_TEST
 
-int kvm_cfi_test_register_host_ctxt_cb(void (*cb)(void));
-int kvm_cfi_test_register_guest_ctxt_cb(void (*cb)(void));
+int kvm_cfi_test_register_host_ctxt_cb(void (*vhe_cb)(void), void *nvhe_cb);
+int kvm_cfi_test_register_guest_ctxt_cb(void (*vhe_cb)(void), void *nvhe_cb);
 
 #else
 
@@ -31,6 +31,8 @@ static inline int kvm_cfi_test_register_guest_ctxt_cb(void (*cb)(void))
 
 /* Symbols which the host can register as hyp callbacks; see <hyp/cfi.h>. */
 void hyp_trigger_builtin_cfi_fault(void);
+DECLARE_KVM_NVHE_SYM(hyp_trigger_builtin_cfi_fault);
 void hyp_builtin_cfi_fault_target(int unused);
+DECLARE_KVM_NVHE_SYM(hyp_builtin_cfi_fault_target);
 
 #endif /* __ARM64_KVM_CFI_H__ */
diff --git a/arch/arm64/kvm/Kconfig b/arch/arm64/kvm/Kconfig
index 5daa8079a120..715c85088c06 100644
--- a/arch/arm64/kvm/Kconfig
+++ b/arch/arm64/kvm/Kconfig
@@ -75,8 +75,6 @@ config HYP_CFI_TEST
 	  Say M here to also build a module which registers callbacks triggering
 	  faults and selected by userspace through its parameters.
 
-	  Note that this feature is currently only supported in VHE mode.
-
 	  If unsure, say N.
 
 config HYP_SUPPORTS_CFI_TEST
diff --git a/arch/arm64/kvm/hyp/vhe/cfi.c b/arch/arm64/kvm/hyp/cfi.c
similarity index 100%
rename from arch/arm64/kvm/hyp/vhe/cfi.c
rename to arch/arm64/kvm/hyp/cfi.c
diff --git a/arch/arm64/kvm/hyp/nvhe/Makefile b/arch/arm64/kvm/hyp/nvhe/Makefile
index 782b34b004be..115aa8880260 100644
--- a/arch/arm64/kvm/hyp/nvhe/Makefile
+++ b/arch/arm64/kvm/hyp/nvhe/Makefile
@@ -25,6 +25,7 @@ hyp-obj-y := timer-sr.o sysreg-sr.o debug-sr.o switch.o tlb.o hyp-init.o host.o
 	 cache.o setup.o mm.o mem_protect.o sys_regs.o pkvm.o stacktrace.o ffa.o
 hyp-obj-y += ../vgic-v3-sr.o ../aarch32.o ../vgic-v2-cpuif-proxy.o ../entry.o \
 	 ../fpsimd.o ../hyp-entry.o ../exception.o ../pgtable.o
+hyp-obj-$(CONFIG_HYP_SUPPORTS_CFI_TEST) += ../cfi.o
 hyp-obj-$(CONFIG_LIST_HARDENED) += list_debug.o
 hyp-obj-y += $(lib-objs)
 
diff --git a/arch/arm64/kvm/hyp/nvhe/hyp-main.c b/arch/arm64/kvm/hyp/nvhe/hyp-main.c
index d5c48dc98f67..39ed06fbb190 100644
--- a/arch/arm64/kvm/hyp/nvhe/hyp-main.c
+++ b/arch/arm64/kvm/hyp/nvhe/hyp-main.c
@@ -5,6 +5,7 @@
  */
 
 #include <hyp/adjust_pc.h>
+#include <hyp/cfi.h>
 
 #include <asm/pgtable-types.h>
 #include <asm/kvm_asm.h>
@@ -13,6 +14,8 @@
 #include <asm/kvm_hyp.h>
 #include <asm/kvm_mmu.h>
 
+#include <linux/compiler.h>
+
 #include <nvhe/ffa.h>
 #include <nvhe/mem_protect.h>
 #include <nvhe/mm.h>
@@ -301,6 +304,19 @@ static void handle___pkvm_teardown_vm(struct kvm_cpu_context *host_ctxt)
 	cpu_reg(host_ctxt, 1) = __pkvm_teardown_vm(handle);
 }
 
+#ifndef CONFIG_HYP_SUPPORTS_CFI_TEST
+__always_unused
+#endif
+static void handle___kvm_register_cfi_test_cb(struct kvm_cpu_context *host_ctxt)
+{
+	DECLARE_REG(phys_addr_t, cb_phys, host_ctxt, 1);
+	DECLARE_REG(bool, in_host_ctxt, host_ctxt, 2);
+
+	void (*cb)(void) = cb_phys ? __hyp_va(cb_phys) : NULL;
+
+	cpu_reg(host_ctxt, 1) = __kvm_register_cfi_test_cb(cb, in_host_ctxt);
+}
+
 typedef void (*hcall_t)(struct kvm_cpu_context *);
 
 #define HANDLE_FUNC(x)	[__KVM_HOST_SMCCC_FUNC_##x] = (hcall_t)handle_##x
@@ -333,6 +349,9 @@ static const hcall_t host_hcall[] = {
 	HANDLE_FUNC(__pkvm_init_vm),
 	HANDLE_FUNC(__pkvm_init_vcpu),
 	HANDLE_FUNC(__pkvm_teardown_vm),
+#ifdef CONFIG_HYP_SUPPORTS_CFI_TEST
+	HANDLE_FUNC(__kvm_register_cfi_test_cb),
+#endif
 };
 
 static void handle_host_hcall(struct kvm_cpu_context *host_ctxt)
diff --git a/arch/arm64/kvm/hyp/nvhe/switch.c b/arch/arm64/kvm/hyp/nvhe/switch.c
index 6758cd905570..52d2fada9e19 100644
--- a/arch/arm64/kvm/hyp/nvhe/switch.c
+++ b/arch/arm64/kvm/hyp/nvhe/switch.c
@@ -4,6 +4,7 @@
  * Author: Marc Zyngier <marc.zyngier@arm.com>
  */
 
+#include <hyp/cfi.h>
 #include <hyp/switch.h>
 #include <hyp/sysreg-sr.h>
 
@@ -249,6 +250,9 @@ int __kvm_vcpu_run(struct kvm_vcpu *vcpu)
 	bool pmu_switch_needed;
 	u64 exit_code;
 
+	if (IS_ENABLED(CONFIG_HYP_SUPPORTS_CFI_TEST) && unlikely(hyp_test_host_ctxt_cfi))
+		hyp_test_host_ctxt_cfi();
+
 	/*
 	 * Having IRQs masked via PMR when entering the guest means the GIC
 	 * will not signal the CPU of interrupts of lower priority, and the
@@ -309,6 +313,9 @@ int __kvm_vcpu_run(struct kvm_vcpu *vcpu)
 
 	__debug_switch_to_guest(vcpu);
 
+	if (IS_ENABLED(CONFIG_HYP_SUPPORTS_CFI_TEST) && unlikely(hyp_test_guest_ctxt_cfi))
+		hyp_test_guest_ctxt_cfi();
+
 	do {
 		/* Jump in the fire! */
 		exit_code = __guest_enter(vcpu);
diff --git a/arch/arm64/kvm/hyp/vhe/Makefile b/arch/arm64/kvm/hyp/vhe/Makefile
index 19ca584cc21e..951c8c00a685 100644
--- a/arch/arm64/kvm/hyp/vhe/Makefile
+++ b/arch/arm64/kvm/hyp/vhe/Makefile
@@ -9,4 +9,4 @@ ccflags-y := -D__KVM_VHE_HYPERVISOR__
 obj-y := timer-sr.o sysreg-sr.o debug-sr.o switch.o tlb.o
 obj-y += ../vgic-v3-sr.o ../aarch32.o ../vgic-v2-cpuif-proxy.o ../entry.o \
 	 ../fpsimd.o ../hyp-entry.o ../exception.o
-obj-$(CONFIG_HYP_SUPPORTS_CFI_TEST) += cfi.o
+obj-$(CONFIG_HYP_SUPPORTS_CFI_TEST) += ../cfi.o
diff --git a/arch/arm64/kvm/hyp_cfi_test.c b/arch/arm64/kvm/hyp_cfi_test.c
index da7b25ca1b1f..6a02b43c45f6 100644
--- a/arch/arm64/kvm/hyp_cfi_test.c
+++ b/arch/arm64/kvm/hyp_cfi_test.c
@@ -6,6 +6,7 @@
 #include <asm/kvm_asm.h>
 #include <asm/kvm_cfi.h>
 #include <asm/kvm_host.h>
+#include <asm/kvm_mmu.h>
 #include <asm/virt.h>
 
 #include <linux/export.h>
@@ -15,29 +16,60 @@
 /* For calling directly into the VHE hypervisor; see <hyp/cfi.h>. */
 int __kvm_register_cfi_test_cb(void (*)(void), bool);
 
-static int kvm_register_cfi_test_cb(void (*vhe_cb)(void), bool in_host_ctxt)
+static int kvm_register_nvhe_cfi_test_cb(void *cb, bool in_host_ctxt)
+{
+	extern void *kvm_nvhe_sym(hyp_test_host_ctxt_cfi);
+	extern void *kvm_nvhe_sym(hyp_test_guest_ctxt_cfi);
+
+	if (is_protected_kvm_enabled()) {
+		phys_addr_t cb_phys = cb ? virt_to_phys(cb) : 0;
+
+		/* Use HVC as only the hyp can modify its callback pointers. */
+		return kvm_call_hyp_nvhe(__kvm_register_cfi_test_cb, cb_phys,
+					 in_host_ctxt);
+	}
+
+	/*
+	 * In non-protected nVHE, the pKVM HVC is not available but the
+	 * hyp callback pointers can be accessed and modified directly.
+	 */
+	if (cb)
+		cb = kern_hyp_va(kvm_ksym_ref(cb));
+
+	if (in_host_ctxt)
+		kvm_nvhe_sym(hyp_test_host_ctxt_cfi) = cb;
+	else
+		kvm_nvhe_sym(hyp_test_guest_ctxt_cfi) = cb;
+
+	return 0;
+}
+
+static int kvm_register_cfi_test_cb(void (*vhe_cb)(void), void *nvhe_cb,
+				    bool in_host_ctxt)
 {
 	if (!is_hyp_mode_available())
 		return -ENXIO;
 
 	if (is_hyp_nvhe())
-		return -EOPNOTSUPP;
+		return kvm_register_nvhe_cfi_test_cb(nvhe_cb, in_host_ctxt);
 
 	return __kvm_register_cfi_test_cb(vhe_cb, in_host_ctxt);
 }
 
-int kvm_cfi_test_register_host_ctxt_cb(void (*cb)(void))
+int kvm_cfi_test_register_host_ctxt_cb(void (*vhe_cb)(void), void *nvhe_cb)
 {
-	return kvm_register_cfi_test_cb(cb, true);
+	return kvm_register_cfi_test_cb(vhe_cb, nvhe_cb, true);
 }
 EXPORT_SYMBOL(kvm_cfi_test_register_host_ctxt_cb);
 
-int kvm_cfi_test_register_guest_ctxt_cb(void (*cb)(void))
+int kvm_cfi_test_register_guest_ctxt_cb(void (*vhe_cb)(void), void *nvhe_cb)
 {
-	return kvm_register_cfi_test_cb(cb, false);
+	return kvm_register_cfi_test_cb(vhe_cb, nvhe_cb, false);
 }
 EXPORT_SYMBOL(kvm_cfi_test_register_guest_ctxt_cb);
 
 /* Hypervisor callbacks for the test module to register. */
 EXPORT_SYMBOL(hyp_trigger_builtin_cfi_fault);
+EXPORT_SYMBOL(kvm_nvhe_sym(hyp_trigger_builtin_cfi_fault));
 EXPORT_SYMBOL(hyp_builtin_cfi_fault_target);
+EXPORT_SYMBOL(kvm_nvhe_sym(hyp_builtin_cfi_fault_target));
diff --git a/arch/arm64/kvm/hyp_cfi_test_module.c b/arch/arm64/kvm/hyp_cfi_test_module.c
index eeda4be4d3ef..63a5e99cb164 100644
--- a/arch/arm64/kvm/hyp_cfi_test_module.c
+++ b/arch/arm64/kvm/hyp_cfi_test_module.c
@@ -20,9 +20,9 @@ static int set_guest_mode(const char *val, const struct kernel_param *kp);
 #define M_DESC \
 	"\n\t0: none" \
 	"\n\t1: built-in caller & built-in callee" \
-	"\n\t2: built-in caller & module callee" \
-	"\n\t3: module caller & built-in callee" \
-	"\n\t4: module caller & module callee"
+	"\n\t2: built-in caller & module callee (VHE only)" \
+	"\n\t3: module caller & built-in callee (VHE only)" \
+	"\n\t4: module caller & module callee (VHE only)"
 
 static unsigned int host_mode;
 module_param_call(host, set_host_mode, param_get_uint, &host_mode, 0644);
@@ -40,7 +40,7 @@ static void hyp_cfi_module2module_test_target(int);
 static void hyp_cfi_builtin2module_test_target(int);
 
 static int set_param_mode(const char *val, const struct kernel_param *kp,
-			 int (*register_cb)(void (*)(void)))
+			 int (*register_cb)(void (*)(void), void *))
 {
 	unsigned int *mode = kp->arg;
 	int err;
@@ -51,15 +51,17 @@ static int set_param_mode(const char *val, const struct kernel_param *kp,
 
 	switch (*mode) {
 	case 0:
-		return register_cb(NULL);
+		return register_cb(NULL, NULL);
 	case 1:
-		return register_cb(hyp_trigger_builtin_cfi_fault);
+		return register_cb(hyp_trigger_builtin_cfi_fault,
+				   kvm_nvhe_sym(hyp_trigger_builtin_cfi_fault));
 	case 2:
-		return register_cb((void *)hyp_cfi_builtin2module_test_target);
+		return register_cb((void *)hyp_cfi_builtin2module_test_target,
+				   NULL);
 	case 3:
-		return register_cb(trigger_module2builtin_cfi_fault);
+		return register_cb(trigger_module2builtin_cfi_fault, NULL);
 	case 4:
-		return register_cb(trigger_module2module_cfi_fault);
+		return register_cb(trigger_module2module_cfi_fault, NULL);
 	default:
 		return -EINVAL;
 	}
@@ -79,11 +81,11 @@ static void __exit exit_hyp_cfi_test(void)
 {
 	int err;
 
-	err = kvm_cfi_test_register_host_ctxt_cb(NULL);
+	err = kvm_cfi_test_register_host_ctxt_cb(NULL, NULL);
 	if (err)
 		pr_err("Failed to unregister host context trigger: %d\n", err);
 
-	err = kvm_cfi_test_register_guest_ctxt_cb(NULL);
+	err = kvm_cfi_test_register_guest_ctxt_cb(NULL, NULL);
 	if (err)
 		pr_err("Failed to unregister guest context trigger: %d\n", err);
 }
-- 
2.45.1.288.g0e0cd299f1-goog


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

Re: [PATCH v4 00/13] KVM: arm64: Add support for hypervisor kCFI

[Will Deacon]

On Wed, May 29, 2024 at 01:12:06PM +0100, Pierre-Clément Tosi wrote:
> CONFIG_CFI_CLANG ("kernel Control Flow Integrity") makes the compiler inject
> runtime type checks before any indirect function call. On AArch64, it generates
> a BRK instruction to be executed on type mismatch and encodes the indices of the
> registers holding the branch target and expected type in the immediate of the
> instruction. As a result, a synchronous exception gets triggered on kCFI failure
> and the fault handler can retrieve the immediate (and indices) from ESR_ELx.
> 
> This feature has been supported at EL1 ("host") since it was introduced by
> b26e484b8bb3 ("arm64: Add CFI error handling"), where cfi_handler() decodes
> ESR_EL1, giving informative panic messages such as
> 
>   [   21.885179] CFI failure at lkdtm_indirect_call+0x2c/0x44 [lkdtm]
>   (target: lkdtm_increment_int+0x0/0x1c [lkdtm]; expected type: 0x7e0c52a)
>   [   21.886593] Internal error: Oops - CFI: 0 [#1] PREEMPT SMP
> 
> However, it is not or only partially supported at EL2: in nVHE (or pKVM),
> CONFIG_CFI_CLANG gets filtered out at build time, preventing the compiler from
> injecting the checks. In VHE, EL2 code gets compiled with the checks but the
> handlers in VBAR_EL2 are not aware of kCFI and will produce a generic and
> not-so-helpful panic message such as
> 
>   [   36.456088][  T200] Kernel panic - not syncing: HYP panic:
>   [   36.456088][  T200] PS:204003c9 PC:ffffffc080092310 ESR:f2008228
>   [   36.456088][  T200] FAR:0000000081a50000 HPFAR:000000000081a500 PAR:1de7ec7edbadc0de
>   [   36.456088][  T200] VCPU:00000000e189c7cf
> 
> To address this,
> 
> - [01/13] fixes an existing bug where the ELR_EL2 was getting clobbered on
>   synchronous exceptions, causing the wrong "PC" to be reported by
>   nvhe_hyp_panic_handler() or __hyp_call_panic(). This is particularly limiting
>   for kCFI, as it would mask the location of the failed type check.
> - [02/13] fixes a minor C/asm ABI mismatch which would trigger a kCFI failure
> - [03/13] to [09/13] prepare nVHE for CONFIG_CFI_CLANG and [10/13] enables it
> - [11/13] improves kCFI error messages by saving then parsing the CPU context
> - [12/13] adds a kCFI test module for VHE and [13/13] extends it to nVHE & pKVM
> 
> As a result, an informative kCFI panic message is printed by or on behalf of EL2
> giving the expected type and target address (possibly resolved to a symbol) for
> VHE, nVHE, and pKVM (iff CONFIG_NVHE_EL2_DEBUG=y).
> 
> Note that kCFI errors remain fatal at EL2, even when CONFIG_CFI_PERMISSIVE=y.
> 
> Changes in v4:
>   - Addressed Will's comments on v3:

nit: but please keep reviewers on CC when you post a new version. I
missed this initially.

Will

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

Re: [PATCH v4 01/13] KVM: arm64: Fix clobbered ELR in sync abort/SError

[Will Deacon]

On Wed, May 29, 2024 at 01:12:07PM +0100, Pierre-Clément Tosi wrote:
> When the hypervisor receives a SError or synchronous exception (EL2h)
> while running with the __kvm_hyp_vector and if ELR_EL2 doesn't point to
> an extable entry, it panics indirectly by overwriting ELR with the
> address of a panic handler in order for the asm routine it returns to to
> ERET into the handler.
> 
> However, this clobbers ELR_EL2 for the handler itself. As a result,
> hyp_panic(), when retrieving what it believes to be the PC where the
> exception happened, actually ends up reading the address of the panic
> handler that called it! This results in an erroneous and confusing panic
> message where the source of any synchronous exception (e.g. BUG() or
> kCFI) appears to be __guest_exit_panic, making it hard to locate the
> actual BRK instruction.
> 
> Therefore, store the original ELR_EL2 in the per-CPU kvm_hyp_ctxt and
> point the sysreg to a routine that first restores it to its previous
> value before running __guest_exit_panic.
> 
> Fixes: 7db21530479f ("KVM: arm64: Restore hyp when panicking in guest context")
> Signed-off-by: Pierre-Clément Tosi <ptosi@google.com>
> ---
>  arch/arm64/kernel/asm-offsets.c         | 1 +
>  arch/arm64/kvm/hyp/entry.S              | 8 ++++++++
>  arch/arm64/kvm/hyp/include/hyp/switch.h | 5 +++--
>  3 files changed, 12 insertions(+), 2 deletions(-)

Acked-by: Will Deacon <will@kernel.org>

Will

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

Re: [PATCH v4 02/13] KVM: arm64: Fix __pkvm_init_switch_pgd call ABI

[Will Deacon]

On Wed, May 29, 2024 at 01:12:08PM +0100, Pierre-Clément Tosi wrote:
> Fix the mismatch between the (incorrect) C signature, C call site, and
> asm implementation by aligning all three on an API passing the
> parameters (pgd and SP) separately, instead of as a bundled struct.
> 
> Remove the now unnecessary memory accesses while the MMU is off from the
> asm, which simplifies the C caller (as it does not need to convert a VA
> struct pointer to PA) and makes the code slightly more robust by
> offsetting the struct fields from C and properly expressing the call to
> the C compiler (e.g. type checker and kCFI).
> 
> Fixes: f320bc742bc2 ("KVM: arm64: Prepare the creation of s1 mappings at EL2")
> Signed-off-by: Pierre-Clément Tosi <ptosi@google.com>
> ---
>  arch/arm64/include/asm/kvm_hyp.h   |  3 +--
>  arch/arm64/kvm/hyp/nvhe/hyp-init.S | 17 +++++++++--------
>  arch/arm64/kvm/hyp/nvhe/setup.c    |  4 ++--
>  3 files changed, 12 insertions(+), 12 deletions(-)
> 
> diff --git a/arch/arm64/include/asm/kvm_hyp.h b/arch/arm64/include/asm/kvm_hyp.h
> index 3e80464f8953..58b5a2b14d88 100644
> --- a/arch/arm64/include/asm/kvm_hyp.h
> +++ b/arch/arm64/include/asm/kvm_hyp.h
> @@ -123,8 +123,7 @@ void __noreturn __hyp_do_panic(struct kvm_cpu_context *host_ctxt, u64 spsr,
>  #endif
>  
>  #ifdef __KVM_NVHE_HYPERVISOR__
> -void __pkvm_init_switch_pgd(phys_addr_t phys, unsigned long size,
> -			    phys_addr_t pgd, void *sp, void *cont_fn);
> +void __pkvm_init_switch_pgd(phys_addr_t pgd, void *sp, void (*fn)(void));
>  int __pkvm_init(phys_addr_t phys, unsigned long size, unsigned long nr_cpus,
>  		unsigned long *per_cpu_base, u32 hyp_va_bits);
>  void __noreturn __host_enter(struct kvm_cpu_context *host_ctxt);
> diff --git a/arch/arm64/kvm/hyp/nvhe/hyp-init.S b/arch/arm64/kvm/hyp/nvhe/hyp-init.S
> index 2994878d68ea..d859c4de06b6 100644
> --- a/arch/arm64/kvm/hyp/nvhe/hyp-init.S
> +++ b/arch/arm64/kvm/hyp/nvhe/hyp-init.S
> @@ -265,33 +265,34 @@ alternative_else_nop_endif
>  
>  SYM_CODE_END(__kvm_handle_stub_hvc)
>  
> +/*
> + * void __pkvm_init_switch_pgd(phys_addr_t pgd, void *sp, void (*fn)(void));
> + */
>  SYM_FUNC_START(__pkvm_init_switch_pgd)
>  	/* Turn the MMU off */
>  	pre_disable_mmu_workaround
> -	mrs	x2, sctlr_el2
> -	bic	x3, x2, #SCTLR_ELx_M
> +	mrs	x9, sctlr_el2
> +	bic	x3, x9, #SCTLR_ELx_M

This is fine, but there's no need to jump all the way to x9 for the
register allocation. I think it would be neatest to re-jig the function
so it uses x4 here for the sctlr and then uses x5 later for the ttbr.

> diff --git a/arch/arm64/kvm/hyp/nvhe/setup.c b/arch/arm64/kvm/hyp/nvhe/setup.c
> index 859f22f754d3..1cbd2c78f7a1 100644
> --- a/arch/arm64/kvm/hyp/nvhe/setup.c
> +++ b/arch/arm64/kvm/hyp/nvhe/setup.c
> @@ -316,7 +316,7 @@ int __pkvm_init(phys_addr_t phys, unsigned long size, unsigned long nr_cpus,
>  {
>  	struct kvm_nvhe_init_params *params;
>  	void *virt = hyp_phys_to_virt(phys);
> -	void (*fn)(phys_addr_t params_pa, void *finalize_fn_va);
> +	typeof(__pkvm_init_switch_pgd) *fn;
>  	int ret;
>  
>  	BUG_ON(kvm_check_pvm_sysreg_table());
> @@ -340,7 +340,7 @@ int __pkvm_init(phys_addr_t phys, unsigned long size, unsigned long nr_cpus,
>  	/* Jump in the idmap page to switch to the new page-tables */
>  	params = this_cpu_ptr(&kvm_init_params);
>  	fn = (typeof(fn))__hyp_pa(__pkvm_init_switch_pgd);
> -	fn(__hyp_pa(params), __pkvm_init_finalise);
> +	fn(params->pgd_pa, (void *)params->stack_hyp_va, __pkvm_init_finalise);

Why not have the prototype of __pkvm_init_switch_pgd() take the SP as
an 'unsigned long' so that you can avoid this cast altogether?

Will

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

Re: [PATCH v4 03/13] KVM: arm64: nVHE: Simplify __guest_exit_panic path

[Will Deacon]

On Wed, May 29, 2024 at 01:12:09PM +0100, Pierre-Clément Tosi wrote:
> In invalid_host_el2_vect (i.e. EL2{t,h} handlers in nVHE guest context),

*guest* context? Are you sure?

> remove the duplicate vCPU context check that __guest_exit_panic also
> performs, allowing an unconditional branch to it.
> 
> Signed-off-by: Pierre-Clément Tosi <ptosi@google.com>
> ---
>  arch/arm64/kvm/hyp/nvhe/host.S | 9 ++-------
>  1 file changed, 2 insertions(+), 7 deletions(-)
> 
> diff --git a/arch/arm64/kvm/hyp/nvhe/host.S b/arch/arm64/kvm/hyp/nvhe/host.S
> index 135cfb294ee5..71fb311b4c0e 100644
> --- a/arch/arm64/kvm/hyp/nvhe/host.S
> +++ b/arch/arm64/kvm/hyp/nvhe/host.S
> @@ -197,18 +197,13 @@ SYM_FUNC_END(__host_hvc)
>  	sub	x0, sp, x0			// x0'' = sp' - x0' = (sp + x0) - sp = x0
>  	sub	sp, sp, x0			// sp'' = sp' - x0 = (sp + x0) - x0 = sp
>  
> -	/* If a guest is loaded, panic out of it. */
> -	stp	x0, x1, [sp, #-16]!
> -	get_loaded_vcpu x0, x1
> -	cbnz	x0, __guest_exit_panic
> -	add	sp, sp, #16

I think this is actually dead code and we should just remove it. AFAICT,
invalid_host_el2_vect is only used for the host vectors and the loaded
vCPU will always be NULL, so this is pointless. set_loaded_vcpu() is
only called by the low-level guest entry/exit code and with the guest
EL2 vectors installed.

> -
>  	/*
>  	 * The panic may not be clean if the exception is taken before the host
>  	 * context has been saved by __host_exit or after the hyp context has
>  	 * been partially clobbered by __host_enter.
>  	 */
> -	b	hyp_panic
> +	stp	x0, x1, [sp, #-16]!
> +	b	__guest_exit_panic

In which case, this should just be:

	add	sp, sp, #16
	b	hyp_panic

Did I miss something?

Will

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

Re: [PATCH v4 04/13] KVM: arm64: nVHE: Add EL2h sync exception handler

[Will Deacon]

On Wed, May 29, 2024 at 01:12:10PM +0100, Pierre-Clément Tosi wrote:
> Introduce a handler for EL2h synchronous exceptions distinct from
> handlers for other "invalid" exceptions when running with the nVHE host
> vector. This will allow a future patch to handle kCFI (synchronous)
> errors without affecting other classes of exceptions.
> 
> Signed-off-by: Pierre-Clément Tosi <ptosi@google.com>
> ---
>  arch/arm64/kvm/hyp/nvhe/host.S | 14 +++++++++++---
>  1 file changed, 11 insertions(+), 3 deletions(-)

Acked-by: Will Deacon <will@kernel.org>

Will

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

Re: [PATCH v4 05/13] KVM: arm64: Rename __guest_exit_panic __hyp_panic

[Will Deacon]

On Wed, May 29, 2024 at 01:12:11PM +0100, Pierre-Clément Tosi wrote:
> Use a name that expresses the fact that the routine might not exit
> through the guest but will always (directly or indirectly) end up
> executing hyp_panic().
> 
> Use CPU_LR_OFFSET to clarify that the routine returns to hyp_panic().
> 
> Signed-off-by: Pierre-Clément Tosi <ptosi@google.com>
> ---
>  arch/arm64/kvm/hyp/entry.S              | 6 +++---
>  arch/arm64/kvm/hyp/hyp-entry.S          | 2 +-
>  arch/arm64/kvm/hyp/include/hyp/switch.h | 4 ++--
>  arch/arm64/kvm/hyp/nvhe/host.S          | 4 ++--
>  4 files changed, 8 insertions(+), 8 deletions(-)

Hmm, I'm not sure about this. When is __guest_exit_panic() called outside
of guest context?

Will

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

Re: [PATCH v4 06/13] KVM: arm64: nVHE: gen-hyprel: Skip R_AARCH64_ABS32

[Will Deacon]

On Wed, May 29, 2024 at 01:12:12PM +0100, Pierre-Clément Tosi wrote:
> Ignore R_AARCH64_ABS32 relocations, instead of panicking, when emitting
> the relocation table of the hypervisor. The toolchain might produce them
> when generating function calls with kCFI to represent the 32-bit type ID
> which can then be resolved across compilation units at link time.  These
> are NOT actual 32-bit addresses and are therefore not needed in the
> final (runtime) relocation table (which is unlikely to use 32-bit
> absolute addresses for arm64 anyway).
> 
> Signed-off-by: Pierre-Clément Tosi <ptosi@google.com>
> ---
>  arch/arm64/kvm/hyp/nvhe/gen-hyprel.c | 6 ++++++
>  1 file changed, 6 insertions(+)

Thanks for updating the commit message:

Acked-by: Will Deacon <will@kernel.org>

Will

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

Re: [PATCH v4 07/13] KVM: arm64: VHE: Mark __hyp_call_panic __noreturn

[Will Deacon]

On Wed, May 29, 2024 at 01:12:13PM +0100, Pierre-Clément Tosi wrote:
> Given that the sole purpose of __hyp_call_panic() is to call panic(), a
> __noreturn function, give it the __noreturn attribute, removing the need
> for its caller to use unreachable().
> 
> Signed-off-by: Pierre-Clément Tosi <ptosi@google.com>
> ---
>  arch/arm64/kvm/hyp/vhe/switch.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)

I acked this before. Please can you preserve the tags when a patch is
reposted without any changes?

Acked-by: Will Deacon <will@kernel.org>

Will

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

Re: [PATCH v4 08/13] arm64: Introduce esr_comment() & esr_is_cfi_brk()

[Will Deacon]

On Wed, May 29, 2024 at 01:12:14PM +0100, Pierre-Clément Tosi wrote:
> As it is already used in two places, move esr_comment() to a header for
> re-use, with a clearer name.
> 
> Introduce esr_is_cfi_brk() to detect kCFI BRK syndromes, currently used
> by early_brk64() but soon to be also used by hypervisor code.
> 
> Signed-off-by: Pierre-Clément Tosi <ptosi@google.com>
> ---
>  arch/arm64/include/asm/esr.h       | 11 +++++++++++
>  arch/arm64/kernel/debug-monitors.c |  4 +---
>  arch/arm64/kernel/traps.c          |  8 +++-----
>  arch/arm64/kvm/handle_exit.c       |  2 +-
>  4 files changed, 16 insertions(+), 9 deletions(-)

(nit: typo in subject, should be esr_brk_comment()).

With that fixed:

Acked-by: Will Deacon <will@kernel.org>

Will

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

Re: [PATCH v4 09/13] KVM: arm64: Introduce print_nvhe_hyp_panic helper

[Will Deacon]

On Wed, May 29, 2024 at 01:12:15PM +0100, Pierre-Clément Tosi wrote:
> Add a helper to display a panic banner soon to also be used for kCFI
> failures, to ensure that we remain consistent.
> 
> Signed-off-by: Pierre-Clément Tosi <ptosi@google.com>
> ---
>  arch/arm64/kvm/handle_exit.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)

Acked-by: Will Deacon <will@kernel.org>

Will

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

Re: [PATCH v4 10/13] KVM: arm64: nVHE: Support CONFIG_CFI_CLANG at EL2

[Will Deacon]

On Wed, May 29, 2024 at 01:12:16PM +0100, Pierre-Clément Tosi wrote:
> The compiler implements kCFI by adding type information (u32) above
> every function that might be indirectly called and, whenever a function
> pointer is called, injects a read-and-compare of that u32 against the
> value corresponding to the expected type. In case of a mismatch, a BRK
> instruction gets executed. When the hypervisor triggers such an
> exception in nVHE, it panics and triggers and exception return to EL1.
> 
> Therefore, teach nvhe_hyp_panic_handler() to detect kCFI errors from the
> ESR and report them. If necessary, remind the user that EL2 kCFI is not
> affected by CONFIG_CFI_PERMISSIVE.
> 
> Pass $(CC_FLAGS_CFI) to the compiler when building the nVHE hyp code.
> 
> Use SYM_TYPED_FUNC_START() for __pkvm_init_switch_pgd, as nVHE can't
> call it directly and must use a PA function pointer from C (because it
> is part of the idmap page), which would trigger a kCFI failure if the
> type ID wasn't present.
> 
> Signed-off-by: Pierre-Clément Tosi <ptosi@google.com>
> ---
>  arch/arm64/kvm/handle_exit.c       | 10 ++++++++++
>  arch/arm64/kvm/hyp/nvhe/Makefile   |  6 +++---
>  arch/arm64/kvm/hyp/nvhe/hyp-init.S |  6 +++++-
>  3 files changed, 18 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/arm64/kvm/handle_exit.c b/arch/arm64/kvm/handle_exit.c
> index b3d6657a259d..69b08ac7322d 100644
> --- a/arch/arm64/kvm/handle_exit.c
> +++ b/arch/arm64/kvm/handle_exit.c
> @@ -417,6 +417,14 @@ 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)
> +{
> +	print_nvhe_hyp_panic("CFI failure", panic_addr);
> +
> +	if (IS_ENABLED(CONFIG_CFI_PERMISSIVE))
> +		kvm_err(" (CONFIG_CFI_PERMISSIVE ignored for hyp failures)\n");
> +}
> +
>  void __noreturn __cold nvhe_hyp_panic_handler(u64 esr, u64 spsr,
>  					      u64 elr_virt, u64 elr_phys,
>  					      u64 par, uintptr_t vcpu,
> @@ -446,6 +454,8 @@ void __noreturn __cold nvhe_hyp_panic_handler(u64 esr, u64 spsr,
>  			kvm_err("nVHE hyp BUG at: %s:%u!\n", file, line);
>  		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);
>  	} else {
>  		print_nvhe_hyp_panic("panic", panic_addr);
>  	}
> diff --git a/arch/arm64/kvm/hyp/nvhe/Makefile b/arch/arm64/kvm/hyp/nvhe/Makefile
> index 50fa0ffb6b7e..782b34b004be 100644
> --- a/arch/arm64/kvm/hyp/nvhe/Makefile
> +++ b/arch/arm64/kvm/hyp/nvhe/Makefile
> @@ -89,9 +89,9 @@ quiet_cmd_hyprel = HYPREL  $@
>  quiet_cmd_hypcopy = HYPCOPY $@
>        cmd_hypcopy = $(OBJCOPY) --prefix-symbols=__kvm_nvhe_ $< $@
>  
> -# Remove ftrace, Shadow Call Stack, and CFI CFLAGS.
> -# This is equivalent to the 'notrace', '__noscs', and '__nocfi' annotations.
> -KBUILD_CFLAGS := $(filter-out $(CC_FLAGS_FTRACE) $(CC_FLAGS_SCS) $(CC_FLAGS_CFI), $(KBUILD_CFLAGS))
> +# Remove ftrace and Shadow Call Stack CFLAGS.
> +# This is equivalent to the 'notrace' and '__noscs' annotations.
> +KBUILD_CFLAGS := $(filter-out $(CC_FLAGS_FTRACE) $(CC_FLAGS_SCS), $(KBUILD_CFLAGS))
>  # Starting from 13.0.0 llvm emits SHT_REL section '.llvm.call-graph-profile'
>  # when profile optimization is applied. gen-hyprel does not support SHT_REL and
>  # causes a build failure. Remove profile optimization flags.
> diff --git a/arch/arm64/kvm/hyp/nvhe/hyp-init.S b/arch/arm64/kvm/hyp/nvhe/hyp-init.S
> index d859c4de06b6..b1c8977e2812 100644
> --- a/arch/arm64/kvm/hyp/nvhe/hyp-init.S
> +++ b/arch/arm64/kvm/hyp/nvhe/hyp-init.S
> @@ -5,6 +5,7 @@
>   */
>  
>  #include <linux/arm-smccc.h>
> +#include <linux/cfi_types.h>
>  #include <linux/linkage.h>
>  
>  #include <asm/alternative.h>
> @@ -267,8 +268,11 @@ SYM_CODE_END(__kvm_handle_stub_hvc)
>  
>  /*
>   * void __pkvm_init_switch_pgd(phys_addr_t pgd, void *sp, void (*fn)(void));
> + *
> + * SYM_TYPED_FUNC_START() allows C to call this ID-mapped function indirectly
> + * using a physical pointer without triggering a kCFI failure.
>   */
> -SYM_FUNC_START(__pkvm_init_switch_pgd)
> +SYM_TYPED_FUNC_START(__pkvm_init_switch_pgd)
>  	/* Turn the MMU off */
>  	pre_disable_mmu_workaround
>  	mrs	x9, sctlr_el2

I still think this last hunk should be merged with the earlier patch
fixing up the prototype of __pkvm_init_switch_pgd().

With that:

Acked-by: Will Deacon <will@kernel.org>

Will

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

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

[Will Deacon]

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

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

Re: [PATCH v4 03/13] KVM: arm64: nVHE: Simplify __guest_exit_panic path

[Pierre-Clément Tosi]

Hi Will,

Thanks for the review; I will make sure to Cc you on v5, with your Acked-by.

On Mon, Jun 03, 2024 at 03:30:30PM +0100, Will Deacon wrote:
> On Wed, May 29, 2024 at 01:12:09PM +0100, Pierre-Clément Tosi wrote:
> > diff --git a/arch/arm64/kvm/hyp/nvhe/host.S b/arch/arm64/kvm/hyp/nvhe/host.S
> > index 135cfb294ee5..71fb311b4c0e 100644
> > --- a/arch/arm64/kvm/hyp/nvhe/host.S
> > +++ b/arch/arm64/kvm/hyp/nvhe/host.S
> > @@ -197,18 +197,13 @@ SYM_FUNC_END(__host_hvc)
> >  	sub	x0, sp, x0			// x0'' = sp' - x0' = (sp + x0) - sp = x0
> >  	sub	sp, sp, x0			// sp'' = sp' - x0 = (sp + x0) - x0 = sp
> >  
> > -	/* If a guest is loaded, panic out of it. */
> > -	stp	x0, x1, [sp, #-16]!
> > -	get_loaded_vcpu x0, x1
> > -	cbnz	x0, __guest_exit_panic
> > -	add	sp, sp, #16
> 
> I think this is actually dead code and we should just remove it. AFAICT,
> invalid_host_el2_vect is only used for the host vectors and the loaded
> vCPU will always be NULL, so this is pointless. set_loaded_vcpu() is
> only called by the low-level guest entry/exit code and with the guest
> EL2 vectors installed.

This is correct.

> > -
> >  	/*
> >  	 * The panic may not be clean if the exception is taken before the host
> >  	 * context has been saved by __host_exit or after the hyp context has
> >  	 * been partially clobbered by __host_enter.
> >  	 */
> > -	b	hyp_panic
> > +	stp	x0, x1, [sp, #-16]!
> > +	b	__guest_exit_panic
> 
> In which case, this should just be:
> 
> 	add	sp, sp, #16
> 	b	hyp_panic
> 
> Did I miss something?

Jumping to hyp_panic directly makes sense.

However, this patch keeps jumping to __guest_exit_panic() to prepare for the
kCFI changes as having a single point where all handlers (from various vectors)
panicking from assembly end up before branching to C turns out to be very
convenient for hooking in the kCFI handler (e.g.  when saving the registers, to
be parsed from C). I also didn't want to modify the same code twice in the
series and found it easier to limit the scope of this commit to a minimum by
following the existing code and keeping the same branch target.

With this in mind, please confirm if you still prefer this fix to jump to
hyp_panic directly (knowing the branch will be modified again in the series).

Also, I don't get why the 'add sp, sp, #16' is needed; what is it undoing?

Thanks,

Pierre

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

Re: [PATCH v4 05/13] KVM: arm64: Rename __guest_exit_panic __hyp_panic

[Pierre-Clément Tosi]

On Mon, Jun 03, 2024 at 03:34:24PM +0100, Will Deacon wrote:
> On Wed, May 29, 2024 at 01:12:11PM +0100, Pierre-Clément Tosi wrote:
> > Use a name that expresses the fact that the routine might not exit
> > through the guest but will always (directly or indirectly) end up
> > executing hyp_panic().
> > 
> > Use CPU_LR_OFFSET to clarify that the routine returns to hyp_panic().
> > 
> > Signed-off-by: Pierre-Clément Tosi <ptosi@google.com>
> > ---
> >  arch/arm64/kvm/hyp/entry.S              | 6 +++---
> >  arch/arm64/kvm/hyp/hyp-entry.S          | 2 +-
> >  arch/arm64/kvm/hyp/include/hyp/switch.h | 4 ++--
> >  arch/arm64/kvm/hyp/nvhe/host.S          | 4 ++--
> >  4 files changed, 8 insertions(+), 8 deletions(-)
> 
> Hmm, I'm not sure about this. When is __guest_exit_panic() called outside
> of guest context?

AFAICT, it is also called from

- the early __kvm_hyp_host_vector, installed by cpu_hyp_init_context()
- the flavors of __kvm_hyp_vector, installed by cpu_hyp_init_features()

which start handling exceptions long before the first guest can even be spawned.
Hence __guest_exit_panic() needing to validate the context on entry.

I don't get why those handlers didn't branch directly to hyp_panic() (perhaps to
have a more robust flow?) but, as mentioned in [1], it is convenient for kCFI to
be able to intercept all panic paths for sync exception from a single place.

[1]: https://lore.kernel.org/kvm/qob5gnca2nte4ggkrnn4uil5mfbkz3p55lmk3egpxstnumixfr@lq7xomrhf6za/

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

Re: [PATCH v4 10/13] KVM: arm64: nVHE: Support CONFIG_CFI_CLANG at EL2

[Pierre-Clément Tosi]

On Mon, Jun 03, 2024 at 03:45:30PM +0100, Will Deacon wrote:
> On Wed, May 29, 2024 at 01:12:16PM +0100, Pierre-Clément Tosi wrote:
> > diff --git a/arch/arm64/kvm/hyp/nvhe/hyp-init.S b/arch/arm64/kvm/hyp/nvhe/hyp-init.S
> > index d859c4de06b6..b1c8977e2812 100644
> > --- a/arch/arm64/kvm/hyp/nvhe/hyp-init.S
> > +++ b/arch/arm64/kvm/hyp/nvhe/hyp-init.S
> > @@ -5,6 +5,7 @@
> >   */
> >  
> >  #include <linux/arm-smccc.h>
> > +#include <linux/cfi_types.h>
> >  #include <linux/linkage.h>
> >  
> >  #include <asm/alternative.h>
> > @@ -267,8 +268,11 @@ SYM_CODE_END(__kvm_handle_stub_hvc)
> >  
> >  /*
> >   * void __pkvm_init_switch_pgd(phys_addr_t pgd, void *sp, void (*fn)(void));
> > + *
> > + * SYM_TYPED_FUNC_START() allows C to call this ID-mapped function indirectly
> > + * using a physical pointer without triggering a kCFI failure.
> >   */
> > -SYM_FUNC_START(__pkvm_init_switch_pgd)
> > +SYM_TYPED_FUNC_START(__pkvm_init_switch_pgd)
> >  	/* Turn the MMU off */
> >  	pre_disable_mmu_workaround
> >  	mrs	x9, sctlr_el2
> 
> I still think this last hunk should be merged with the earlier patch
> fixing up the prototype of __pkvm_init_switch_pgd().

Unfortunately, this is not possible because

  SYM_TYPED_FUNC_START(__pkvm_init_switch_pgd)

makes the assembler generate an unresolved symbol for the function type, which
the compiler only generates (from the C declaration) if it compiles with kCFI so
that moving this hunk to an earlier patch results in a linker error:

  ld.lld: error: undefined symbol: __kvm_nvhe___kcfi_typeid___pkvm_init_switch_pgd

OTOH, moving it to a later patch triggers a kCFI (runtime) panic.

As a result, this hunk *must* be part of this patch.

> 
> With that:
> 
> Acked-by: Will Deacon <will@kernel.org>

As I haven't followed your suggestion, I'll ignore this.

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

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

[Pierre-Clément Tosi]

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.

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

Re: [PATCH v4 03/13] KVM: arm64: nVHE: Simplify __guest_exit_panic path

[Will Deacon]

On Tue, Jun 04, 2024 at 04:48:02PM +0100, Pierre-Clément Tosi wrote:
> On Mon, Jun 03, 2024 at 03:30:30PM +0100, Will Deacon wrote:
> > On Wed, May 29, 2024 at 01:12:09PM +0100, Pierre-Clément Tosi wrote:
> > > diff --git a/arch/arm64/kvm/hyp/nvhe/host.S b/arch/arm64/kvm/hyp/nvhe/host.S
> > > index 135cfb294ee5..71fb311b4c0e 100644
> > > --- a/arch/arm64/kvm/hyp/nvhe/host.S
> > > +++ b/arch/arm64/kvm/hyp/nvhe/host.S
> > > @@ -197,18 +197,13 @@ SYM_FUNC_END(__host_hvc)
> > >  	sub	x0, sp, x0			// x0'' = sp' - x0' = (sp + x0) - sp = x0
> > >  	sub	sp, sp, x0			// sp'' = sp' - x0 = (sp + x0) - x0 = sp
> > >  
> > > -	/* If a guest is loaded, panic out of it. */
> > > -	stp	x0, x1, [sp, #-16]!
> > > -	get_loaded_vcpu x0, x1
> > > -	cbnz	x0, __guest_exit_panic
> > > -	add	sp, sp, #16
> > 
> > I think this is actually dead code and we should just remove it. AFAICT,
> > invalid_host_el2_vect is only used for the host vectors and the loaded
> > vCPU will always be NULL, so this is pointless. set_loaded_vcpu() is
> > only called by the low-level guest entry/exit code and with the guest
> > EL2 vectors installed.
> 
> This is correct.
> 
> > > -
> > >  	/*
> > >  	 * The panic may not be clean if the exception is taken before the host
> > >  	 * context has been saved by __host_exit or after the hyp context has
> > >  	 * been partially clobbered by __host_enter.
> > >  	 */
> > > -	b	hyp_panic
> > > +	stp	x0, x1, [sp, #-16]!
> > > +	b	__guest_exit_panic
> > 
> > In which case, this should just be:
> > 
> > 	add	sp, sp, #16
> > 	b	hyp_panic
> > 
> > Did I miss something?
> 
> Jumping to hyp_panic directly makes sense.
> 
> However, this patch keeps jumping to __guest_exit_panic() to prepare for the
> kCFI changes as having a single point where all handlers (from various vectors)
> panicking from assembly end up before branching to C turns out to be very
> convenient for hooking in the kCFI handler (e.g.  when saving the registers, to
> be parsed from C). I also didn't want to modify the same code twice in the
> series and found it easier to limit the scope of this commit to a minimum by
> following the existing code and keeping the same branch target.
> 
> With this in mind, please confirm if you still prefer this fix to jump to
> hyp_panic directly (knowing the branch will be modified again in the series).

I think having a patch which removes the dead code and has the
unconditional branch to hyp_panic is the best thing here. It might
change later on in the series, but it's a sensible patch on its own and,
with assembly, I think having small incremental changes is the best
option.

> Also, I don't get why the 'add sp, sp, #16' is needed; what is it undoing?

Oh, sorry, I missed that you'd dropped the stp earlier on. So the SP doesn't
need any adjusting and we can just branch to hyp_panic after the overflow
check.

Will

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

Re: [PATCH v4 05/13] KVM: arm64: Rename __guest_exit_panic __hyp_panic

[Will Deacon]

On Tue, Jun 04, 2024 at 04:51:58PM +0100, Pierre-Clément Tosi wrote:
> On Mon, Jun 03, 2024 at 03:34:24PM +0100, Will Deacon wrote:
> > On Wed, May 29, 2024 at 01:12:11PM +0100, Pierre-Clément Tosi wrote:
> > > Use a name that expresses the fact that the routine might not exit
> > > through the guest but will always (directly or indirectly) end up
> > > executing hyp_panic().
> > > 
> > > Use CPU_LR_OFFSET to clarify that the routine returns to hyp_panic().
> > > 
> > > Signed-off-by: Pierre-Clément Tosi <ptosi@google.com>
> > > ---
> > >  arch/arm64/kvm/hyp/entry.S              | 6 +++---
> > >  arch/arm64/kvm/hyp/hyp-entry.S          | 2 +-
> > >  arch/arm64/kvm/hyp/include/hyp/switch.h | 4 ++--
> > >  arch/arm64/kvm/hyp/nvhe/host.S          | 4 ++--
> > >  4 files changed, 8 insertions(+), 8 deletions(-)
> > 
> > Hmm, I'm not sure about this. When is __guest_exit_panic() called outside
> > of guest context?
> 
> AFAICT, it is also called from
> 
> - the early __kvm_hyp_host_vector, installed by cpu_hyp_init_context()

Well, we've just agreed to remove that one :)

> - the flavors of __kvm_hyp_vector, installed by cpu_hyp_init_features()

cpu_hyp_init_features() doesn't actually plumb the vector into VBAR,
though, so I still think that __guest_exit_panic() is only reachable
in guest context.

> which start handling exceptions long before the first guest can even be spawned.

I don't see how :/

Will

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

Re: [PATCH v4 10/13] KVM: arm64: nVHE: Support CONFIG_CFI_CLANG at EL2

[Will Deacon]

On Tue, Jun 04, 2024 at 05:04:40PM +0100, Pierre-Clément Tosi wrote:
> On Mon, Jun 03, 2024 at 03:45:30PM +0100, Will Deacon wrote:
> > On Wed, May 29, 2024 at 01:12:16PM +0100, Pierre-Clément Tosi wrote:
> > > diff --git a/arch/arm64/kvm/hyp/nvhe/hyp-init.S b/arch/arm64/kvm/hyp/nvhe/hyp-init.S
> > > index d859c4de06b6..b1c8977e2812 100644
> > > --- a/arch/arm64/kvm/hyp/nvhe/hyp-init.S
> > > +++ b/arch/arm64/kvm/hyp/nvhe/hyp-init.S
> > > @@ -5,6 +5,7 @@
> > >   */
> > >  
> > >  #include <linux/arm-smccc.h>
> > > +#include <linux/cfi_types.h>
> > >  #include <linux/linkage.h>
> > >  
> > >  #include <asm/alternative.h>
> > > @@ -267,8 +268,11 @@ SYM_CODE_END(__kvm_handle_stub_hvc)
> > >  
> > >  /*
> > >   * void __pkvm_init_switch_pgd(phys_addr_t pgd, void *sp, void (*fn)(void));
> > > + *
> > > + * SYM_TYPED_FUNC_START() allows C to call this ID-mapped function indirectly
> > > + * using a physical pointer without triggering a kCFI failure.
> > >   */
> > > -SYM_FUNC_START(__pkvm_init_switch_pgd)
> > > +SYM_TYPED_FUNC_START(__pkvm_init_switch_pgd)
> > >  	/* Turn the MMU off */
> > >  	pre_disable_mmu_workaround
> > >  	mrs	x9, sctlr_el2
> > 
> > I still think this last hunk should be merged with the earlier patch
> > fixing up the prototype of __pkvm_init_switch_pgd().
> 
> Unfortunately, this is not possible because
> 
>   SYM_TYPED_FUNC_START(__pkvm_init_switch_pgd)
> 
> makes the assembler generate an unresolved symbol for the function type, which
> the compiler only generates (from the C declaration) if it compiles with kCFI so
> that moving this hunk to an earlier patch results in a linker error:
> 
>   ld.lld: error: undefined symbol: __kvm_nvhe___kcfi_typeid___pkvm_init_switch_pgd
> 
> OTOH, moving it to a later patch triggers a kCFI (runtime) panic.
> 
> As a result, this hunk *must* be part of this patch.

Argh, thanks for the explanation. I thought CONFIG_CFI_CLANG would save
us here, but that's already enabled for the rest of the kernel so now I
understand what you mean.

> > With that:
> > 
> > Acked-by: Will Deacon <will@kernel.org>
> 
> As I haven't followed your suggestion, I'll ignore this.

You can keep the Ack.

Cheers,

Will

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

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

[Will Deacon]

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

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