Re: [PATCH] dmaengine: txx9dmac: use devm_platform_ioremap_resource()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] The error path in `txx9dmac_chan_probe` fails to kill `dc->tasklet` and leaves a dangling pointer in `dc->ddev->chan`, leading to Use-After-Free.
- [High] Missing `dma_mapping_error()` check after mapping memory with `dma_map_single()` in `txx9dmac_desc_alloc`.
--

--- Patch [1]: [PATCH] dmaengine: txx9dmac: use devm_platform_ioremap_resource() ---
commit bc23f2a8099e4ec156f46c83d8a2cae426f12327
Author: Rosen Penev <rosenp@gmail.com>

dmaengine: txx9dmac: use devm_platform_ioremap_resource()

Replace the open-coded platform_get_resource() plus devm_request_mem_region()
and devm_ioremap() sequence with a single devm_platform_ioremap_resource()
call. This folds the resource lookup, region reservation and mapping into
one step and returns an ERR_PTR on failure. This is behaviorally equivalent
as the driver already reserved the region.

> diff --git a/drivers/dma/txx9dmac.c b/drivers/dma/txx9dmac.c
> index 05622b68a9363..6595a54a4b97f 100644
> --- a/drivers/dma/txx9dmac.c
> +++ b/drivers/dma/txx9dmac.c
> @@ -1167,26 +1167,20 @@ static void txx9dmac_chan_remove(struct platform_device *pdev)
>  static int __init txx9dmac_probe(struct platform_device *pdev)
>  {
>  	struct txx9dmac_platform_data *pdata = dev_get_platdata(&pdev->dev);
> -	struct resource *io;
>  	struct txx9dmac_dev *ddev;
> +	void __iomem *regs;
>  	u32 mcr;
>  	int err;
>  
> -	io = platform_get_resource(pdev, IORESOURCE_MEM, 0);
> -	if (!io)
> -		return -EINVAL;
> +	regs = devm_platform_ioremap_resource(pdev, 0);
> +	if (IS_ERR(regs))
> +		return PTR_ERR(regs);
>  
>  	ddev = devm_kzalloc(&pdev->dev, sizeof(*ddev), GFP_KERNEL);
>  	if (!ddev)
>  		return -ENOMEM;
>  
> -	if (!devm_request_mem_region(&pdev->dev, io->start, resource_size(io),
> -				     dev_name(&pdev->dev)))
> -		return -EBUSY;
> -
> -	ddev->regs = devm_ioremap(&pdev->dev, io->start, resource_size(io));
> -	if (!ddev->regs)
> -		return -ENOMEM;
> +	ddev->regs = regs;
>  	ddev->have_64bit_regs = pdata->have_64bit_regs;
>  	if (__is_dmac64(ddev))
>  		ddev->descsize = sizeof(struct txx9dmac_hwdesc);

While reviewing this file, I noticed a couple of pre-existing issues.

drivers/dma/txx9dmac.c:txx9dmac_chan_probe() {
    ...
	dc->ddev->chan[ch] = dc;
	dc->chan.device = &dc->dma;
	list_add_tail(&dc->chan.device_node, &dc->chan.device->channels);
	dma_cookie_init(&dc->chan);
    ...
	err = dma_async_device_register(&dc->dma);
	if (err)
		return err;
    ...
}

[Severity: High]
This is a pre-existing issue, but does this error path lead to a
Use-After-Free?

If dma_async_device_register() fails, the function returns without performing
cleanup. It leaves a dangling pointer in dc->ddev->chan[ch] and fails to call
tasklet_kill(&dc->tasklet), unlike the cleanup in txx9dmac_chan_remove().

Because the devres framework will free the dc structure, a previously scheduled
tasklet or an interrupt handler referencing ddev->chan[0] could dereference the
freed dc structure.


drivers/dma/txx9dmac.c:txx9dmac_desc_alloc() {
    ...
	desc->txd.phys = dma_map_single(chan2parent(&dc->chan), &desc->hwdesc,
					ddev->descsize, DMA_TO_DEVICE);
	return desc;
}

[Severity: High]
This is a pre-existing issue, but does this code need to check for DMA mapping
errors?

The physical address returned by dma_map_single() is assigned and returned
without being validated with dma_mapping_error().

If the mapping fails, it returns a bad DMA address, and the hardware will
subsequently attempt to access this invalid address during descriptor fetching.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260714234742.908956-1-rosenp@gmail.com?part=1