From mboxrd@z Thu Jan 1 00:00:00 1970 Received: from smtp.kernel.org (aws-us-west-2-korg-mail-1.web.codeaurora.org [10.30.226.201]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by smtp.subspace.kernel.org (Postfix) with ESMTPS id A50BE1B7E4; Sun, 17 Mar 2024 12:26:25 +0000 (UTC) Authentication-Results: smtp.subspace.kernel.org; arc=none smtp.client-ip=10.30.226.201 ARC-Seal:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1710678386; cv=none; b=A+Uu82QDOsUlDskZ+XCYtbTyZ4+5uTm7BzNMwjXs7wFVqADl2CxHqDdlEQUtrrrDg0aR0jTfvunMoSniAeOLyfRndTx2Vzykt94dNxROuFG3B8M/UDJEDsTWz5qZoIb1YmvlHuWlEahk8G6xqBly4nWMxGocQ36/0OfI4UHBPaQ= ARC-Message-Signature:i=1; a=rsa-sha256; d=subspace.kernel.org; s=arc-20240116; t=1710678386; c=relaxed/simple; bh=iYxTMqDgxfAqrpKM39EoXettOKZYey71bShInFhDSTk=; h=Date:Message-ID:From:To:Cc:Subject:In-Reply-To:References: MIME-Version:Content-Type; b=oGYzC50/wzXqZYrZivsXpZGah4WCitz9TRRdkaZfgBfB82+QW+imLyP82kbxGgSkszQlyWGzWzQpriOTFsuj6Bs52lwxOeiXxWQnwTi4A4crKuk8kwUh0sAoflCFM2VQ10qlPuYQmUocDOotnrxVo9uzlcDnD/6ji2Oh9oOcg0E= ARC-Authentication-Results:i=1; smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b=Hj7oKJDa; arc=none smtp.client-ip=10.30.226.201 Authentication-Results: smtp.subspace.kernel.org; dkim=pass (2048-bit key) header.d=kernel.org header.i=@kernel.org header.b="Hj7oKJDa" Received: by smtp.kernel.org (Postfix) with ESMTPSA id 8C889C433F1; Sun, 17 Mar 2024 12:26:25 +0000 (UTC) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=kernel.org; s=k20201202; t=1710678385; bh=iYxTMqDgxfAqrpKM39EoXettOKZYey71bShInFhDSTk=; h=Date:From:To:Cc:Subject:In-Reply-To:References:From; b=Hj7oKJDa6uvMGqLUWJJEr92TqIWE1Nolf84xYqoALjfpEXHvFzZh4PmeE5KKUWbF9 IG8GULM/fNK7z//SG3PIR6xUT0PF4Eku0xao/q5XeIGnX6AJIQSHVzx88BjBjzMx7Z PLyTj5pkc2uH8AlqmTBJzApk9DgXHXjRMpNhUV9XzdbmnyGVVpuTJPerjeRy1ERW/0 EDz+189kO2gR6R+cfdtF/+jdwgxz8PlefQZJjCxWqIn7doGg0wBjbI/cSRTipQqV/w Mal+kgdvMRo5Nk0ImnorLQR3obfj6xEyPCbYhFw2iSmVqPqqbr9Vo623edB9E56GBE kda1cKMNh1IZg== Received: from sofa.misterjones.org ([185.219.108.64] helo=goblin-girl.misterjones.org) by disco-boy.misterjones.org with esmtpsa (TLS1.3) tls TLS_ECDHE_RSA_WITH_AES_256_GCM_SHA384 (Exim 4.95) (envelope-from ) id 1rlpap-00D1nT-63; Sun, 17 Mar 2024 12:26:23 +0000 Date: Sun, 17 Mar 2024 12:26:22 +0000 Message-ID: <86a5mx11u9.wl-maz@kernel.org> From: Marc Zyngier To: =?UTF-8?B?UGllcnJlLUNsw6ltZW50?= Tosi Cc: kvmarm@lists.linux.dev, linux-arm-kernel@lists.infradead.org, kvm@vger.kernel.org, James Morse , Suzuki K Poulose , Oliver Upton , Zenghui Yu , Andrew Scull Subject: Re: [PATCH 01/10] KVM: arm64: Fix clobbered ELR in sync abort In-Reply-To: <0dfcc4c5c898941147723ba530c81ddc8399ef55.1710446682.git.ptosi@google.com> References: <0dfcc4c5c898941147723ba530c81ddc8399ef55.1710446682.git.ptosi@google.com> User-Agent: Wanderlust/2.15.9 (Almost Unreal) SEMI-EPG/1.14.7 (Harue) FLIM-LB/1.14.9 (=?UTF-8?B?R29qxY0=?=) APEL-LB/10.8 EasyPG/1.0.0 Emacs/29.1 (aarch64-unknown-linux-gnu) MULE/6.0 (HANACHIRUSATO) Precedence: bulk X-Mailing-List: kvm@vger.kernel.org List-Id: List-Subscribe: List-Unsubscribe: MIME-Version: 1.0 (generated by SEMI-EPG 1.14.7 - "Harue") Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: quoted-printable X-SA-Exim-Connect-IP: 185.219.108.64 X-SA-Exim-Rcpt-To: ptosi@google.com, kvmarm@lists.linux.dev, linux-arm-kernel@lists.infradead.org, kvm@vger.kernel.org, james.morse@arm.com, suzuki.poulose@arm.com, oliver.upton@linux.dev, yuzenghui@huawei.com, ascull@google.com X-SA-Exim-Mail-From: maz@kernel.org X-SA-Exim-Scanned: No (on disco-boy.misterjones.org); SAEximRunCond expanded to false On Thu, 14 Mar 2024 20:23:22 +0000, Pierre-Cl=C3=A9ment Tosi wrote: >=20 > When the hypervisor receives a SError or synchronous exception (EL2h) nit: since this also affects SError, $subject seems wrong. > 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. This is done (instead of a simple function call) > to ensure that the panic handler runs with the SPSel that was in use > when the exception was triggered, necessary to support features such as > the shadow call stack. I don't understand this. Who changes SPsel? At any given point, SP_EL0 is that of either the host or the guest, but never the hypervisor's. If something was touching SP_EL0 behind our back, it would result in corruption (see 6e977984f6d8 for a bit of rationale about it). > > 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. >=20 > Therefore, store the original ELR_EL2 in a per-CPU struct and point the Can you elaborate on *which* per-CPU structure you are using? > sysreg to a routine that first restores it to its previous value before > running __guest_exit_panic. >=20 > Fixes: 7db21530479f ("KVM: arm64: Restore hyp when panicking in guest con= text") > Signed-off-by: Pierre-Cl=C3=A9ment Tosi > --- > arch/arm64/kernel/asm-offsets.c | 1 + > arch/arm64/kvm/hyp/entry.S | 9 +++++++++ > arch/arm64/kvm/hyp/include/hyp/switch.h | 6 ++++-- > 3 files changed, 14 insertions(+), 2 deletions(-) >=20 > diff --git a/arch/arm64/kernel/asm-offsets.c b/arch/arm64/kernel/asm-offs= ets.c > index 5a7dbbe0ce63..e62353168a57 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_E= L1])); > DEFINE(CPU_GCR_EL1, offsetof(struct kvm_cpu_context, sys_regs[GCR_EL1= ])); > DEFINE(CPU_APIAKEYLO_EL1, offsetof(struct kvm_cpu_context, sys_regs[AP= IAKEYLO_EL1])); > diff --git a/arch/arm64/kvm/hyp/entry.S b/arch/arm64/kvm/hyp/entry.S > index f3aa7738b477..9cdf46da3051 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 > =20 > +SYM_INNER_LABEL(__guest_exit_panic_with_restored_elr, SYM_L_GLOBAL) The name of the function isn't great. Crucially, 'with_restored_elr' implies that ELR is already restored, while it is the hunk below that does the 'restore' part. Something like '__guest_exit_restore_elr_and_panic' seems more appropriate. > + // 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 > + > 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 a038320cdb08..6a8dc8d3c193 100644 > --- a/arch/arm64/kvm/hyp/include/hyp/switch.h > +++ b/arch/arm64/kvm/hyp/include/hyp/switch.h > @@ -747,7 +747,7 @@ static inline bool fixup_guest_exit(struct kvm_vcpu *= vcpu, u64 *exit_code) > =20 > static inline void __kvm_unexpected_el2_exception(void) > { > - extern char __guest_exit_panic[]; > + extern char __guest_exit_panic_with_restored_elr[]; > unsigned long addr, fixup; > struct kvm_exception_table_entry *entry, *end; > unsigned long elr_el2 =3D read_sysreg(elr_el2); > @@ -769,7 +769,9 @@ static inline void __kvm_unexpected_el2_exception(voi= d) > } > =20 > /* Trigger a panic after restoring the hyp context. */ > - write_sysreg(__guest_exit_panic, elr_el2); > + write_sysreg(__guest_exit_panic_with_restored_elr, elr_el2); > + > + this_cpu_ptr(&kvm_hyp_ctxt)->sys_regs[ELR_EL2] =3D elr_el2; nit: placing the saving of ELR_EL2 before overriding the sysreg makes the whole thing a bit more readable, specially given the poor choice of 'elr_el2' as a variable (visually clashing with 'elr_el2' as a system register). Thanks, M. --=20 Without deviation from the norm, progress is not possible.