On Wed, May 12, 2021, Ben Gardon wrote:
> On Wed, Jan 15, 2020 at 4:32 PM Liran Alon <liran.alon@xxxxxxxxxx> wrote:
> >
> >
> >
> > > On 16 Jan 2020, at 2:16, Jim Mattson <jmattson@xxxxxxxxxx> 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@xxxxxxxxxx>
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@xxxxxxxxxx>)
Signed-off-by: Boris Ostrovsky <boris.ostrovsky@xxxxxxxxxx>
Reviewed-by: Joao Martins <joao.m.martins@xxxxxxxxxx>
Cc: stable@xxxxxxxxxxxxxxx
Signed-off-by: Paolo Bonzini <pbonzini@xxxxxxxxxx>
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@xxxxxxxxxx>
> > > Tested-by: Kevin Mcgaire <kevinmcgaire@xxxxxxxxxx>
> > > Reviewed-by: Ben Gardon <bgardon@xxxxxxxxxx>
> > > Reviewed-by: Oliver Upton <oupton@xxxxxxxxxx>
> >
> > Good catch.
> > Reviewed-by: Liran Alon <liran.alon@xxxxxxxxxx>
> >
> > -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
> > >
> >
