| Message ID | FEBA39FEBDA1C9D7+20241017061334.222103-1-wangyuli@uniontech.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | KVM: arm64: vgic-its: Do not call vgic_put_irq() within vgic_its_inject_cached_translation() | expand |
Hi, On Thu, Oct 17, 2024 at 02:13:34PM +0800, WangYuli wrote: > There is a probability that the host machine will also restart > when the virtual machine is restarting. This is a start, but can you please describe in detail what the flow is you're seeing that leads to the refcount bug / UAF? > Commit ad362fe07fec ("KVM: arm64: vgic-its: Avoid potential UAF > in LPI translation cache") released the reference count of an IRQ > when it shouldn't have. This led to a situation where, when the > system finally released the IRQ, it found that the structure had > already been freed, triggering a > 'refcount_t: underflow; use-after-free' error. > > In fact, the function "vgic_put_irq" should be called by > "vgic_its_inject_cached_translation" instead of > "vgic_its_trigger_msi". This line doesn't match your patch, and instead aligns with the upstream behavior. The put in vgic_its_inject_cached_translation() pairs with the get in vgic_its_check_cache(). We need to do this because the LPI injection fast path happens outside of the its_lock. The slow path for LPI injection takes the its_lock and is safe because the ITE holds a reference on the descriptor as well. Because of that, vgic_its_trigger_msi() doesn't touch the refcount on the resolved IRQ. So I'm not following how any of this leads to the underflow you observe. Even if the put in vgic_its_inject_cached_translation() causes the refcount to drop to 0, it is likely that an unbalanced put happened somewhere else in the ITS emulation.
diff --git a/arch/arm64/kvm/vgic/vgic-its.c b/arch/arm64/kvm/vgic/vgic-its.c index ba945ba78cc7..fb5f57cbab42 100644 --- a/arch/arm64/kvm/vgic/vgic-its.c +++ b/arch/arm64/kvm/vgic/vgic-its.c @@ -679,6 +679,7 @@ static int vgic_its_trigger_msi(struct kvm *kvm, struct vgic_its *its, raw_spin_lock_irqsave(&irq->irq_lock, flags); irq->pending_latch = true; vgic_queue_irq_unlock(kvm, irq, flags); + vgic_put_irq(kvm, irq); return 0; } @@ -697,7 +698,6 @@ int vgic_its_inject_cached_translation(struct kvm *kvm, struct kvm_msi *msi) raw_spin_lock_irqsave(&irq->irq_lock, flags); irq->pending_latch = true; vgic_queue_irq_unlock(kvm, irq, flags); - vgic_put_irq(kvm, irq); return 0; }
There is a probability that the host machine will also restart when the virtual machine is restarting. Commit ad362fe07fec ("KVM: arm64: vgic-its: Avoid potential UAF in LPI translation cache") released the reference count of an IRQ when it shouldn't have. This led to a situation where, when the system finally released the IRQ, it found that the structure had already been freed, triggering a 'refcount_t: underflow; use-after-free' error. In fact, the function "vgic_put_irq" should be called by "vgic_its_inject_cached_translation" instead of "vgic_its_trigger_msi". Call trace: its_free_ite+0x90/0xa0 vgic_its_free_device+0x3c/0xa0 vgic_its_destroy+0x4c/0xb8 kvm_put_kvm+0x214/0x358 kvm_vcpu_release+0x24/0x38 __fput+0x84/0x278 ____fput+0x20/0x30 task_work_run+0xcc/0x190 do_exit+0x36c/0xa88 do_group_exit+0x4c/0xb8 __arm64_sys_exit_group+0x24/0x28 invoke_syscall+0x54/0x120 el0_svc_common.constprop.4+0x16c/0x1f0 do_el0_svc+0x34/0xb0 el0_svc+0x1c/0x28 el0_sync_handler+0x8c/0xb0 el0_sync+0x148/0x180 Fixes: ad362fe07fec ("KVM: arm64: vgic-its: Avoid potential UAF in LPI translation cache") Cc: stable@vger.kernel.org Signed-off-by: Wenyao Hai <haiwenyao@uniontech.com> Signed-off-by: WangYuli <wangyuli@uniontech.com> --- arch/arm64/kvm/vgic/vgic-its.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)