Re: [PATCH 14/17] dmaengine: idxd: complete an iaa_crypto descriptor through its own driver on abort and flush

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] dmaengine: idxd: active DMA unmap and UAF in idxd_flush_pending_descs()
--

commit 17311ece6ccc1def94a5c98267c599339eae70ce
Author: Daniel Byrne <daniel1.byrne@intel.com>

dmaengine: idxd: complete an iaa_crypto descriptor through its own driver on abort and flush

This commit updates idxd_flush_pending_descs() and llist_abort_desc() to
complete or skip non-dmaengine descriptors via their own driver's completion
handler rather than treating them as dmaengine transactions, and avoids
blocking in idxd_alloc_desc() when a workqueue is permanently dying.

> diff --git a/drivers/dma/idxd/device.c b/drivers/dma/idxd/device.c
> index 222c201669bdd..a33c4309f5d42 100644
> --- a/drivers/dma/idxd/device.c
> +++ b/drivers/dma/idxd/device.c
> @@ -1534,10 +1534,25 @@ static void idxd_flush_pending_descs(struct idxd_irq_entry *ie,
>  		list_del(&desc->list);
>  		ctype = desc->completion->status ? IDXD_COMPLETE_NORMAL : IDXD_COMPLETE_ABORT;
>  		/*
> -		 * wq is being disabled. Any remaining descriptors are
> -		 * likely to be stuck and can be dropped. callback could
> -		 * point to code that is no longer accessible, for example
> -		 * if dmatest module has been unloaded.
> +		 * Another driver's descriptor carries that driver's request
> +		 * in place of a txd, and a client may still wait for it, so it
> +		 * goes back through that driver's completion. The completion
> +		 * reads the outcome from the record, which the device never
> +		 * wrote for an unfinished descriptor. Only a dmaengine channel
> +		 * terminates, so nothing of such a queue is quarantined.
> +		 */
> +		if (!idxd_wq_has_txd(wq)) {
> +			if (ctype == IDXD_COMPLETE_ABORT)
> +				desc->completion->status = IDXD_COMP_DESC_ABORT;
> +			idxd_desc_complete(desc, ctype, true);
> +			continue;
> +		}

[Severity: Critical]
When an iaa_crypto workqueue with in-flight interrupt-armed requests is
disabled via iaa_crypto_remove() -> idxd_drv_disable_wq(), idxd_wq_free_irq()
calls idxd_flush_pending_descs() unconditionally even if idxd_wq_drain()
timed out:

drivers/dma/idxd/device.c:idxd_drv_disable_wq() {
	...
	idxd_wq_unmap_portal(wq);
	rc = idxd_wq_drain(wq);
	idxd_wq_free_irq(wq);
	rc |= idxd_wq_reset(wq);
	...
}

If idxd_wq_drain() returns -ETIMEDOUT (either because DRAIN_WQ timed out or
because an earlier command set IDXD_FLAG_CMD_TIMEDOUT), the hardware engine is
still active and processing unfinished descriptors when
idxd_flush_pending_descs() runs.

For an unfinished descriptor (ctype == IDXD_COMPLETE_ABORT), calling
idxd_desc_complete() invokes iaa_desc_complete():

drivers/crypto/intel/iaa/iaa_crypto_main.c:iaa_desc_complete() {
	...
err:
	dma_unmap_sg(dev, ctx->req->dst, sg_nents(ctx->req->dst), DMA_FROM_DEVICE);
	iaa_unmap_src(dev, ctx->req);
out:
	if (ret != 0)
		dev_dbg(dev, "asynchronous compress failed ret=%d\n", ret);

	if (ctx->req->base.complete)
		acomp_request_complete(ctx->req, err);

	if (free_desc)
		idxd_free_desc(idxd_desc->wq, idxd_desc);
	iaa_wq_put(idxd_desc->wq);
}

Doesn't completing an unfinished descriptor here while the device is still
active cause active DMA unmapping and use-after-free? Specifically:

1. iaa_desc_complete() unmaps ctx->req->dst and ctx->req->src (and via
iaa_unmap_src() unmaps req_ctx->bounce_src_dma and frees req_ctx->bounce_src
back to iaa_bounce_pool) while the hardware engine may still be DMA-reading
src1_addr and DMA-writing dst_addr.

2. acomp_request_complete() notifies the caller, which can free or reuse the
request and scatterlist buffers while hardware DMA is still in flight.

3. iaa_wq_put() drops the iaa_wq reference. During iaa_crypto_remove(),
iaa_wq->remove is set to true, so dropping the last reference calls
__free_iaa_wq() -> free_iaa_device() -> remove_device_compression_modes() ->
free_device_compression_mode(), freeing device_mode->aecs_comp_table via
dma_free_coherent() while an in-flight compression descriptor in hardware
still reads src2_addr (aecs_comp_table_dma_addr).

4. Because iaa_desc_complete() ignores comp_type and inspects
idxd_desc->iax_completion->status via check_completion(), can a concurrent
hardware completion write between setting desc->completion->status =
IDXD_COMP_DESC_ABORT and check_completion() overwrite IDXD_COMP_DESC_ABORT
with IAX_COMP_SUCCESS or IAA_ANALYTICS_ERROR, causing iaa_desc_complete() to
treat the aborted descriptor as succeeded or retryable during teardown?

>  		/*
> -		 * wq is being disabled. Any remaining descriptors are
[ ... ]
>  		tx = &desc->txd;
>  		tx->callback = NULL;

[ ... ]

>
> -- 
> 2.51.2
>

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