Paolo Bonzini pbonzini@redhat.com writes:

Commit 7e2175ebd695 ("KVM: x86: Fix recording of guest steal time / preempted status", 2021-11-11) open coded the previous call to kvm_map_gfn, but in doing so it dropped the comparison between the cached guest physical address and the one in the MSR. This cause an incorrect cache hit if the guest modifies the steal time address while the memslots remain the same. This can happen with kexec, in which case the steal time data is written at the address used by the old kernel instead of the old one.

While at it, rename the variable from gfn to gpa since it is a plain physical address and not a right-shifted one.

Reported-by: Dave Young ruyang@redhat.com Reported-by: Xiaoying Yan yiyan@redhat.com Analyzed-by: Dr. David Alan Gilbert dgilbert@redhat.com Cc: David Woodhouse dwmw@amazon.co.uk Cc: stable@vger.kernel.org Fixes: 7e2175ebd695 ("KVM: x86: Fix recording of guest steal time / preempted status") Signed-off-by: Paolo Bonzini pbonzini@redhat.com

---

arch/x86/kvm/x86.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c index e5fa335a4ea7..36dcf18b04bf 100644 --- a/arch/x86/kvm/x86.c +++ b/arch/x86/kvm/x86.c @@ -3380,6 +3380,7 @@ static void record_steal_time(struct kvm_vcpu *vcpu) struct gfn_to_hva_cache *ghc = &vcpu->arch.st.cache; struct kvm_steal_time __user *st; struct kvm_memslots *slots;

• gpa_t gpa = vcpu->arch.st.msr_val & KVM_STEAL_VALID_BITS; u64 steal; u32 version;

@@ -3397,13 +3398,12 @@ static void record_steal_time(struct kvm_vcpu *vcpu) slots = kvm_memslots(vcpu->kvm); if (unlikely(slots->generation != ghc->generation ||

•      gpa != ghc->gpa ||
  kvm_is_error_hva(ghc->hva) || !ghc->memslot)) {
  

• gfn_t gfn = vcpu->arch.st.msr_val & KVM_STEAL_VALID_BITS;
  

• /* We rely on the fact that it fits in a single page. */ BUILD_BUG_ON((sizeof(*st) - 1) & KVM_STEAL_VALID_BITS);

• if (kvm_gfn_to_hva_cache_init(vcpu->kvm, ghc, gfn, sizeof(*st)) ||
  

• if (kvm_gfn_to_hva_cache_init(vcpu->kvm, ghc, gpa, sizeof(*st)) ||
  

(It would be nice to somehow get at least a warning when 'gfn_t' is used instead of 'gpa_t' and vice versa)

    kvm_is_error_hva(ghc->hva) || !ghc->memslot)
	return;

}

Reviewed-by: Vitaly Kuznetsov vkuznets@redhat.com

On Thu, 2022-08-04 at 15:28 +0200, Paolo Bonzini wrote:

@@ -3397,13 +3398,12 @@ static void record_steal_time(struct kvm_vcpu *vcpu)= slots = kvm_memslots(vcpu->kvm);

    if (unlikely(slots->generation != ghc->generation ||

•                gpa != ghc->gpa ||
                 kvm_is_error_hva(ghc->hva) || !ghc->memslot)) {
  

If we have to fix the same bug in two open-coded versions of the same thing then surely it's time for something like...

static inline bool kvm_gfn_to_hva_cache_valid(struct kvm *kvm, struct gfn_to_hva_cache *ghc, gpa_t gpa) { struct kvm_memslots *slots = kvm_memslots(kvm);

return !unlikely(slots->generation != ghc->generation || gpa != ghc->gpa || kvm_is_error_hva(ghc->hva) || !ghc->memslot); }