On Wed, Aug 17, 2022, Dr. David Alan Gilbert wrote:
> * Sean Christopherson (seanjc@xxxxxxxxxx) wrote:
> > On Tue, Aug 16, 2022, Dr. David Alan Gilbert (git) wrote:
> > > diff --git a/arch/x86/kvm/cpuid.c b/arch/x86/kvm/cpuid.c
> > > index de6d44e07e34..3b2319cecfd1 100644
> > > --- a/arch/x86/kvm/cpuid.c
> > > +++ b/arch/x86/kvm/cpuid.c
> > > @@ -298,7 +298,8 @@ static void kvm_vcpu_after_set_cpuid(struct kvm_vcpu *vcpu)
> > > guest_supported_xcr0 =
> > > cpuid_get_supported_xcr0(vcpu->arch.cpuid_entries, vcpu->arch.cpuid_nent);
> > >
> > > - vcpu->arch.guest_fpu.fpstate->user_xfeatures = guest_supported_xcr0;
> > > + vcpu->arch.guest_fpu.fpstate->user_xfeatures = guest_supported_xcr0 |
> > > + XFEATURE_MASK_FPSSE;
>
> Hi Sean,
> Thanks for the reply,
>
> > I don't think this is correct. This will allow the guest to set the SSE bit
> > even when XSAVE isn't supported due to kvm_guest_supported_xcr0() returning
> > user_xfeatures.
> >
> > static inline u64 kvm_guest_supported_xcr0(struct kvm_vcpu *vcpu)
> > {
> > return vcpu->arch.guest_fpu.fpstate->user_xfeatures;
> > }
> >
> > I believe the right place to fix this is in validate_user_xstate_header(). It's
> > reachable if and only if XSAVE is supported in the host, and when XSAVE is _not_
> > supported, the kernel unconditionally allows FP+SSE. So it follows that the kernel
> > should also allow FP+SSE when using XSAVE too. That would also align the logic
> > with fpu_copy_guest_fpstate_to_uabi(), which fordces the FPSSE flags. Ditto for
> > the non-KVM save_xstate_epilog().
>
> OK, yes, I'd followed the check that failed down to this test; although
> by itself this test works until Leo's patch came along later; so I
> wasn't sure where to fix it.
>
> > Aha! And fpu__init_system_xstate() ensure the host supports FP+SSE when XSAVE
> > is enabled (knew their had to be a sanity check somewhere).
> >
> > ---
> > arch/x86/kernel/fpu/xstate.c | 9 +++++++--
> > 1 file changed, 7 insertions(+), 2 deletions(-)
> >
> > diff --git a/arch/x86/kernel/fpu/xstate.c b/arch/x86/kernel/fpu/xstate.c
> > index c8340156bfd2..83b9a9653d47 100644
> > --- a/arch/x86/kernel/fpu/xstate.c
> > +++ b/arch/x86/kernel/fpu/xstate.c
> > @@ -399,8 +399,13 @@ int xfeature_size(int xfeature_nr)
> > static int validate_user_xstate_header(const struct xstate_header *hdr,
> > struct fpstate *fpstate)
> > {
> > - /* No unknown or supervisor features may be set */
> > - if (hdr->xfeatures & ~fpstate->user_xfeatures)
> > + /*
> > + * No unknown or supervisor features may be set. Userspace is always
> > + * allowed to restore FP+SSE state (XSAVE/XRSTOR are used by the kernel
> > + * if and only if FP+SSE are supported in xstate).
> > + */
> > + if (hdr->xfeatures & ~fpstate->user_xfeatures &
> > + ~(XFEATURE_MASK_FP | XFEATURE_MASK_SSE))
> > return -EINVAL;
> >
> > /* Userspace must use the uncompacted format */
>
> That passes the small smoke test for me; will you repost that then?
*sigh*
The bug is more subtle than just failing to restore. Saving can also "fail". If
XSAVE is hidden from the guest on an XSAVE-capable host, __copy_xstate_to_uabi_buf()
will happily reinitialize FP+SSE state and thus corrupt guest FPU state on migration.
And not that it matters now, but before realizing that KVM_GET_XSAVE is also broken,
I decided I like Dave's patch better because KVM really should separate what userspace
can save/restore from what the guest can access.
Amusingly, there's actually another bug lurking with respect to usurping user_xfeatures
to represent supported_guest_xcr0. The latter is zero-initialized, whereas
user_xfeatures is set to the "default" features on initialization, i.e. migrating a
VM without ever doing KVM_SET_CPUID2 would do odd things.
Sending a v2 shortly to reinstate guest_supported_xcr0 before landing Dave's patch.
