Re: [PATCH] ata: pata_pxa: Fix DMA channel leak on probe error

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Incorrect teardown order in `pxa_ata_remove()` releases the DMA channel before detaching the ATA host, creating a use-after-free window.
- [High] Missing NULL check for platform data (`pdata`) before dereferencing it in the probe function.
- [High] Returning `AC_ERR_OK` when DMA descriptor preparation fails leads to an unrecoverable 30-second ATA command timeout.
--

commit fb8345f4cc3cb507abc9c29fac4d410d641eab22
Author: Wentao Liang <vulab@iscas.ac.cn>
Subject: ata: pata_pxa: Fix DMA channel leak on probe error

When dmaengine_slave_config() fails, the DMA channel acquired by
dma_request_chan() is not released before returning the error,
leaking the channel reference. This commit adds dma_release_channel()
in the error path.

> diff --git a/drivers/ata/pata_pxa.c b/drivers/ata/pata_pxa.c
> index 03dbaf4a13a75..9f63bdfb8576e 100644
> --- a/drivers/ata/pata_pxa.c
> +++ b/drivers/ata/pata_pxa.c
> @@ -286,6 +286,7 @@ static int pxa_ata_probe(struct platform_device *pdev)
>  	ret = dmaengine_slave_config(data->dma_chan, &config);
>  	if (ret < 0) {
>  		dev_err(&pdev->dev, "dma configuration failed: %d\n", ret);
> +		dma_release_channel(data->dma_chan);
>  		return ret;
>  	}

While reviewing this patch, I noticed a few pre-existing issues in the same
file.

In pxa_ata_probe():

> 	struct pata_pxa_pdata *pdata = dev_get_platdata(&pdev->dev);

[ ... ]

> 	/*
> 	 * Adjust register offsets
> 	 */
> 	ap->ioaddr.altstatus_addr = ap->ioaddr.ctl_addr;
> 	ap->ioaddr.data_addr	= ap->ioaddr.cmd_addr +
> 					(ATA_REG_DATA << pdata->reg_shift);

[Severity: High]
This isn't a bug introduced by this patch, but is it possible for pdata to be
NULL here?

If the driver is bound manually via sysfs to a platform device lacking
platform data, dev_get_platdata() will return NULL, which would cause a
direct kernel panic when dereferencing pdata->reg_shift.


In pxa_qc_prep():

> 	tx = dmaengine_prep_slave_sg(pd->dma_chan, qc->sg, qc->n_elem, dir,
> 				     DMA_PREP_INTERRUPT);
> 	if (!tx) {
> 		ata_dev_err(qc->dev, "prep_slave_sg() failed\n");
> 		return AC_ERR_OK;
> 	}

[Severity: High]
This isn't a bug introduced by this patch, but should this return an error
code like AC_ERR_SYSTEM instead of AC_ERR_OK?

Returning AC_ERR_OK when dmaengine_prep_slave_sg() fails incorrectly masks
the error from the ATA core. The core proceeds to issue the command thinking
it was prepared, but since no DMA descriptors were actually submitted, the
device will never complete the transfer.

This inevitably results in a 30-second block layer timeout.


In pxa_ata_remove():

> static void pxa_ata_remove(struct platform_device *pdev)
> {
> 	struct ata_host *host = platform_get_drvdata(pdev);
> 	struct pata_pxa_data *data = host->ports[0]->private_data;
> 
> 	dma_release_channel(data->dma_chan);
> 
> 	ata_host_detach(host);
> }

[Severity: High]
This isn't a bug introduced by this patch, but is the teardown order safe
here?

ata_host_detach() unregisters the SCSI host, which can invoke the ATA error
handler (EH) to flush or abort pending commands. If an ongoing command is
aborted, the EH invokes driver callbacks like pxa_bmdma_stop(), which
attempt to call dmaengine_tx_status() and dmaengine_terminate_all() on the
already freed data->dma_chan.

Does calling dma_release_channel() before ata_host_detach() create a
use-after-free window?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260625141837.62362-1-vulab@iscas.ac.cn?part=1