Re: [PATCH 1/3] ata: pata_ep93xx: avoid asm on non ARM

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing condition code clobber in inline assembly.
- [High] Double free and invalid pointer dereference on DMA probe error path.
--

commit 161745bc82337864b2d7b61cafb8fabbb474d029
Author: Rosen Penev <rosenp@gmail.com>

ata: pata_ep93xx: avoid asm on non ARM

The raw ARM asm delay loop prevents COMPILE_TEST builds on
non-ARM architectures. Guard it with CONFIG_ARM and provide a
cpu_relax() fallback for compilation on other architectures.

> diff --git a/drivers/ata/pata_ep93xx.c b/drivers/ata/pata_ep93xx.c
> index b2b9e0058333e..f7e87d6af2641 100644
> --- a/drivers/ata/pata_ep93xx.c
> +++ b/drivers/ata/pata_ep93xx.c
> @@ -204,6 +204,7 @@ static void ep93xx_pata_enable_pio(void __iomem *base, int pio_mode)
>   */
>  static void ep93xx_pata_delay(unsigned long count)
>  {
> +#ifdef CONFIG_ARM
>  	__asm__ volatile (
>  		"0:\n"
>  		"mov r0, r0\n"
>  		"subs %0, %1, #1\n"
>  		"bge 0b\n"
>  		: "=r" (count)
>  		: "0" (count)
>  	);

[Severity: High]
This is a pre-existing issue, but since this inline assembly uses the
"subs" instruction which alters the ARM condition code flags, does it
need a "cc" clobber?

Without the "cc" clobber, the compiler might assume that condition flags
set prior to the inline assembly remain valid across it. Because
ep93xx_pata_delay() is a small static function, it's likely to be
inlined into callers like ep93xx_pata_wait_for_iordy(). Could this lead
to unpredictable control flow corruption if the compiler schedules
flag-dependent instructions across the assembly block?

> +#else
> +	while (count--)
> +		cpu_relax();
> +#endif
>  }

[Severity: High]
This is a pre-existing issue, but I noticed a potential double free and
invalid pointer dereference on the DMA probe error path in this driver.

If dma_request_chan(&pdev->dev, "tx") fails during ep93xx_pata_dma_init(),
it returns an ERR_PTR which is stored in drv_data->dma_tx_channel.

The error path jumps to fail_release_rx, which releases dma_rx_channel but
doesn't clear the pointer, and then falls through to ep93xx_pata_release_dma():

drivers/ata/pata_ep93xx.c:ep93xx_pata_dma_init() {
    ...
fail_release_rx:
	dma_release_channel(drv_data->dma_rx_channel);
fail_release_dma:
	ep93xx_pata_release_dma(drv_data);
    ...
}

Inside ep93xx_pata_release_dma(), the code attempts to free both channels:

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;
	}
    ...
}

Since drv_data->dma_rx_channel was never set to NULL, does this cause a
double free of dma_rx_channel?

Additionally, since drv_data->dma_tx_channel holds an ERR_PTR rather than
NULL, this check evaluates to true and dma_release_channel() is called with
the ERR_PTR. Could this cause a kernel panic when the DMA subsystem attempts
to dereference the error pointer?

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