Re: [PATCH v1 2/2] KVM: arm64: Trap access to SMPRI_EL1 in VHE mode

From: Marc Zyngier <maz@kernel.org>
To: Mark Brown <broonie@kernel.org>
Cc: Catalin Marinas <catalin.marinas@arm.com>,
	Will Deacon <will@kernel.org>,
	Peter Maydell <peter.maydell@linaro.org>,
	Richard Henderson <richard.henderson@linaro.org>,
	Vincent Donnefort <vdonnefort@google.com>,
	James Morse <james.morse@arm.com>,
	Alexandru Elisei <alexandru.elisei@arm.com>,
	Suzuki K Poulose <suzuki.poulose@arm.com>,
	Oliver Upton <oliver.upton@linux.dev>,
	linux-arm-kernel@lists.infradead.org, kvmarm@lists.linux.dev,
	stable@vger.kernel.org
Subject: Re: [PATCH v1 2/2] KVM: arm64: Trap access to SMPRI_EL1 in VHE mode
Date: Mon, 31 Oct 2022 09:45:48 +0000	[thread overview]
Message-ID: <86eduoe377.wl-maz@kernel.org> (raw)
In-Reply-To: <20221027210441.814061-3-broonie@kernel.org>

On Thu, 27 Oct 2022 22:04:40 +0100,
Mark Brown <broonie@kernel.org> wrote:
> 
> On systems with SME access to the SMPRI_EL1 priority management register is
> controlled by the nSMPRI_EL1 fine grained trap. We manage this trap in nVHE
> mode but do not do so when in VHE mode, add the required management.
> 
> On systems which do not implement priority mapping not enabling this trap
> will allow the guest to discover if the host support SME since the register
> will be RES0 rather than UNDEF. On systems implementing priority mapping
> the register could be used as a side channel by guests.
> 
> Fixes: 861262ab8627 ("KVM: arm64: Handle SME host state when running guests")
> Signed-off-by: Mark Brown <broonie@kernel.org>
> Cc: stable@vger.kernel.org
> ---
>  arch/arm64/kvm/hyp/vhe/switch.c | 24 ++++++++++++++++++++++--
>  1 file changed, 22 insertions(+), 2 deletions(-)
> 
> diff --git a/arch/arm64/kvm/hyp/vhe/switch.c b/arch/arm64/kvm/hyp/vhe/switch.c
> index 7acb87eaa092..cae581e8dd56 100644
> --- a/arch/arm64/kvm/hyp/vhe/switch.c
> +++ b/arch/arm64/kvm/hyp/vhe/switch.c
> @@ -63,10 +63,20 @@ static void __activate_traps(struct kvm_vcpu *vcpu)
>  		__activate_traps_fpsimd32(vcpu);
>  	}
>  
> -	if (cpus_have_final_cap(ARM64_SME))
> +	if (cpus_have_final_cap(ARM64_SME)) {
>  		write_sysreg(read_sysreg(sctlr_el2) & ~SCTLR_ELx_ENTP2,
>  			     sctlr_el2);
>  
> +		/*
> +		 * Disable access to SMPRI_EL1 - we don't need to control
> +		 * nTPIDR2_EL0 in VHE mode.

It really isn't obvious to me why this is the case. The pseudocode
says for a 'MSR TPIDR2_EL0, <Xt>' (DDI0616 A.a p225):

<quote>
elsif PSTATE.EL == EL1 then
	if Halted() && HaveEL(EL3) && EDSCR.SDD == '1' &&
	   boolean IMPLEMENTATION_DEFINED "EL3 trap priority, when SDD == '1'" &&
	   SCR_EL3.EnTP2 == '0' then
		UNDEFINED;
	elsif EL2Enabled() && (!HaveEL(EL3) || SCR_EL3.FGTEn == '1') &&
	   HFGWTR_EL2.nTPIDR2_EL0 == '0' then
		AArch64.SystemAccessTrap(EL2, 0x18);
	elsif HaveEL(EL3) && SCR_EL3.EnTP2 == '0' then
		if Halted() && EDSCR.SDD == '1' then
			UNDEFINED;
		else
			AArch64.SystemAccessTrap(EL3, 0x18);
	else
		TPIDR2_EL0 = X[t, 64];
</quote>

So when running at EL1, and short of clearing nTPIDR2_EL0, EL1 will
have access to TPIDR2_EL0. What prevents that?

The write to SCTLR_EL2.EnTP2 is also pretty dubious, and doesn't
really cover the access to EL0 (think SCTLR_EL1.EnTP2=1 and
HCR_EL2.{E2H,TGE}={1,0}, for example).

	M.

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