Re: [PATCH v1 1/2] x86/kvm/fpu: Mask guest fpstate->xfeatures with guest_supported_xcr0

[Leonardo Bras Soares Passos]

Hello Paolo,

On Mon, Feb 7, 2022 at 10:30 AM Paolo Bonzini <pbonzini@xxxxxxxxxx> wrote:
>
> On 2/5/22 09:16, Leonardo Bras wrote:
> > During host/guest switch (like in kvm_arch_vcpu_ioctl_run()), the kernel
> > swaps the fpu between host/guest contexts, by using fpu_swap_kvm_fpstate().
> >
> > When xsave feature is available, the fpu swap is done by:
> > - xsave(s) instruction, with guest's fpstate->xfeatures as mask, is used
> >    to store the current state of the fpu registers to a buffer.
> > - xrstor(s) instruction, with (fpu_kernel_cfg.max_features &
> >    XFEATURE_MASK_FPSTATE) as mask, is used to put the buffer into fpu regs.
> >
> > For xsave(s) the mask is used to limit what parts of the fpu regs will
> > be copied to the buffer. Likewise on xrstor(s), the mask is used to
> > limit what parts of the fpu regs will be changed.
> >
> > The mask for xsave(s), the guest's fpstate->xfeatures, is defined on
> > kvm_arch_vcpu_create(), which (in summary) sets it to all features
> > supported by the cpu which are enabled on kernel config.
> >
> > This means that xsave(s) will save to guest buffer all the fpu regs
> > contents the cpu has enabled when the guest is paused, even if they
> > are not used.
> >
> > This would not be an issue, if xrstor(s) would also do that.
> >
> > xrstor(s)'s mask for host/guest swap is basically every valid feature
> > contained in kernel config, except XFEATURE_MASK_PKRU.
> > According to kernel src, it is instead switched in switch_to() and
> > flush_thread().
>
> Hi Leonardo, is this an issue when patch 2 is applied?

Yes.
This issue happens on host/guest context switch, instead of KVM_{GET,SET}_XSAVE,
so this bug will be triggered whenever the guest doesn't support PKRU
but the host
does, without any interference of above IOCTLs.
In fact, IIUC,  even if we are able to fix the feature bit with
KVM_SET_XSAVE, it would
come back after another host/guest context switch if we don't fix
vcpu->arch.guest_fpu.fpstate->xfeatures.

> With this patch,
> we have to reason about the effect of calling KVM_SET_CPUID2 twice calls
> back to back.  I think an "&=" would be wrong in that case.

So, you suggest something like this ?

vcpu->arch.guest_fpu.fpstate->xfeatures =
        fpu_user_cfg.default_features & vcpu->arch.guest_supported_xcr0;


>
> On the other hand, with patch 2 the change is only in the KVM_SET_XSAVE
> output, which is much more self-contained.

Agree, but they solve different sources of the same issue.
Patch 2 will only address a bug that can happen if userspace mistakenly
tries to set a feature the guest does not support.

>
> Thanks,

Thank you!

Best regards,
Leo
[...]