On Wed, May 12, 2021 at 4:08 PM Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
>
> 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...
That's great to know. Thanks for digging that up Sean.
>
> 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
> > > >
> > >
