Re: [PATCH] ata: pata_ep93xx: fix double free and ERR_PTR dereference on DMA error path

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The PATA driver fails to initialize if DMA channels are not available, breaking the PIO-only fallback mode.
- [High] Trailing byte transfer logic in PIO data transfer truncates 16-bit values, leading to data corruption on big-endian systems.
--

commit 01fbd237d0698dff8591ae6c458732b482391c35
Author: Rosen Penev <rosenp@gmail.com>

ata: pata_ep93xx: fix double free and ERR_PTR dereference on DMA error path

This commit fixes a double free of the rx channel and an invalid pointer
dereference of the tx channel on the DMA initialization error path. By
NULLing the tx channel before calling the release function, the cleanup
is handled correctly.

> diff --git a/drivers/ata/pata_ep93xx.c b/drivers/ata/pata_ep93xx.c
> index b2b9e0058333e..aa6f8172925d5 100644
> --- a/drivers/ata/pata_ep93xx.c
> +++ b/drivers/ata/pata_ep93xx.c
> @@ -688,7 +688,7 @@ static int ep93xx_pata_dma_init(struct ep93xx_pata_data *drv_data)
>  	return 0;
>  
>  fail_release_rx:
> -	dma_release_channel(drv_data->dma_rx_channel);
> +	drv_data->dma_tx_channel = NULL;

[Severity: High]
This isn't a bug introduced by this patch, but since we are modifying the
error paths in ep93xx_pata_dma_init(), does this initialization failure
unintentionally break the PIO-only fallback?

Looking at the start of ep93xx_pata_dma_init(), if dma_request_chan()
returns -ENODEV because no DMA channels are available in the device tree:

	drv_data->dma_rx_channel = dma_request_chan(dev, "rx");
	if (IS_ERR(drv_data->dma_rx_channel))
		return dev_err_probe(dev, PTR_ERR(drv_data->dma_rx_channel),
				     "rx DMA setup failed\n");

This error propagates directly back to ep93xx_pata_probe():

	err = ep93xx_pata_dma_init(drv_data);
	if (err)
		return err;

Does this abort the probe entirely on systems without DMA, rather than cleanly
dropping DMA support and allowing the driver to fall back to PIO mode?

>  fail_release_dma:
>  	ep93xx_pata_release_dma(drv_data);
>  

[Severity: High]
This is a pre-existing issue, but could the trailing byte transfer logic in
ep93xx_pata_data_xfer() truncate 16-bit values and cause data corruption on
big-endian systems?

In ep93xx_pata_data_xfer(), the pad variable is an array of 2 bytes:

	unsigned char pad[2] = { };

On the READ path, a 16-bit value is assigned directly to *pad:

	if (rw == READ) {
		*pad = cpu_to_le16(
			ep93xx_pata_read_data(
				drv_data, IDECTRL_ADDR_DATA));
		*buf = pad[0];

Doesn't this truncate the 16-bit integer to its lowest 8 bits? On a
little-endian CPU this evaluates to the correct first byte, but on a
big-endian CPU, cpu_to_le16() places the first byte into the upper 8 bits,
which are lost during truncation.

Similarly, on the WRITE path:

	} else {
		pad[0] = *buf;
		ep93xx_pata_write_data(drv_data, le16_to_cpu(*pad),
				  IDECTRL_ADDR_DATA);
	}

Will le16_to_cpu(*pad) zero-extend the 8-bit pad[0] value to a 16-bit
integer, resulting in 0x00BB instead of 0xBB00 before byte swapping,
ultimately writing 0x00 as the first byte of the transfer? Would casting
to a 16-bit pointer better manage memory across endianness boundaries here?

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