Re: [PATCH 09/17] dmaengine: idxd: quarantine terminate_all's descriptor slots until the device has drained

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] dmaengine: idxd: missing wq_lock in idxd_dma_synchronize() causes UAF
- [High] dmaengine: idxd: missing idxd_wq_thread() sync on channel termination
--

commit 9c6985db5bd50b1fe55f86734ee2fd37bc6a5f05
Author: Daniel Byrne <daniel1.byrne@intel.com>

dmaengine: idxd: quarantine terminate_all's descriptor slots until the device has drained

This commit prevents premature descriptor slot and DMA unmap data reuse on
idxd_dma_terminate_all() by moving in-flight aborted descriptors to a
per-queue quarantine list. The quarantined slots and unmap data are then
released in idxd_dma_synchronize() after draining the workqueue, or upon
device halt or workqueue resource teardown.

> diff --git a/drivers/dma/idxd/device.c b/drivers/dma/idxd/device.c
> index 41223d2e20af0..09475f5ea026d 100644
> --- a/drivers/dma/idxd/device.c
> +++ b/drivers/dma/idxd/device.c

[ ... ]

> @@ -170,10 +172,18 @@ EXPORT_SYMBOL_NS_GPL(idxd_wq_alloc_resources, "IDXD");
>  void idxd_wq_free_resources(struct idxd_wq *wq)
>  {
>  	struct device *dev = &wq->idxd->pdev->dev;
> +	LIST_HEAD(dead_descs);
>  
>  	if (wq->type != IDXD_WQT_KERNEL)
>  		return;
>  
> +	/*
> +	 * The disable frees the pool only after its drain and its reset
> +	 * completed, and a failed enable has run nothing, so the device no
> +	 * longer reaches what a quarantined descriptor maps.
> +	 */
> +	idxd_wq_quarantine_take(wq, &dead_descs);
> +	idxd_wq_quarantine_free(wq, &dead_descs);
>  	free_hw_descs(wq);
>  	free_descs(wq);
>  	dma_free_coherent(dev, wq->compls_size, wq->compls, wq->compls_addr);

[ ... ]

> @@ -1476,16 +1563,38 @@ void idxd_wq_free_irq(struct idxd_wq *wq)
>  	ie->pasid = IOMMU_PASID_INVALID;
>  }
>  
> -void idxd_wq_flush_descs(struct idxd_wq *wq)
> +/*
> + * @quarantine: the device keeps running, so the slots of the descriptors
> + * it has not finished stay out of use until a drain covers them. A halt
> + * has killed them and frees their slots at once.
> + */
> +void idxd_wq_flush_descs(struct idxd_wq *wq, bool quarantine)
>  {
>  	struct idxd_irq_entry *ie = &wq->ie;
> +	bool killed = false;
>  
>  	guard(mutex)(&wq->wq_lock);
>  
>  	if (wq->state != IDXD_WQ_ENABLED || wq->type != IDXD_WQT_KERNEL)
>  		return;
>  
> -	idxd_flush_pending_descs(ie);
> +	/*
> +	 * A submitter between its list add and its portal write holds
> +	 * wq_active. Waiting for the reference to die puts every listed
> +	 * descriptor in the device, where the drain that frees the
> +	 * quarantine covers it. A halt flush has done this itself.
> +	 */
> +	if (quarantine && !percpu_ref_is_dying(&wq->wq_active)) {
> +		reinit_completion(&wq->wq_resurrect);
> +		percpu_ref_kill(&wq->wq_active);
> +		wait_for_completion(&wq->wq_dead);
> +		killed = true;
> +	}
> +	idxd_flush_pending_descs(ie, quarantine);
> +	if (killed) {
> +		percpu_ref_reinit(&wq->wq_active);
> +		complete_all(&wq->wq_resurrect);
> +	}
>  }

[Severity: High]
How are idxd_wq_flush_descs() and idxd_dma_synchronize() synchronized
against the per-WQ MSI-X threaded interrupt handler idxd_wq_thread()?

In drivers/dma/idxd/device.c, idxd_wq_flush_descs() kills wq->wq_active to
wait for submitters before calling idxd_flush_pending_descs(), which drains
ie->pending_llist and ie->work_list under ie->list_lock:

	spin_lock(&ie->list_lock);
	head = llist_del_all(&ie->pending_llist);
	if (head) {
		llist_for_each_entry_safe(desc, itr, head, llnode)
			list_add_tail(&desc->list, &ie->work_list);
	}

	list_for_each_entry_safe(desc, itr, &ie->work_list, list)
		list_move_tail(&desc->list, &flist);
	spin_unlock(&ie->list_lock);

However, idxd_wq_thread() remains active during both idxd_dma_terminate_all()
and idxd_dma_synchronize().

First, at this commit, irq_process_pending_llist() in drivers/dma/idxd/irq.c
detaches ie->pending_llist outside ie->list_lock and re-queues incomplete
descriptors one at a time:

	head = llist_del_all(&irq_entry->pending_llist);
	if (!head)
		return;

	llist_for_each_entry_safe(desc, t, head, llnode) {
		u8 status = desc->completion->status & DSA_COMP_STATUS_MASK;

		if (status) {
			...
		} else {
			spin_lock(&irq_entry->list_lock);
			list_add_tail(&desc->list,
				      &irq_entry->work_list);
			spin_unlock(&irq_entry->list_lock);
		}
	}

If idxd_flush_pending_descs() runs while irq_process_pending_llist() holds
the detached list in head, those descriptors are missed by the flush and
subsequently added back onto ie->work_list un-aborted and un-quarantined.
(A later commit in this series, "dmaengine: idxd: fix the double free of a
descriptor whose submission failed", moves llist_del_all() and the work_list
insertion under ie->list_lock, which closes that window for incomplete
descriptors.)

Second, even with that later change, both irq_process_work_list() and
irq_process_pending_llist() move completed descriptors onto a local flist
under ie->list_lock, drop ie->list_lock, and then invoke
idxd_desc_complete() -> idxd_dma_complete_txd() outside the lock. Any
descriptor moved to idxd_wq_thread()'s local flist before
idxd_flush_pending_descs() takes ie->list_lock will not have its callback
cleared.

Because idxd_dma_synchronize() only drains the hardware queue via
idxd_wq_drain(wq) and does not synchronize with the threaded IRQ handler
(for example via synchronize_irq(wq->ie.vector)), could
idxd_dma_synchronize() return while idxd_wq_thread() is still running
dma_descriptor_unmap() and dmaengine_desc_get_callback_invoke() on
previously submitted descriptors, leading to a use-after-free if the client
frees its callback context after dmaengine_synchronize()?

> diff --git a/drivers/dma/idxd/dma.c b/drivers/dma/idxd/dma.c
> index defd1af74c0a9..196045cb9ef2d 100644
> --- a/drivers/dma/idxd/dma.c
> +++ b/drivers/dma/idxd/dma.c
> @@ -199,7 +199,8 @@ static int idxd_dma_terminate_all(struct dma_chan *c)
>  {
>  	struct idxd_wq *wq = to_idxd_wq(c);
>  
> -	idxd_wq_flush_descs(wq);
> +	/* The device keeps running what the lists held. See synchronize. */
> +	idxd_wq_flush_descs(wq, true);
>  
>  	return 0;
>  }
>  
>  static void idxd_dma_synchronize(struct dma_chan *c)
>  {
>  	struct idxd_wq *wq = to_idxd_wq(c);
> +	LIST_HEAD(quarantined);
>  
> +	/*
> +	 * dmaengine_synchronize() promises the client that its memory may
> +	 * be freed afterwards. The drain keeps that promise, and nothing
> +	 * the device held before it is left to write a completion record,
> +	 * so the slots terminate_all quarantined are free again.
> +	 */
> +	idxd_wq_quarantine_take(wq, &quarantined);
>  	idxd_wq_drain(wq);
> +	idxd_wq_quarantine_free(wq, &quarantined);
>  }

[Severity: High]
Can idxd_dma_synchronize() race with workqueue teardown and cause a
use-after-free on wq->descs and wq->sbq?

idxd_wq_halt_release() in drivers/dma/idxd/device.c acquires wq->wq_lock and
checks wq->state and wq->type before touching the quarantine:

void idxd_wq_halt_release(struct idxd_wq *wq)
{
	LIST_HEAD(dead_descs);

	guard(mutex)(&wq->wq_lock);

	if (wq->state != IDXD_WQ_ENABLED || wq->type != IDXD_WQT_KERNEL)
		return;

	idxd_wq_quarantine_take(wq, &dead_descs);
	idxd_wq_quarantine_free(wq, &dead_descs);
}

In contrast, idxd_dma_synchronize() splices wq->quarantine onto its local
stack list under only wq->ie.list_lock, sleeps in idxd_wq_drain() ->
idxd_cmd_exec(), and then frees the descriptors without holding wq->wq_lock.

If workqueue teardown runs concurrently (such as via sysfs unbind, PCI
removal in idxd_remove(), or FLR recovery in idxd_reset_done() calling
idxd_unbind() -> idxd_device_drv_remove() -> idxd_dmaengine_drv_remove() ->
idxd_drv_disable_wq() -> idxd_wq_free_resources()), idxd_wq_free_resources()
sees wq->quarantine empty because the descriptors were already moved onto
idxd_dma_synchronize()'s stack list, and proceeds to free wq->descs[i] and
wq->sbq:

	idxd_wq_quarantine_take(wq, &dead_descs);
	idxd_wq_quarantine_free(wq, &dead_descs);
	free_hw_descs(wq);
	free_descs(wq);
	dma_free_coherent(dev, wq->compls_size, wq->compls, wq->compls_addr);
	sbitmap_queue_free(&wq->sbq);

When idxd_wq_drain() returns in idxd_dma_synchronize(),
idxd_wq_quarantine_free() (or idxd_wq_quarantine_put_back() in the later
commit "dmaengine: idxd: return -ETIMEDOUT from drains and resets and leak
what the device may still write") will dereference the freed struct idxd_desc
objects and call idxd_free_desc() -> sbitmap_queue_clear() on the freed
wq->sbq. Should idxd_dma_synchronize() also serialize under wq->wq_lock and
verify the workqueue state before taking and freeing quarantined descriptors?

>  
>  int idxd_register_dma_device(struct idxd_device *idxd)

[ ... ]

>

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261019150630.4026341-1-daniel1.byrne@intel.com?part=9