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

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Hardware retains access to freed DMA memory due to missing L1 entry invalidation during teardown.
- [Medium] Incorrect error check for `dma_pool_zalloc` allocation of `cxt->cxt_ctl`.
--

--- Patch [8]: [PATCH 08/23] dmaengine: sdxi: Install administrative context ---
commit 17401dc26aa25b16b19cf4cf87900b7c0d64a889
Author: Nathan Lynch <[email protected]>

dmaengine: sdxi: Install administrative context

Serialize the context control block, akey table, and L1 entry for the
admin context, making its descriptor ring, write index, and context
status block visible to the SDXI implementation once it is activated.

> diff --git a/drivers/dma/sdxi/context.c b/drivers/dma/sdxi/context.c
> index 0a6821992776c..097d871e530fd 100644
> --- a/drivers/dma/sdxi/context.c
> +++ b/drivers/dma/sdxi/context.c

[ ... ]

> @@ -115,13 +267,23 @@ static void free_admin_cxt(void *ptr)
>  
>  int sdxi_admin_cxt_init(struct sdxi_dev *sdxi)
>  {
> +	int err;
> +	struct sdxi_sq *sq;
> +
>  	struct sdxi_cxt *cxt __free(sdxi_cxt) = sdxi_alloc_cxt(sdxi);

[Severity: Medium]
In sdxi_alloc_cxt() called here, the code handles allocation as follows:

drivers/dma/sdxi/context.c:sdxi_alloc_cxt() {
    ...
    cxt->cxt_ctl = dma_pool_zalloc(sdxi->cxt_ctl_pool, GFP_KERNEL,
                                   &cxt->cxt_ctl_dma);
    if (!cxt->cxt_ctl_dma)
        return NULL;
    ...
}

Is this the correct check for dma_pool_zalloc() failure? DMA address 0 can be
a valid address returned by the DMA API on some platforms, which would cause
this check to falsely treat a successful allocation as a failure. Should it
check if (!cxt->cxt_ctl) instead?

>  	if (!cxt)
>  		return -ENOMEM;
>  
> +	sq = cxt->sq;
> +	/* SDXI 1.0 4.1.8.4.b: Set CXT_STS.state to CXTV_RUN. */
> +	sq->cxt_sts->state = FIELD_PREP(SDXI_CXT_STS_STATE, CXTV_RUN);
>  	cxt->id = SDXI_ADMIN_CXT_ID;
>  	cxt->db = sdxi->dbs + cxt->id * sdxi->db_stride;
>  
> +	err = sdxi_publish_cxt(cxt);
> +	if (err)
> +		return err;
> +
>  	sdxi->admin_cxt = no_free_ptr(cxt);
>  
>  	return devm_add_action_or_reset(sdxi_to_dev(sdxi), free_admin_cxt, sdxi);

[Severity: High]
When an administrator unbinds the driver via sysfs or unloads the module, the
devres teardown path calls free_admin_cxt(). This ultimately calls
sdxi_free_cxt() which frees the DMA memory back to the kernel.

Since sdxi_publish_cxt() now writes the VL=1 bit and makes the memory visible
to the hardware, does the driver need to clear this bit in the L1 table entry
and stop the hardware before freeing the memory?

The driver also lacks a .remove callback in sdxi_driver (in
drivers/dma/sdxi/pci.c) to safely stop the PCI device and its DMA operations
before devres cleanup begins.

Will the active hardware continue to fetch descriptors and write statuses to
the now-freed memory pages, causing a DMA use-after-free and potential kernel
memory corruption?

>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=8