[PATCH 01/18] KVM: arm64: Always start with clearing SVE flag on load

[PATCH 01/18] KVM: arm64: Always start with clearing SVE flag on load

[Marc Zyngier]

On each vcpu load, we set the KVM_ARM64_HOST_SVE_ENABLED
flag if SVE is enabled for EL0 on the host. This is used to restore
the correct state on vpcu put.

However, it appears that nothing ever clears this flag. Once
set, it will stick until the vcpu is destroyed, which has the
potential to spuriously enable SVE for userspace.

We probably never saw the issue because no VMM uses SVE, but
that's still pretty bad. Unconditionally clearing the flag
on vcpu load addresses the issue.

Fixes: 8383741ab2e7 ("KVM: arm64: Get rid of host SVE tracking/saving")
Signed-off-by: Marc Zyngier <maz@kernel.org>
Cc: stable@vger.kernel.org
---
 arch/arm64/kvm/fpsimd.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/arm64/kvm/fpsimd.c b/arch/arm64/kvm/fpsimd.c
index 441edb9c398c..3c2cfc3adc51 100644
--- a/arch/arm64/kvm/fpsimd.c
+++ b/arch/arm64/kvm/fpsimd.c
@@ -80,6 +80,7 @@ void kvm_arch_vcpu_load_fp(struct kvm_vcpu *vcpu)
 	vcpu->arch.flags &= ~KVM_ARM64_FP_ENABLED;
 	vcpu->arch.flags |= KVM_ARM64_FP_HOST;
 
+	vcpu->arch.flags &= ~KVM_ARM64_HOST_SVE_ENABLED;
 	if (read_sysreg(cpacr_el1) & CPACR_EL1_ZEN_EL0EN)
 		vcpu->arch.flags |= KVM_ARM64_HOST_SVE_ENABLED;
 
-- 
2.34.1

Re: [PATCH 01/18] KVM: arm64: Always start with clearing SVE flag on load

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 962 bytes --]

On Sat, May 28, 2022 at 12:38:11PM +0100, Marc Zyngier wrote:
> On each vcpu load, we set the KVM_ARM64_HOST_SVE_ENABLED
> flag if SVE is enabled for EL0 on the host. This is used to restore
> the correct state on vpcu put.
> 
> However, it appears that nothing ever clears this flag. Once
> set, it will stick until the vcpu is destroyed, which has the
> potential to spuriously enable SVE for userspace.

Oh dear.

Reviewed-by: Mark Brown <broonie@kernel.org>

> We probably never saw the issue because no VMM uses SVE, but
> that's still pretty bad. Unconditionally clearing the flag
> on vcpu load addresses the issue.

Unless I'm missing something since we currently always disable
SVE on syscall even if the VMM were using SVE for some reason
(SVE memcpy()?) we should already have disabled SVE for EL0 in
sve_user_discard() during kernel entry so EL0 access to SVE
should be disabled in the system register by the time we get
here.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]

Re: [PATCH 01/18] KVM: arm64: Always start with clearing SVE flag on load

[Marc Zyngier]

On Mon, 30 May 2022 15:41:54 +0100,
Mark Brown <broonie@kernel.org> wrote:
> 
> [1  <text/plain; us-ascii (quoted-printable)>]
> On Sat, May 28, 2022 at 12:38:11PM +0100, Marc Zyngier wrote:
> > On each vcpu load, we set the KVM_ARM64_HOST_SVE_ENABLED
> > flag if SVE is enabled for EL0 on the host. This is used to restore
> > the correct state on vpcu put.
> > 
> > However, it appears that nothing ever clears this flag. Once
> > set, it will stick until the vcpu is destroyed, which has the
> > potential to spuriously enable SVE for userspace.
> 
> Oh dear.
> 
> Reviewed-by: Mark Brown <broonie@kernel.org>
> 
> > We probably never saw the issue because no VMM uses SVE, but
> > that's still pretty bad. Unconditionally clearing the flag
> > on vcpu load addresses the issue.
> 
> Unless I'm missing something since we currently always disable
> SVE on syscall even if the VMM were using SVE for some reason
> (SVE memcpy()?) we should already have disabled SVE for EL0 in
> sve_user_discard() during kernel entry so EL0 access to SVE
> should be disabled in the system register by the time we get
> here.

Indeed. And this begs the question: what is this code actually doing?
Is there any way we can end-up running a guest with any valid host SVE
state?

I remember being >this< close to removing that code some time ago, and
only stopped because I vaguely remembered Dave Martin convincing me at
some point that it was necessary. I'm unable to piece the argument
together again though.

	M.

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

Re: [PATCH 01/18] KVM: arm64: Always start with clearing SVE flag on load

[Mark Brown]

[-- Attachment #1: Type: text/plain, Size: 1843 bytes --]

On Mon, Jun 06, 2022 at 12:28:32PM +0100, Marc Zyngier wrote:
> Mark Brown <broonie@kernel.org> wrote:
> > On Sat, May 28, 2022 at 12:38:11PM +0100, Marc Zyngier wrote:

> > > We probably never saw the issue because no VMM uses SVE, but
> > > that's still pretty bad. Unconditionally clearing the flag
> > > on vcpu load addresses the issue.

> > Unless I'm missing something since we currently always disable
> > SVE on syscall even if the VMM were using SVE for some reason
> > (SVE memcpy()?) we should already have disabled SVE for EL0 in
> > sve_user_discard() during kernel entry so EL0 access to SVE
> > should be disabled in the system register by the time we get
> > here.

> Indeed. And this begs the question: what is this code actually doing?
> Is there any way we can end-up running a guest with any valid host SVE
> state?

> I remember being >this< close to removing that code some time ago, and
> only stopped because I vaguely remembered Dave Martin convincing me at
> some point that it was necessary. I'm unable to piece the argument
> together again though.

I've stared at that code a few times as well, I think I'd ended up
assuming it was some path to do with preempting and context switching
but in that case I've never been clear why there'd be anything left that
we'd need to preserve, or if we do why we don't just force a
fpsimd_save().  It's possible this was from some earlier stage in review
where the ABI didn't allow us to discard the SVE register state, or that
it's there as defensive programming so for future work where we don't
just disable on entry.

Conicidentally I am going to post some patches later today or tomorrow
which leave SVE enabled on syscall, they still have the hook for
disabling it when entering KVM though so we'd still not need to save the
EL0 state and the above should still apply.

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 488 bytes --]