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

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

[Rosen Penev]

If dma_request_chan(&pdev->dev, "tx") fails during ep93xx_pata_dma_init(),
drv_data->dma_tx_channel holds an ERR_PTR. The error path at
fail_release_rx releases dma_rx_channel but does not NULL the pointer,
then falls through to ep93xx_pata_release_dma() which attempts to release
dma_rx_channel again (double free) and calls dma_release_channel() on the
ERR_PTR dma_tx_channel (invalid pointer dereference).

Fix by NULLing dma_tx_channel before falling through to
ep93xx_pata_release_dma(), which will then clean up dma_rx_channel.

Fixes: 9963113e3a92 ("ata: pata_ep93xx: add device tree support")
Assisted-by: Opencode:Big-Pickle
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/ata/pata_ep93xx.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/ata/pata_ep93xx.c b/drivers/ata/pata_ep93xx.c
index b2b9e0058333..aa6f8172925d 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;
 fail_release_dma:
 	ep93xx_pata_release_dma(drv_data);
 
-- 
2.54.0

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

[sashiko-bot]

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

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

[Rosen Penev]

On Mon, Jun 1, 2026 at 11:44 AM <sashiko-bot@kernel.org> wrote:
>
> 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.
Yep. Makes this patch completely pointless.
> - [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