Re: [PATCH v2 08/23] dmaengine: sdxi: Install administrative context

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] Hardware Use-After-Free due to missing admin context teardown
- [High] Torn writes on 32-bit platforms exposing incomplete addresses to hardware
- [Medium] Incorrect error check for `dma_pool_zalloc()` using the DMA handle instead of the virtual pointer
--

commit 794d6cb84f7ad6a7699311744a5e9c87b9d20316
Author: Nathan Lynch <nathan.lynch@amd.com>

dmaengine: sdxi: Install administrative context

This commit serializes the context control block, akey table, and L1 entry for
the admin context. It makes the descriptor ring, write index, and context
status block visible to the SDXI implementation once activated.

> diff --git a/drivers/dma/sdxi/context.c b/drivers/dma/sdxi/context.c
> index 27821cfaf0311..c0b55c945cc4e 100644
> --- a/drivers/dma/sdxi/context.c
> +++ b/drivers/dma/sdxi/context.c

[ ... ]

> @@ -106,6 +112,152 @@ static struct sdxi_cxt *sdxi_alloc_cxt(struct sdxi_dev *sdxi)

[ ... ]

> +static int configure_cxt_ctl(struct sdxi_cxt_ctl *ctl, const struct sdxi_cxt_ctl_cfg *cfg)
> +{
> +	u64 ds_ring_ptr, cxt_sts_ptr, write_index_ptr;

[ ... ]

> +	ds_ring_ptr = FIELD_PREP(SDXI_CXT_CTL_VL, 1) |
> +		      FIELD_PREP(SDXI_CXT_CTL_QOS, cfg->qos) |
> +		      FIELD_PREP(SDXI_CXT_CTL_SE, cfg->se) |
> +		      FIELD_PREP(SDXI_CXT_CTL_CSA, cfg->csa) |
> +		      FIELD_PREP(SDXI_CXT_CTL_DS_RING_PTR,
> +				 cfg->ds_ring_ptr >> DESC_RING_BASE_PTR_SHIFT);
> +	/* Ensure other fields are visible before hw sees vl=1. */
> +	dma_wmb();
> +	WRITE_ONCE(ctl->ds_ring_ptr, cpu_to_le64(ds_ring_ptr));

Can this introduce a regression with torn writes on 32-bit platforms?

Since the driver does not strictly depend on 64BIT in Kconfig, writing a
64-bit structure pointer that also contains the valid bit (VL=1 at bit 0)
via WRITE_ONCE() translates to two separate 32-bit stores.

If the compiler orders the lower 32-bit store before the upper 32-bit store,
could the hardware concurrently read the descriptor and fetch from an
incomplete, torn physical address?

> +
> +	return 0;
> +}
> +
> +/*
> + * Logical representation of CXT_L1_ENT subfields.
> + */

[ ... ]

> +static int configure_L1_entry(struct sdxi_cxt_L1_ent *ent,
> +			      const struct sdxi_cxt_L1_cfg *cfg)
> +{

[ ... ]

> +	cxt_ctl_ptr = FIELD_PREP(SDXI_CXT_L1_ENT_VL, 1) |
> +		      FIELD_PREP(SDXI_CXT_L1_ENT_KA, cfg->ka) |
> +		      FIELD_PREP(SDXI_CXT_L1_ENT_PV, cfg->pv) |
> +		      FIELD_PREP(SDXI_CXT_L1_ENT_CXT_CTL_PTR,
> +				 cfg->cxt_ctl_ptr >> L1_CXT_CTRL_PTR_SHIFT);
> +	/* Ensure other fields are visible before hw sees vl=1. */
> +	dma_wmb();
> +	WRITE_ONCE(ent->cxt_ctl_ptr, cpu_to_le64(cxt_ctl_ptr));

Similarly, could this 64-bit WRITE_ONCE() cause torn writes on 32-bit
platforms, exposing an incomplete physical address to the hardware?

> +
> +	return 0;
> +}
> +

[ ... ]

> +static void free_admin_cxt(void *ptr)
> +{
> +	struct sdxi_dev *sdxi = ptr;
> +
> +	sdxi_free_cxt(sdxi->admin_cxt);
> +}

Does this sequence cause a use-after-free regression?

In sdxi_publish_cxt(), the admin context is published to the hardware by
setting the valid bit (VL=1) in the L1_table.

When the context is torn down via the devres cleanup action free_admin_cxt(),
it directly invokes sdxi_free_cxt(), which frees the DMA memory for the
context control block, descriptor ring, and akey table.

However, the VL bit in the hardware's L1 table entry is never cleared. Could
this leave the hardware actively fetching from freed physical addresses?

Additionally, looking at the sdxi_alloc_cxt() implementation, the return value
of dma_pool_zalloc() is checked via the DMA address instead of the virtual
pointer:

	cxt->cxt_ctl = dma_pool_zalloc(sdxi->cxt_ctl_pool, GFP_KERNEL,
				       &cxt->cxt_ctl_dma);
	if (!cxt->cxt_ctl_dma)
		return NULL;

Since physical address 0 is a valid DMA address on many architectures and IOMMU
configurations, could this regression incorrectly abort initialization if a
successful allocation returns DMA address 0? Should this code check the
returned virtual pointer (!cxt->cxt_ctl) instead?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260511-sdxi-base-v2-0-889cfed17e3f@amd.com?part=8