Fix a potential race between decrementing an LPI's reference count and evicting that structure from the LPI xarray. LPI structures are maintained in the VGIC LPI xarray (dist->lpi_xa). When the reference count of an LPI structure drops to zero, vgic_release_lpi_locked() removes the structure from the xarray and frees it under the xarray lock. However, the release of an LPI can race with a concurrent LPI re-registration with the same INTID via vgic_add_lpi() on another CPU, since the reference count drop and the xarray eviction are not performed in a single atomic step. This can happen e.g. if the guest issues a DISCARD while the LPI is still referenced from a vCPU's active-pending list (ap_list), and the same INTID is re-mapped via MAPTI. Particularly, vgic_release_lpi_locked() is called from two distinct paths: direct release via vgic_put_irq(), and deferred release via vgic_release_deleted_lpis(). During direct release, the issue can result in deleting a newly registered LPI from the xarray: CPU0 (Releasing LPI) CPU1 (Adding new LPI) ==================== ===================== vgic_put_irq() __vgic_put_irq() refcount_dec_and_test() vgic_add_lpi() xa_lock_irqsave() old_irq = xa_load(.., intid) vgic_try_get_irq_ref(old_irq) == false new IRQ inserted --> __xa_store(.., intid, ..) xa_unlock_irqrestore() xa_lock_irqsave(); vgic_release_lpi_locked() __xa_erase(.., irq->intid) <-- BUG: new IRQ is erased kfree_rcu(old_irq) During the deferred release path, the old IRQ can be leaked: CPU0 (Releasing LPI) CPU1 (Adding new LPI) ==================== ===================== vgic_put_irq_norelease() __vgic_put_irq() refcount_dec_and_test() irq->pending_release = true vgic_add_lpi() xa_lock_irqsave() old_irq = xa_load(.., intid) vgic_try_get_irq_ref(oldirq) == false BUG: old IRQ overwritten --> __xa_store(.., intid, ..) xa_unlock_irqrestore() vgic_release_deleted_lpis() xa_lock_irqsave() xa_for_each() { .. } <-- old IRQ with pending_release = true is gone, so it cannot be released To fix the direct release path, move the reference count drop inside the xarray lock, making sure that vgic_add_lpi() never encounters the to-be-released LPI. In the deferred release path, the refcount drop must happen under a raw spinlock, so the xarray lock cannot be grabbed, and the same solution does not work. Instead, update vgic_add_lpi(), so that if it evicts an LPI from the xarray, it takes on the responsibility of freeing it. Consequently, an LPI may now be freed concurrently after a deferred release drops the refcount, so accessing the pending_release field is no longer safe from use-after-free. Delete all uses of the flag, and update vgic_release_deleted_lpis() to identify orphaned LPIs purely based on their refcount.
Unified · Split
| @@ -116,18 +116,26 @@ static struct vgic_irq *vgic_add_lpi(struct kvm *kvm, u32 intid, · 115 unmodified lines | |||
| 116 | kfree(irq); | 116 | kfree(irq); |
| 117 | irq = oldirq; | 117 | irq = oldirq; |
| 118 | } else { | 118 | } else { |
| 119 | ret = xa_err(__xa_store(&dist->lpi_xa, intid, irq, 0)); | 119 | /* |
| 120 | } | 120 | * The entry is either empty or contains a dead LPI (refcount=0) |
| 121 | 121 | * from the deferred release path, pending cleanup by | |
| 122 | xa_unlock_irqrestore(&dist->lpi_xa, flags); | 122 | * vgic_release_deleted_lpis(). Evict and free it if present. |
| 123 | */ | ||
| 124 | oldirq = __xa_store(&dist->lpi_xa, intid, irq, 0); | ||
| 125 | ret = xa_err(oldirq); | ||
| 126 | if (ret) { | ||
| 127 | xa_unlock_irqrestore(&dist->lpi_xa, flags); | ||
| 128 | kfree(irq); | ||
| 123 | 129 | ||
| 124 | if (ret) { | 130 | return ERR_PTR(ret); |
| 125 | xa_release(&dist->lpi_xa, intid); | 131 | } |
| 126 | kfree(irq); | ||
| 127 | 132 | ||
| 128 | return ERR_PTR(ret); | 133 | if (oldirq && !WARN_ON_ONCE(refcount_read(&oldirq->refcount))) |
| 134 | kfree_rcu(oldirq, rcu); | ||
| 129 | } | 135 | } |
| 130 | 136 | ||
| 137 | xa_unlock_irqrestore(&dist->lpi_xa, flags); | ||
| 138 | |||
| 131 | /* | 139 | /* |
| 132 | * We "cache" the configuration table entries in our struct vgic_irq's. | 140 | * We "cache" the configuration table entries in our struct vgic_irq's. |
| 133 | * However we only have those structs for mapped IRQs, so we read in | 141 | * However we only have those structs for mapped IRQs, so we read in |
| @@ -146,11 +146,7 @@ static __must_check bool __vgic_put_irq(struct kvm *kvm, struct vgic_irq *irq) · 145 unmodified lines | |||
| 146 | 146 | ||
| 147 | static __must_check bool vgic_put_irq_norelease(struct kvm *kvm, struct vgic_irq *irq) | 147 | static __must_check bool vgic_put_irq_norelease(struct kvm *kvm, struct vgic_irq *irq) |
| 148 | { | 148 | { |
| 149 | if (!__vgic_put_irq(kvm, irq)) | 149 | return __vgic_put_irq(kvm, irq); |
| 150 | return false; | ||
| 151 | |||
| 152 | irq->pending_release = true; | ||
| 153 | return true; | ||
| 154 | } | 150 | } |
| 155 | 151 | ||
| 156 | void vgic_put_irq(struct kvm *kvm, struct vgic_irq *irq) | 152 | void vgic_put_irq(struct kvm *kvm, struct vgic_irq *irq) |
| @@ -167,12 +163,14 @@ void vgic_put_irq(struct kvm *kvm, struct vgic_irq *irq) · 10 unmodified lines | |||
| 167 | guard(spinlock_irqsave)(&dist->lpi_xa.xa_lock); | 163 | guard(spinlock_irqsave)(&dist->lpi_xa.xa_lock); |
| 168 | } | 164 | } |
| 169 | 165 | ||
| 170 | if (!__vgic_put_irq(kvm, irq)) | 166 | if (!irq_is_lpi(kvm, irq->intid)) |
| 171 | return; | 167 | return; |
| 172 | 168 | ||
| 173 | xa_lock_irqsave(&dist->lpi_xa, flags); | 169 | if (refcount_dec_and_lock_irqsave(&irq->refcount, |
| 174 | vgic_release_lpi_locked(dist, irq); | 170 | &dist->lpi_xa.xa_lock, &flags)) { |
| 175 | xa_unlock_irqrestore(&dist->lpi_xa, flags); | 171 | vgic_release_lpi_locked(dist, irq); |
| 172 | xa_unlock_irqrestore(&dist->lpi_xa, flags); | ||
| 173 | } | ||
| 176 | } | 174 | } |
| 177 | 175 | ||
| 178 | static void vgic_release_deleted_lpis(struct kvm *kvm) | 176 | static void vgic_release_deleted_lpis(struct kvm *kvm) |
| @@ -184,7 +182,7 @@ static void vgic_release_deleted_lpis(struct kvm *kvm) · 5 unmodified lines | |||
| 184 | xa_lock_irqsave(&dist->lpi_xa, flags); | 182 | xa_lock_irqsave(&dist->lpi_xa, flags); |
| 185 | 183 | ||
| 186 | xa_for_each(&dist->lpi_xa, intid, irq) { | 184 | xa_for_each(&dist->lpi_xa, intid, irq) { |
| 187 | if (irq->pending_release) | 185 | if (!refcount_read(&irq->refcount)) |
| 188 | vgic_release_lpi_locked(dist, irq); | 186 | vgic_release_lpi_locked(dist, irq); |
| 189 | } | 187 | } |
| 190 | 188 | ||
| @@ -247,9 +247,6 @@ struct vgic_irq { · 246 unmodified lines | |||
| 247 | * affinity reg (v3). | 247 | * affinity reg (v3). |
| 248 | */ | 248 | */ |
| 249 | 249 | ||
| 250 | bool pending_release:1; /* Used for LPIs only, unreferenced IRQ | ||
| 251 | * pending a release */ | ||
| 252 | |||
| 253 | bool pending_latch:1; /* The pending latch state used to calculate | 250 | bool pending_latch:1; /* The pending latch state used to calculate |
| 254 | * the pending state for both level | 251 | * the pending state for both level |
| 255 | * and edge triggered IRQs. */ | 252 | * and edge triggered IRQs. */ |