KVM: x86/mmu: Protect noncoherent DMA zaps with SRCU
Subsystems (4)
PATCH 1/1 KVM: x86/mmu: Protect noncoherent DMA zaps with SRCU
Protect the noncoherent DMA zap with KVM's SRCU so that memslots and their architecture-specific metadata remain alive if the rmap walk drops mmu_lock to reschedule.
The VFIO noncoherent-DMA path invokes kvm_arch_register_noncoherent_dma() without holding slots_lock or an SRCU read lock. kvm_zap_gfn_range() can then enter __walk_slot_rmaps(), which retains pointers to a memslot and its rmap while cond_resched_rwlock_write() temporarily drops mmu_lock.
The race looks like this:
CPU 0: VFIO coherency update CPU 1: memslot delete
---------------------------- ---------------------
kvm_vfio_set_attr()
kvm_arch_register_noncoherent_dma()
kvm_zap_gfn_range()
__walk_slot_rmaps()
iterator.rmap = slot->arch.rmap
cond_resched_rwlock_write()
drop mmu_lock
KVM_SET_USER_MEMORY_REGION(DELETE)
kvm_arch_flush_shadow_memslot()
zap SPTEs under mmu_lock
kvm_swap_active_memslots()
synchronize_srcu_expedited()
kvm_free_memslot()
vfree(slot->arch.rmap[i])
kfree(slot)
reacquire mmu_lock
slot_rmap_walk_next()
read freed iterator.rmapThe delete path is allowed to free the old memslot because the zap path holds no SRCU read lock. synchronize_srcu_expedited() therefore does not wait for the rmap walk before kvm_free_memslot() releases the old slot and its rmap array. When the zap resumes, slot_rmap_walk_next() reads from freed memory.
KASAN reported:
BUG: KASAN: vmalloc-out-of-bounds in slot_rmap_walk_next+0x82/0x1c0 Read of size 8 at addr ffffc900005c1008 Call Trace: slot_rmap_walk_next+0x82/0x1c0 __kvm_rmap_zap_gfn_range+0x17a/0x280 kvm_zap_gfn_range+0x2a6/0x6a0 kvm_vfio_set_attr+0x576/0x770 kvm_device_ioctl+0x1ff/0x3b0 __x64_sys_ioctl+0x134/0x1c0
Hold SRCU across the zap so that memslot deletion waits for the walk to finish before freeing the old slot, without changing zap behavior.
--- arch/x86/kvm/x86.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)
arch/x86/kvm/x86.c +5 -1
@@ -14092,8 +14092,12 @@ static void kvm_noncoherent_dma_assignment_start_or_stop(struct kvm *kvm)* * If KVM always honors guest PAT, however, there is nothing to do. */ -if (kvm_check_has_quirk (kvm,KVM_X86_QUIRK_IGNORE_GUEST_PAT )) +if (kvm_check_has_quirk (kvm,KVM_X86_QUIRK_IGNORE_GUEST_PAT )) { +int idx =srcu_read_lock (&kvm->srcu); +kvm_zap_gfn_range (kvm,gpa_to_gfn (0 ),gpa_to_gfn (~0ULL )); +srcu_read_unlock (&kvm->srcu, idx); + } }void kvm_arch_register_noncoherent_dma (struct kvm *kvm)
Signature · 1 line
2.43.0
Quoted 9 lines · Chengfeng Ye
Protect the noncoherent DMA zap with KVM's SRCU so that memslots and their architecture-specific metadata remain alive if the rmap walk drops
mmu_lockto reschedule.The VFIO noncoherent-DMA path invokes
kvm_arch_register_noncoherent_dma()without holdingslots_lockor an SRCU read lock.kvm_zap_gfn_range()can then enter__walk_slot_rmaps(), which retains pointers to a memslot and its rmap whilecond_resched_rwlock_write()temporarily dropsmmu_lock.
kvm_zap_gfn_range() can invoke
slots = __kvm_memslots(kvm, i);
internally (from kvm_rmap_zap_gfn_range()). AFAICT this itself is enough that the caller should either grab read lock of KVM's SRCU or slots_lock.
For instance, __kvm_set_or_clear_apicv_inhibit() is already holding KVM's SRCU read lock when calling kvm_zap_gfn_range() to zap the GFN of guest's APIC page (which is only 1 page thus won't yield).
Quoted 6 lines
The race looks like this:
CPU 0: VFIO coherency update CPU 1: memslot delete
---------------------------- ---------------------
kvm_vfio_set_attr()
Quoted 12 lines
kvm_arch_register_noncoherent_dma()
kvm_zap_gfn_range()
__walk_slot_rmaps()
iterator.rmap = slot->arch.rmap
cond_resched_rwlock_write()
drop mmu_lock
KVM_SET_USER_MEMORY_REGION(DELETE)
kvm_arch_flush_shadow_memslot()
zap SPTEs under mmu_lock
kvm_swap_active_memslots()
synchronize_srcu_expedited()Isn't kvm_swap_active_memslots() called _before_kvm_arch_flush_shadow_memslot()?
Quoted 28 lines
kvm_free_memslot()
vfree(slot->arch.rmap[i])kfree(slot)reacquire
mmu_lock
slot_rmap_walk_next()
read freed iterator.rmapThe delete path is allowed to free the old memslot because the zap path holds no SRCU read lock.
synchronize_srcu_expedited()therefore does not wait for the rmap walk beforekvm_free_memslot()releases the old slot and its rmap array. When the zap resumes,slot_rmap_walk_next()reads from freed memory.KASAN reported:
BUG: KASAN: vmalloc-out-of-bounds in slot_rmap_walk_next+0x82/0x1c0 Read of size 8 at addr ffffc900005c1008 Call Trace: slot_rmap_walk_next+0x82/0x1c0 __kvm_rmap_zap_gfn_range+0x17a/0x280 kvm_zap_gfn_range+0x2a6/0x6a0 kvm_vfio_set_attr+0x576/0x770 kvm_device_ioctl+0x1ff/0x3b0 __x64_sys_ioctl+0x134/0x1c0
Quoted 6 lines
Hold SRCU across the zap so that memslot deletion waits for the walk to finish before freeing the old slot, without changing zap behavior.
Fixes 362ff6dca541 ("KVM: x86/mmu: Zap KVM TDP when noncoherent DMA assignment starts/stops")Signed-off-by Chengfeng Ye <[email protected]>
Code change LGTM, feel free to add:
Nit: the short-log uses "x86/mmu" as topic, but maybe "x86" is more appropriate since the code change is more like at x86 level?
Quoted 20 lines
--- arch/x86/kvm/x86.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)arch/x86/kvm/x86.c +5
-1@@ -14092,8 +14092,12 @@ static void kvm_noncoherent_dma_assignment_start_or_stop(struct kvm *kvm)* * If KVM always honors guest PAT, however, there is nothing to do. */ -if (kvm_check_has_quirk (kvm,KVM_X86_QUIRK_IGNORE_GUEST_PAT )) +if (kvm_check_has_quirk (kvm,KVM_X86_QUIRK_IGNORE_GUEST_PAT )) { +int idx =srcu_read_lock (&kvm->srcu); +kvm_zap_gfn_range (kvm,gpa_to_gfn (0 ),gpa_to_gfn (~0ULL )); +srcu_read_unlock (&kvm->srcu, idx); + } }
Sean Christopherson
maintainerQuoted 12 lines · Chengfeng Ye
Fixes 362ff6dca541 ("KVM: x86/mmu: Zap KVM TDP when noncoherent DMA assignment starts/stops")Signed-off-by Chengfeng Ye <[email protected]>--- arch/x86/kvm/x86.c | 6 +++++- 1 file changed, 5 insertions(+), 1 deletion(-)arch/x86/kvm/x86.c +0
-0
arch/x86/kvm/x86.c static void kvm_noncoherent_dma_assignment_start_or_stop(struct kvm *kvm) · line 14096
Quoted 6 lines
arch/x86/kvm/x86.c +2
-1@@ -14092,8 +14092,12 @@ static void kvm_noncoherent_dma_assignment_start_or_stop(struct kvm *kvm)* * If KVM always honors guest PAT, however, there is nothing to do. */ -if (kvm_check_has_quirk (kvm,KVM_X86_QUIRK_IGNORE_GUEST_PAT )) +if (kvm_check_has_quirk (kvm,KVM_X86_QUIRK_IGNORE_GUEST_PAT )) { +int idx =srcu_read_lock (&kvm->srcu);
Please do:
guard(srcu)(&kvm->srcu);
even though this is tagged for stable.
Alternatively, what if we have kvm_zap_gfn_range() acquire SRCU? Nesting "locks" is a-ok, and two of the three users of kvm_zap_gfn_range() have had this bug (see commit 074c00800719 ("KVM: x86: Use SRCU to protect zap in __kvm_set_or_clear_apicv_inhibit()").
arch/x86/kvm/mmu/mmu.c +2 -0
@@ -7072,6 +7072,8 @@ void kvm_zap_gfn_range(struct kvm *kvm, gfn_t gfn_start, gfn_t gfn_end)if (WARN_ON_ONCE (gfn_end <= gfn_start))return ; +guard (srcu)(&kvm->srcu); +write_lock (&kvm->mmu_lock);kvm_mmu_invalidate_start (kvm);
arch/x86/kvm/x86.c +0 -2
@@ -7875,10 +7875,8 @@ void __kvm_set_or_clear_apicv_inhibit(struct kvm *kvm, kvm->arch.apicv_inhibit_reasons = new;if (new) {unsigned long gfn =gpa_to_gfn (APIC_DEFAULT_PHYS_BASE ); -int idx =srcu_read_lock (&kvm->srcu);kvm_zap_gfn_range (kvm, gfn, gfn+1 ); -srcu_read_unlock (&kvm->srcu, idx); } }else { kvm->arch.apicv_inhibit_reasons = new;
Quoted 9 lines
arch/x86/kvm/x86.c +3
-0+kvm_zap_gfn_range (kvm,gpa_to_gfn (0 ),gpa_to_gfn (~0ULL )); +srcu_read_unlock (&kvm->srcu, idx); + } }void kvm_arch_register_noncoherent_dma (struct kvm *kvm)Signature · 1 line
2.43.0
Quoted 4 lines · Sean Christopherson
Alternatively, what if we have
kvm_zap_gfn_range()acquire SRCU? Nesting "locks" is a-ok, and two of the three users ofkvm_zap_gfn_range()have had this bug (see commit074c00800719("KVM: x86: Use SRCU to protect zap in__kvm_set_or_clear_apicv_inhibit()").
kvm_vcpu_srcu_read_lock() doesn't allow nesting lock of KVM's SRCU. Doesn't seem to be a problem for this particular case, though, because sev_handle_rmp_fault() calls kvm_zap_gfn_range() after kvm_vcpu_srcu_read_lock() is called.
Sean Christopherson
maintainerQuoted 7 lines · Kai Huang
Quoted 4 lines · Sean Christopherson
Alternatively, what if we have
kvm_zap_gfn_range()acquire SRCU? Nesting "locks" is a-ok, and two of the three users ofkvm_zap_gfn_range()have had this bug (see commit074c00800719("KVM: x86: Use SRCU to protect zap in__kvm_set_or_clear_apicv_inhibit()").
kvm_vcpu_srcu_read_lock()doesn't allow nesting lock of KVM's SRCU.
No, kvm_vcpu_srcu_read_lock() doesn't allow nested usage of itself. Commit 2031f2876896 ("KVM: Add helpers to wrap vcpu->srcu_idx and yell if it's abused") added the protection because we had multiple bugs where KVM would clobber vcpu->srcu_idx and cause a deadlock by leaking a grace period reference.
Quoted 3 lines
Doesn't seem to be a problem for this particular case, though, because
sev_handle_rmp_fault()callskvm_zap_gfn_range()afterkvm_vcpu_srcu_read_lock()is called.
kvm_vcpu_srcu_read_lock() can nested inside an existing kvm->srcu critical section, just so long as the existing critical section isn't tracking its grace period via vcpu->____srcu_idx.
Quoted 14 lines · Sean Christopherson
Quoted 7 lines · Kai Huang
Quoted 4 lines · Sean Christopherson
Alternatively, what if we have
kvm_zap_gfn_range()acquire SRCU? Nesting "locks" is a-ok, and two of the three users ofkvm_zap_gfn_range()have had this bug (see commit074c00800719("KVM: x86: Use SRCU to protect zap in__kvm_set_or_clear_apicv_inhibit()").
kvm_vcpu_srcu_read_lock()doesn't allow nesting lock of KVM's SRCU.No,
kvm_vcpu_srcu_read_lock()doesn't allow nested usage of itself. Commit2031f2876896("KVM: Add helpers to wrapvcpu->srcu_idxand yell if it's abused") added the protection because we had multiple bugs where KVM would clobbervcpu->srcu_idxand cause a deadlock by leaking a grace period reference.
Quoted 7 lines
Quoted 3 lines
Doesn't seem to be a problem for this particular case, though, because
sev_handle_rmp_fault()callskvm_zap_gfn_range()afterkvm_vcpu_srcu_read_lock()is called.
kvm_vcpu_srcu_read_lock()can nested inside an existingkvm->srcucritical section, just so long as the existing critical section isn't tracking its grace period viavcpu->____srcu_idx.
Hmm right, doesn't seem calling kvm_vcpu_srcu_read_lock() inside existing kvm-
Quoted 1 line
srcu could clobber
vcpu->____srcu_idx. So agree it's safe to explicitly hold
SRCU read lock inside kvm_zap_gfn_range().
Thanks for your review! I have just sent a v2 to fix as the way
suggested: https://lore.kernel.org/kvm/[email protected]/T/#t
Best regards,
Chengfeng
PATCH v2 1/1 KVM: x86: Take SRCU in kvm_zap_gfn_range()
In reply to Chengfeng Yekvm_zap_gfn_range() walks memslots and rmaps and may drop mmu_lock to reschedule. Callers that do not already hold kvm->srcu (or slots_lock) can race with memslot deletion: synchronize_srcu_expedited() does not wait, kvm_free_memslot() frees the old slot and its rmap, and the zap resumes on freed memory.
The VFIO noncoherent-DMA path hits this by zapping the entire GPA space without SRCU. KASAN reported:
BUG: KASAN: vmalloc-out-of-bounds in slot_rmap_walk_next+0x82/0x1c0 Read of size 8 at addr ffffc900005c1008 Call Trace: slot_rmap_walk_next+0x82/0x1c0 __kvm_rmap_zap_gfn_range+0x17a/0x280 kvm_zap_gfn_range+0x2a6/0x6a0 kvm_vfio_set_attr+0x576/0x770 kvm_device_ioctl+0x1ff/0x3b0
Take SRCU inside kvm_zap_gfn_range() so every caller is covered. Nesting with an existing kvm->srcu critical section is fine; the helper uses a local index. Drop the now-redundant SRCU pair from __kvm_set_or_clear_apicv_inhibit().
---
v2:
- Take SRCU inside kvm_zap_gfn_range() with guard(srcu)(), as suggested by Sean Christopherson, instead of wrapping only the noncoherent-DMA caller.
- Drop the now-redundant SRCU pair from __kvm_set_or_clear_apicv_inhibit(). Nested kvm->srcu is fine; only kvm_vcpu_srcu_read_lock() cannot nest with itself.
arch/x86/kvm/mmu/mmu.c | 2 ++ arch/x86/kvm/x86.c | 2 -- 2 files changed, 2 insertions(+), 2 deletions(-)
arch/x86/kvm/mmu/mmu.c +2 -0
@@ -7047,6 +7047,8 @@ void kvm_zap_gfn_range(struct kvm *kvm, gfn_t gfn_start, gfn_t gfn_end)if (WARN_ON_ONCE (gfn_end <= gfn_start))return ; +guard (srcu)(&kvm->srcu); +write_lock (&kvm->mmu_lock);kvm_mmu_invalidate_start (kvm);
arch/x86/kvm/x86.c +0 -2
@@ -10986,10 +10986,8 @@ void __kvm_set_or_clear_apicv_inhibit(struct kvm *kvm, kvm->arch.apicv_inhibit_reasons = new;if (new) {unsigned long gfn =gpa_to_gfn (APIC_DEFAULT_PHYS_BASE ); -int idx =srcu_read_lock (&kvm->srcu);kvm_zap_gfn_range (kvm, gfn, gfn+1 ); -srcu_read_unlock (&kvm->srcu, idx); } }else { kvm->arch.apicv_inhibit_reasons = new;
Signature · 1 line
2.43.0
Re: [PATCH v2] KVM: x86: Take SRCU in kvm_zap_gfn_range()
Quoted 18 lines · Chengfeng Ye
kvm_zap_gfn_range()walks memslots and rmaps and may dropmmu_lockto reschedule. Callers that do not already holdkvm->srcu(orslots_lock) can race with memslot deletion:synchronize_srcu_expedited()does not wait,kvm_free_memslot()frees the old slot and its rmap, and the zap resumes on freed memory.The VFIO noncoherent-DMA path hits this by zapping the entire GPA space without SRCU. KASAN reported:
BUG: KASAN: vmalloc-out-of-bounds in slot_rmap_walk_next+0x82/0x1c0 Read of size 8 at addr ffffc900005c1008 Call Trace: slot_rmap_walk_next+0x82/0x1c0 __kvm_rmap_zap_gfn_range+0x17a/0x280 kvm_zap_gfn_range+0x2a6/0x6a0 kvm_vfio_set_attr+0x576/0x770 kvm_device_ioctl+0x1ff/0x3b0
Quoted 8 lines
Take SRCU inside
kvm_zap_gfn_range()so every caller is covered. Nesting with an existingkvm->srcucritical section is fine; the helper uses a local index. Drop the now-redundant SRCU pair from__kvm_set_or_clear_apicv_inhibit().Fixes 362ff6dca541 ("KVM: x86/mmu: Zap KVM TDP when noncoherent DMA assignment starts/stops")Signed-off-by Chengfeng Ye <[email protected]>
Btw, I found kvm_noncoherent_dma_assignment_start_or_stop() was added at July/2023 and the SRCU lock to __kvm_set_or_clear_apicv_inhibit() was added at Nov/2022, so I think this patch should be able to be applied to stable kernels cleanly (if no other conflicts).
Re: [PATCH v2] KVM: x86: Take SRCU in kvm_zap_gfn_range()
In reply to Chengfeng YeQuoted 10 lines · Chengfeng Ye
kvm_zap_gfn_range()walks memslots and rmaps and may dropmmu_lockto reschedule. Callers that do not already holdkvm->srcu(orslots_lock) can race with memslot deletion:synchronize_srcu_expedited()does not wait,kvm_free_memslot()frees the old slot and its rmap, and the zap resumes on freed memory.The VFIO noncoherent-DMA path hits this by zapping the entire GPA space without SRCU. KASAN reported:
[...]
Applied to kvm-x86 mmu, thanks!
[1/1] KVM: x86: Take SRCU in kvm_zap_gfn_range()
https://github.com/kvm-x86/linux/commit/b59a87c1ba3c