Re: [PATCH] kvm: x86: Don't dirty guest memory on every vcpu_put()

[Sean Christopherson <seanjc@google.com>]

On Wed, May 12, 2021, Ben Gardon wrote:
> On Wed, Jan 15, 2020 at 4:32 PM Liran Alon <liran.alon@oracle.com> wrote:
> >
> >
> >
> > > On 16 Jan 2020, at 2:16, Jim Mattson <jmattson@google.com> wrote:
> > >
> > > Beginning with commit 0b9f6c4615c99 ("x86/kvm: Support the vCPU
> > > preemption check"), the KVM_VCPU_PREEMPTED flag is set in the guest
> > > copy of the kvm_steal_time struct on every call to vcpu_put(). As a
> > > result, guest memory is dirtied on every call to vcpu_put(), even when
> > > the VM is quiescent.
> > >
> > > To avoid dirtying guest memory unnecessarily, don't bother setting the
> > > flag in the guest copy of the struct if it is already set in the
> > > kernel copy of the struct.
> >
> > I suggest adding this comment to code as-well.
> 
> Ping. I don't know if a v2 of this change with the comment in code is
> needed for acceptance, but I don't want this to fall through the
> cracks and get lost.

A version of this was committed a while ago.  The CVE number makes me think it
went stealthily...

commit 8c6de56a42e0c657955e12b882a81ef07d1d073e
Author: Boris Ostrovsky <boris.ostrovsky@oracle.com>
Date:   Wed Oct 30 19:01:31 2019 +0000

    x86/kvm: Be careful not to clear KVM_VCPU_FLUSH_TLB bit

    kvm_steal_time_set_preempted() may accidentally clear KVM_VCPU_FLUSH_TLB
    bit if it is called more than once while VCPU is preempted.

    This is part of CVE-2019-3016.

    (This bug was also independently discovered by Jim Mattson
    <jmattson@google.com>)

    Signed-off-by: Boris Ostrovsky <boris.ostrovsky@oracle.com>
    Reviewed-by: Joao Martins <joao.m.martins@oracle.com>
    Cc: stable@vger.kernel.org
    Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
index cf917139de6b..8c9369151e9f 100644
--- a/arch/x86/kvm/x86.c
+++ b/arch/x86/kvm/x86.c
@@ -3504,6 +3504,9 @@ static void kvm_steal_time_set_preempted(struct kvm_vcpu *vcpu)
        if (!(vcpu->arch.st.msr_val & KVM_MSR_ENABLED))
                return;

+       if (vcpu->arch.st.steal.preempted)
+               return;
+
        vcpu->arch.st.steal.preempted = KVM_VCPU_PREEMPTED;

        kvm_write_guest_offset_cached(vcpu->kvm, &vcpu->arch.st.stime,


> > > If a different vCPU thread clears the guest copy of the flag, it will
> > > no longer get reset on the next call to vcpu_put, but it's not clear
> > > that resetting the flag in this case was intentional to begin with.
> >
> > I agree… I find it hard to believe that guest vCPU is allowed to clear the flag
> > and expect host to set it again on the next vcpu_put() call. Doesn’t really make sense.
> >
> > >
> > > Signed-off-by: Jim Mattson <jmattson@google.com>
> > > Tested-by: Kevin Mcgaire <kevinmcgaire@google.com>
> > > Reviewed-by: Ben Gardon <bgardon@google.com>
> > > Reviewed-by: Oliver Upton <oupton@google.com>
> >
> > Good catch.
> > Reviewed-by: Liran Alon <liran.alon@oracle.com>
> >
> > -Liran
> >
> > >
> > > ---
> > > arch/x86/kvm/x86.c | 3 +++
> > > 1 file changed, 3 insertions(+)
> > >
> > > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > > index cf917139de6b..3dc17b173f88 100644
> > > --- a/arch/x86/kvm/x86.c
> > > +++ b/arch/x86/kvm/x86.c
> > > @@ -3504,6 +3504,9 @@ static void kvm_steal_time_set_preempted(struct kvm_vcpu *vcpu)
> > >       if (!(vcpu->arch.st.msr_val & KVM_MSR_ENABLED))
> > >               return;
> > >
> > > +     if (vcpu->arch.st.steal.preempted & KVM_VCPU_PREEMPTED)
> > > +             return;
> > > +
> > >       vcpu->arch.st.steal.preempted = KVM_VCPU_PREEMPTED;
> > >
> > >       kvm_write_guest_offset_cached(vcpu->kvm, &vcpu->arch.st.stime,
> > > --
> > > 2.25.0.rc1.283.g88dfdc4193-goog
> > >
> >