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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 7 Feb 2022 17:24:11 -0300
From:   Leonardo Bras Soares Passos <leobras@...hat.com>
To:     Paolo Bonzini <pbonzini@...hat.com>
Cc:     Sean Christopherson <seanjc@...gle.com>,
        Vitaly Kuznetsov <vkuznets@...hat.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Jim Mattson <jmattson@...gle.com>,
        Joerg Roedel <joro@...tes.org>,
        Thomas Gleixner <tglx@...utronix.de>,
        Ingo Molnar <mingo@...hat.com>, Borislav Petkov <bp@...en8.de>,
        Dave Hansen <dave.hansen@...ux.intel.com>, x86@...nel.org,
        "H. Peter Anvin" <hpa@...or.com>, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v1 1/2] x86/kvm/fpu: Mask guest fpstate->xfeatures with guest_supported_xcr0

Hello Paolo,

On Mon, Feb 7, 2022 at 10:30 AM Paolo Bonzini <pbonzini@...hat.com> 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
[...]