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

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

[Jim Mattson]

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.

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.

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>

---
 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

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

[Liran Alon]

> 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.

> 
> 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
> 

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

[Ben Gardon]

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.

>
> >
> > 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
> >
>

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

[Sean Christopherson]

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
> > >
> >

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

[Ben Gardon]

On Wed, May 12, 2021 at 4:08 PM Sean Christopherson <seanjc@google.com> wrote:
>
> 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...

That's great to know. Thanks for digging that up Sean.

>
> 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
> > > >
> > >

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

[Paolo Bonzini]

On 13/05/21 18:14, Ben Gardon wrote:
>> 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.
> 

I usually try to send CVE/embargoed patches after the fact to the list, 
using with an [FYI PATCH] prefix.

Paolo

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

[Paolo Bonzini]

On 13/05/21 18:14, Ben Gardon wrote:
>> 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.
> 

I usually try to send CVE/embargoed patches after the fact to the list, 
using an [FYI PATCH] prefix.

Paolo