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

From: Will Deacon <will@kernel.org>
To: "Pierre-Clément Tosi" <ptosi@google.com>
Cc: kvmarm@lists.linux.dev, linux-arm-kernel@lists.infradead.org,
	kvm@vger.kernel.org, Marc Zyngier <maz@kernel.org>,
	Oliver Upton <oliver.upton@linux.dev>,
	Suzuki K Poulose <suzuki.poulose@arm.com>,
	Vincent Donnefort <vdonnefort@google.com>
Subject: Re: [PATCH v3 01/12] KVM: arm64: Fix clobbered ELR in sync abort/SError
Date: Mon, 13 May 2024 14:55:42 +0100	[thread overview]
Message-ID: <20240513135542.GA28749@willie-the-truck> (raw)
In-Reply-To: <20240510112645.3625702-2-ptosi@google.com>

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@google.com>
> ---
>  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

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