Message-ID: <86plhakbq4.wl-maz@kernel.org>
Date: Thu, 17 Apr 2025 17:13:23 +0100
From: Marc Zyngier <maz@...nel.org>
To: D Scott Phillips <scott@...amperecomputing.com>
Cc: Catalin Marinas <catalin.marinas@....com>,
	Joey Gouly
 <joey.gouly@....com>,
	Oliver Upton <oliver.upton@...ux.dev>,
	Suzuki K
 Poulose <suzuki.poulose@....com>,
	Will Deacon <will@...nel.org>,
	Zenghui
 Yu <yuzenghui@...wei.com>,
	kvmarm@...ts.linux.dev,
	linux-arm-kernel@...ts.infradead.org,
	linux-kernel@...r.kernel.org
Subject: Re: [PATCH 1/2] KVM: arm64: fix config.hcr used uninitialized in __kvm_at_s1e01_fast

On Thu, 17 Apr 2025 00:00:39 +0100,
D Scott Phillips <scott@...amperecomputing.com> wrote:
> 
> Marc Zyngier <maz@...nel.org> writes:
> 
> > On Tue, 15 Apr 2025 16:46:55 +0100,
> > D Scott Phillips <scott@...amperecomputing.com> wrote:
> >> 
> >> In the skip_mmu_switch case, config.hcr was used uninitialized. On my
> >> machine that caused garbage to be written to HCR_EL2 and then the CPU
> >> got stuck at the synchronous exception handler. Also, the restore of
> >> HCR_EL2 was missing at the end of the function in the same case.
> >
> > Huh, how embarrassing. Thanks for spotting this one.
> >
> >> 
> >> In skip_mmu_switch case, initialize config.hcr with HCR_HOST_VHE_FLAGS.
> >> 
> >> Signed-off-by: D Scott Phillips <scott@...amperecomputing.com>
> >> ---
> >>  arch/arm64/kvm/at.c | 8 ++++++--
> >>  1 file changed, 6 insertions(+), 2 deletions(-)
> >> 
> >> diff --git a/arch/arm64/kvm/at.c b/arch/arm64/kvm/at.c
> >> index f74a66ce3064b..ff4b06ce661af 100644
> >> --- a/arch/arm64/kvm/at.c
> >> +++ b/arch/arm64/kvm/at.c
> >> @@ -1233,8 +1233,10 @@ static u64 __kvm_at_s1e01_fast(struct kvm_vcpu *vcpu, u32 op, u64 vaddr)
> >>  	 * the right one (as we trapped from vEL2). If not, save the
> >>  	 * full MMU context.
> >>  	 */
> >> -	if (vcpu_el2_e2h_is_set(vcpu) && vcpu_el2_tge_is_set(vcpu))
> >> +	if (vcpu_el2_e2h_is_set(vcpu) && vcpu_el2_tge_is_set(vcpu)) {
> >> +		config.hcr = read_sysreg(hcr_el2);
> >>  		goto skip_mmu_switch;
> >> +	}
> >>  
> >>  	/*
> >>  	 * Obtaining the S2 MMU for a L2 is horribly racy, and we may not
> >> @@ -1299,7 +1301,9 @@ static u64 __kvm_at_s1e01_fast(struct kvm_vcpu *vcpu, u32 op, u64 vaddr)
> >>  	if (!fail)
> >>  		par = read_sysreg_par();
> >>  
> >> -	if (!(vcpu_el2_e2h_is_set(vcpu) && vcpu_el2_tge_is_set(vcpu)))
> >> +	if (vcpu_el2_e2h_is_set(vcpu) && vcpu_el2_tge_is_set(vcpu))
> >> +		write_sysreg(config.hcr, hcr_el2);
> >> +	else
> >>  		__mmu_config_restore(&config);
> >>  
> >>  	return par;
> >
> > I think the diff below should do the trick (and incidently matches
> > your commit message).
> 
> Looks good Marc, thanks
> 
> Reviewed-by: D Scott Phillips <scott@...amperecomputing.com>

Actually, we can do much better, because the more I look at this, the
more I find it silly. How about the patch below? It also fixes a
latent issue where HCR_PTW was never set... I quickly tested it on my
Q box, and nothing exploded. But at the same time, nothing exploded
with the buggy code...

Thanks,

	M.

From 0191a6dfbeb52b41b2c8aa81edf7812b56a3612f Mon Sep 17 00:00:00 2001
From: Marc Zyngier <maz@...nel.org>
Date: Thu, 17 Apr 2025 16:24:29 +0100
Subject: [PATCH] KVM: arm64: Don't feed uninitialised data to HCR_EL2

When the guest executes an AT S1E{0,1} from EL2, and that its
HCR_EL2.{E2H,TGE}=={1,1}, then this is a pure S1 translation
that doesn't involve a guest-supplied S2, and the full S1
context is already in place. This allows us to take a shortcut
and avoid save/restoring a bunch of registers.

However, we set HCR_EL2 to a value suitable for the use of AT
in guest context. And we do so by using the value that we saved.
Or not. In the case described above, we restore whatever junk
was on the stack, and carry on with it until the next entry.

Needless to say, this is completely broken.

But this also triggers the realisation that saving HCR_EL2 is
a bit pointless. We are always in host context at the point where
reach this code, and what we program to enter the guest is a known
value (vcpu->arch.hcr_el2).

Drop the pointless save/restore, and wrap the AT operations with
writes that switch between guest and host values for HCR_EL2.

Reported-by: D Scott Phillips <scott@...amperecomputing.com>
Signed-off-by: Marc Zyngier <maz@...nel.org>
---
 arch/arm64/kvm/at.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/arch/arm64/kvm/at.c b/arch/arm64/kvm/at.c
index ea497ffbd1b19..da5359668b9c9 100644
--- a/arch/arm64/kvm/at.c
+++ b/arch/arm64/kvm/at.c
@@ -464,7 +464,6 @@ struct mmu_config {
 	u64	sctlr;
 	u64	vttbr;
 	u64	vtcr;
-	u64	hcr;
 };
 
 static void __mmu_config_save(struct mmu_config *config)
@@ -487,13 +486,10 @@ static void __mmu_config_save(struct mmu_config *config)
 	config->sctlr	= read_sysreg_el1(SYS_SCTLR);
 	config->vttbr	= read_sysreg(vttbr_el2);
 	config->vtcr	= read_sysreg(vtcr_el2);
-	config->hcr	= read_sysreg(hcr_el2);
 }
 
 static void __mmu_config_restore(struct mmu_config *config)
 {
-	write_sysreg(config->hcr,	hcr_el2);
-
 	/*
 	 * ARM errata 1165522 and 1530923 require TGE to be 1 before
 	 * we update the guest state.
@@ -1248,8 +1244,8 @@ static u64 __kvm_at_s1e01_fast(struct kvm_vcpu *vcpu, u32 op, u64 vaddr)
 	__load_stage2(mmu, mmu->arch);
 
 skip_mmu_switch:
-	/* Clear TGE, enable S2 translation, we're rolling */
-	write_sysreg((config.hcr & ~HCR_TGE) | HCR_VM,	hcr_el2);
+	/* Temporarily switch back to guest context */
+	write_sysreg(vcpu->arch.hcr_el2, hcr_el2);
 	isb();
 
 	switch (op) {
@@ -1281,6 +1277,8 @@ static u64 __kvm_at_s1e01_fast(struct kvm_vcpu *vcpu, u32 op, u64 vaddr)
 	if (!fail)
 		par = read_sysreg_par();
 
+	write_sysreg(HCR_HOST_VHE_FLAGS, hcr_el2);
+
 	if (!(vcpu_el2_e2h_is_set(vcpu) && vcpu_el2_tge_is_set(vcpu)))
 		__mmu_config_restore(&config);
 
-- 
2.39.2


-- 
Without deviation from the norm, progress is not possible.

Powered by blists - more mailing lists