From: David Woodhouse The gfn_to_pfn_cache refresh path guards against mmu notifier invalidations which complete while it has dropped gpc->lock for the HVA->PFN lookup: hva_to_pfn_retry() samples kvm->mmu_invalidate_seq and retries if it changed, or if mn_active_invalidate_count is still elevated. That is insufficient for HVA-based caches. mmu_invalidate_seq is only advanced by kvm_mmu_invalidate_end() when the invalidated range overlaps a memslot, and an HVA-based cache (e.g. the Xen shared_info page mapped with KVM_XEN_ATTR_TYPE_SHARED_INFO_HVA) need not be backed by any memslot at all. An invalidation of the cached HVA which starts and ends entirely within the lookup window is thus invisible to the retry check: mn_active_invalidate_count is back to zero and the sequence never moved. The refresh then publishes a mapping of a page which has already been freed, and the next reader dereferences it: BUG: KASAN: use-after-free in kvm_xen_shared_info_init+0x3c6/0x440 Read of size 4 at addr ffff8880599c2900 by task syz.2.383/7257 Since gfn_to_pfn_cache_invalidate_start() deliberately skips caches which are not currently valid (including one whose refresh is in progress, as the refresh clears the valid flag before dropping the lock), the retry check is the only line of defence, and it must fire for *any* invalidation, not just those hitting a memslot. Add a dedicated kvm->gpc_invalidate_seq, incremented by every kvm_mmu_notifier_invalidate_range_end() under mn_invalidate_lock before mn_active_invalidate_count is decremented, and check it in hva_to_pfn_retry() instead of mmu_invalidate_seq. Incrementing in range_end() in the same critical section as the in-progress count also closes the variant where the cache is activated with the contested HVA only after invalidate_range_start() has run. The same bug is also reachable through the per-vCPU vcpu_info cache (KVM_XEN_VCPU_ATTR_TYPE_VCPU_INFO_HVA), where the stale mapping is then dereferenced by kvm_setup_guest_pvclock() on the next KVM_RUN: BUG: KASAN: use-after-free in kvm_setup_guest_pvclock+0x5bf/0x660 This intentionally makes refresh retry on *unrelated* mmu notifier events; restoring precision (and reworking the GPC locking more generally) is left for a subsequent series. Reproducers: https://david.woodhou.se/xen_shinfo_race.c https://david.woodhou.se/vcpu_info_race.c Suggested-by: Sean Christopherson Reported-by: syzbot+0948c82180d475ad24e2@syzkaller.appspotmail.com Closes: https://lore.kernel.org/all/6a0c5f2c.a00a0220.2c7954.0000.GAE@google.com/ Reported-by: syzbot+fb7c2dd166d3ea63df2a@syzkaller.appspotmail.com Closes: https://lore.kernel.org/all/6a426dd2.854d4ab9.360e1d.0008.GAE@google.com/ Fixes: b9220d32799a ("KVM: x86/xen: allow shared_info to be mapped by fixed HVA") Cc: stable@vger.kernel.org Signed-off-by: David Woodhouse --- It turns out that I'm not massively fond of *either* my original (needs_invalidation) approach *or* Sean's range-based thing from https://lore.kernel.org/all/Zw8DINUkJJKDByXE@google.com/ I'm playing with reworking the GPC locking to use RCU instead. It's arguably a much better approach than rwlocks anyway. But this fix stands alone and we can worry about *optimising* things later. include/linux/kvm_host.h | 2 ++ virt/kvm/kvm_main.c | 10 ++++++++++ virt/kvm/pfncache.c | 18 +++++++++--------- 3 files changed, 21 insertions(+), 9 deletions(-) diff --git a/include/linux/kvm_host.h b/include/linux/kvm_host.h index ab8cfaec82d3..d7406c10e090 100644 --- a/include/linux/kvm_host.h +++ b/include/linux/kvm_host.h @@ -854,6 +854,8 @@ struct kvm { gfn_t mmu_invalidate_range_start; gfn_t mmu_invalidate_range_end; + unsigned long gpc_invalidate_seq; + struct list_head devices; u64 manual_dirty_log_protect; struct dentry *debugfs_dentry; diff --git a/virt/kvm/kvm_main.c b/virt/kvm/kvm_main.c index 45e784462ec6..d247f741d1e3 100644 --- a/virt/kvm/kvm_main.c +++ b/virt/kvm/kvm_main.c @@ -812,6 +812,16 @@ static void kvm_mmu_notifier_invalidate_range_end(struct mmu_notifier *mn, /* Pairs with the increment in range_start(). */ spin_lock(&kvm->mn_invalidate_lock); + kvm->gpc_invalidate_seq++; + + /* + * As with the MMU sequence counter and mmu_invalidate_in_progress, the + * GPC sequence increase must be visible before the invalidate count + * goes to zero. Pairs with the smp_rmb() in + * mmu_notifier_retry_cache(). + */ + smp_wmb(); + if (!WARN_ON_ONCE(!kvm->mn_active_invalidate_count)) --kvm->mn_active_invalidate_count; wake = !kvm->mn_active_invalidate_count; diff --git a/virt/kvm/pfncache.c b/virt/kvm/pfncache.c index 728d2c1b488a..3659686b97c2 100644 --- a/virt/kvm/pfncache.c +++ b/virt/kvm/pfncache.c @@ -124,7 +124,7 @@ static void gpc_unmap(kvm_pfn_t pfn, void *khva) #endif } -static inline bool mmu_notifier_retry_cache(struct kvm *kvm, unsigned long mmu_seq) +static inline bool mmu_notifier_retry_cache(struct kvm *kvm, unsigned long gpc_seq) { /* * mn_active_invalidate_count acts for all intents and purposes @@ -136,20 +136,20 @@ static inline bool mmu_notifier_retry_cache(struct kvm *kvm, unsigned long mmu_s * Note, it does not matter that mn_active_invalidate_count * is not protected by gpc->lock. It is guaranteed to * be elevated before the mmu_notifier acquires gpc->lock, and - * isn't dropped until after mmu_invalidate_seq is updated. + * isn't dropped until after gpc_invalidate_seq is updated. */ if (kvm->mn_active_invalidate_count) return true; /* * Ensure mn_active_invalidate_count is read before - * mmu_invalidate_seq. This pairs with the smp_wmb() in - * mmu_notifier_invalidate_range_end() to guarantee either the + * gpc_invalidate_seq. This pairs with the smp_wmb() in + * kvm_mmu_notifier_invalidate_range_end() to guarantee either the * old (non-zero) value of mn_active_invalidate_count or the - * new (incremented) value of mmu_invalidate_seq is observed. + * new (incremented) value of gpc_invalidate_seq is observed. */ smp_rmb(); - return kvm->mmu_invalidate_seq != mmu_seq; + return kvm->gpc_invalidate_seq != gpc_seq; } static kvm_pfn_t hva_to_pfn_retry(struct gfn_to_pfn_cache *gpc) @@ -158,7 +158,7 @@ static kvm_pfn_t hva_to_pfn_retry(struct gfn_to_pfn_cache *gpc) void *old_khva = (void *)PAGE_ALIGN_DOWN((uintptr_t)gpc->khva); kvm_pfn_t new_pfn = KVM_PFN_ERR_FAULT; void *new_khva = NULL; - unsigned long mmu_seq; + unsigned long gpc_seq; struct page *page; struct kvm_follow_pfn kfp = { @@ -181,7 +181,7 @@ static kvm_pfn_t hva_to_pfn_retry(struct gfn_to_pfn_cache *gpc) gpc->valid = false; do { - mmu_seq = gpc->kvm->mmu_invalidate_seq; + gpc_seq = gpc->kvm->gpc_invalidate_seq; smp_rmb(); write_unlock_irq(&gpc->lock); @@ -232,7 +232,7 @@ static kvm_pfn_t hva_to_pfn_retry(struct gfn_to_pfn_cache *gpc) * attempting to refresh. */ WARN_ON_ONCE(gpc->valid); - } while (mmu_notifier_retry_cache(gpc->kvm, mmu_seq)); + } while (mmu_notifier_retry_cache(gpc->kvm, gpc_seq)); gpc->valid = true; gpc->pfn = new_pfn; base-commit: 2d2338c93da79b3bfe4b6099a931d9468d539952 -- 2.43.0