On Fri, May 10, 2024 at 12:26:30PM +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@xxxxxxxxxx>
> ---
> arch/arm64/kernel/asm-offsets.c | 1 +
> arch/arm64/kvm/hyp/entry.S | 9 +++++++++
> arch/arm64/kvm/hyp/include/hyp/switch.h | 5 +++--
> 3 files changed, 13 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..bcaaf1a11b4e 100644
> --- a/arch/arm64/kvm/hyp/entry.S
> +++ b/arch/arm64/kvm/hyp/entry.S
> @@ -83,6 +83,15 @@ alternative_else_nop_endif
> eret
> sb
>
> +SYM_INNER_LABEL(__guest_exit_restore_elr_and_panic, SYM_L_GLOBAL)
> + // x0-x29,lr: hyp regs
> +
> + stp x0, x1, [sp, #-16]!
> + adr_this_cpu x0, kvm_hyp_ctxt, x1
> + ldr x0, [x0, #CPU_ELR_EL2]
> + msr elr_el2, x0
> + ldp x0, x1, [sp], #16
Why do you have to preserve x0 and x1 here? afaict, we fall into
__guest_exit_panic(), which clobbers them both immediately because it's
going to pull them off the stack (they get saved _very_ early during
exception entry).
Will
