Re: [PATCH v3 11/23] dmaengine: sdxi: Add client context alloc and release APIs

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Critical] Inverted teardown sequence in `sdxi_unregister()` causes DMA Use-After-Free and host Use-After-Free by freeing contexts while active.
- [High] Context DMA memory is freed without hardware synchronization, leading to use-after-free by the device.
--

commit f66266c7bb64ddb6a5bf8b858f8554af8ba46c5a
Author: Nathan Lynch <nathan.lynch@amd.com>
Subject: dmaengine: sdxi: Add client context alloc and release APIs

This commit exposes sdxi_cxt_new() and sdxi_cxt_exit() to create and release
SDXI contexts, tracking them in an xarray. It also introduces logic to
invalidate a context's L1 entry upon deallocation.

> diff --git a/drivers/dma/sdxi/context.c b/drivers/dma/sdxi/context.c
> index cc99fe9ecf928..2f9a29df5d9ce 100644
> --- a/drivers/dma/sdxi/context.c
> +++ b/drivers/dma/sdxi/context.c

[ ... ]

> +/* Invalidate a context. */
> +static void sdxi_rescind_cxt(struct sdxi_cxt *cxt)
> +{
> +	u8 l1_idx = ID_TO_L1_INDEX(cxt->id);
> +	struct sdxi_cxt_L1_ent *ent = &cxt->sdxi->L1_table->entry[l1_idx];
> +
> +	invalidate_L1_entry(ent);
> +	invalidate_cxtl_ctl(cxt->cxt_ctl);
> +	/* todo: need to send DSC_CXT_UPD to admin */
> +}

[ ... ]

> +void sdxi_cxt_exit(struct sdxi_cxt *cxt)
> +{
> +	if (WARN_ON(sdxi_cxt_is_admin(cxt)))
> +		return;
> +
> +	sdxi_rescind_cxt(cxt);
> +	sdxi_free_cxt(cxt);
> +}

[Severity: High]
Does this code free context DMA memory without synchronizing with the hardware?

By clearing the valid bits but leaving the DSC_CXT_UPD synchronization for a
later TODO in sdxi_rescind_cxt(), the device is not notified and its internal
caches may still reference these context structures.

When sdxi_cxt_exit() immediately calls sdxi_free_cxt() afterwards, could the
hardware perform DMA accesses on memory that has been returned to the
allocators, leading to a device use-after-free?

[ ... ]

> diff --git a/drivers/dma/sdxi/device.c b/drivers/dma/sdxi/device.c
> index b875d8cdb58af..d3d1ad2f2effe 100644
> --- a/drivers/dma/sdxi/device.c
> +++ b/drivers/dma/sdxi/device.c

[ ... ]

> @@ -346,6 +348,12 @@ EXPORT_SYMBOL_NS_GPL(sdxi_register, "SDXI");
>  void sdxi_unregister(struct device *dev)
>  {
>  	struct sdxi_dev *sdxi = dev_get_drvdata(dev);
> +	struct sdxi_cxt *cxt;
> +	unsigned long index;
> +
> +	xa_for_each(&sdxi->client_cxts, index, cxt)
> +		sdxi_cxt_exit(cxt);
> +	xa_destroy(&sdxi->client_cxts);
>  
>  	sdxi_dev_stop(sdxi);
>  }

[Severity: Critical]
Does this teardown sequence result in DMA memory corruption and a host
double-free?

By forcefully freeing all client context DMA memory via sdxi_cxt_exit()
before calling sdxi_dev_stop(), the device hardware is still active and the
contexts themselves have never been stopped via sdxi_stop_cxt().

Furthermore, since these contexts are allocated per channel, won't the DMA
engine framework later invoke sdxi_dma_free_chan_resources() on channel
release or devres teardown, which will attempt to stop and free the already
freed sdchan->cxt?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260605-sdxi-base-v3-0-4d38ca2bdffe@amd.com?part=11