Re: [PATCH 2/3] ata: pata_ep93xx: use unsigned long for data

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Double free and invalid pointer dereference on DMA TX channel request failure
--

commit 29937e8c8082c869105691e9f38d16e45bbd37fb
Author: Rosen Penev <rosenp@gmail.com>

ata: pata_ep93xx: use unsigned long for data

An int is being encoded as a void pointer but that breaks on 64-bit
systems as the type needs to match pointer size.

> diff --git a/drivers/ata/pata_ep93xx.c b/drivers/ata/pata_ep93xx.c
> index f7e87d6af2641..1663dcd00a932 100644
> --- a/drivers/ata/pata_ep93xx.c
> +++ b/drivers/ata/pata_ep93xx.c
> @@ -977,7 +977,7 @@ static int ep93xx_pata_probe(struct platform_device *pdev)
>  
>  		match = soc_device_match(ep93xx_soc_table);
>  		if (match)
> -			ap->udma_mask = (unsigned int) match->data;
> +			ap->udma_mask = (unsigned long) match->data;

[Severity: High]
This is a pre-existing issue, but while reviewing this file I noticed a
potential double free and invalid pointer dereference in the DMA error
handling paths.

If dma_request_chan() for the "tx" channel fails in ep93xx_pata_dma_init(),
it returns an ERR_PTR which is stored in drv_data->dma_tx_channel, and jumps
to fail_release_rx:

drivers/ata/pata_ep93xx.c:ep93xx_pata_dma_init() {
    ...
	drv_data->dma_tx_channel = dma_request_chan(&pdev->dev, "tx");
	if (IS_ERR(drv_data->dma_tx_channel)) {
		ret = dev_err_probe(dev, PTR_ERR(drv_data->dma_tx_channel),
				    "tx DMA setup failed\n");
		goto fail_release_rx;
	}
    ...
fail_release_rx:
	dma_release_channel(drv_data->dma_rx_channel);
fail_release_dma:
	ep93xx_pata_release_dma(drv_data);
}

The fail_release_rx path calls dma_release_channel() on the rx channel but
doesn't set drv_data->dma_rx_channel to NULL. It then falls through to
fail_release_dma which calls ep93xx_pata_release_dma():

drivers/ata/pata_ep93xx.c:ep93xx_pata_release_dma() {
	if (drv_data->dma_rx_channel) {
		dma_release_channel(drv_data->dma_rx_channel);
		drv_data->dma_rx_channel = NULL;
	}
	if (drv_data->dma_tx_channel) {
		dma_release_channel(drv_data->dma_tx_channel);
		drv_data->dma_tx_channel = NULL;
	}
}

Will this cause dma_release_channel() to be called a second time on the rx
channel?

Also, since drv_data->dma_tx_channel holds an ERR_PTR rather than NULL,
won't the second if block try to dereference that ERR_PTR when
dma_release_channel() is called on it?

>  		else
>  			ap->udma_mask = ATA_UDMA2;
>  	}

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