Re: [PATCHv2 1/5] ata: pata_mpc52xx: fix NULL pointer dereference in IRQ handler

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Double free of the `task_irq` mapping due to redundant `irq_dispose_mapping()` calls during error handling and removal.
- [High] Premature destruction of devres-managed IRQ mappings (`task_irq` and `ata_irq`), causing Use-After-Free of `irq_desc` and memory leaks of `struct irqaction`.
- [High] The `dmatsk` structure is freed during driver removal or probe failure while the devres-managed `task_irq` handler is still active, causing a potential Use-After-Free.
- [High] Data race between the BestComm IRQ handler (`mpc52xx_ata_task_irq`) and `mpc52xx_bmdma_stop`.
- [High] The DMA FIFO configuration state is not reset upon system resume, causing potential DMA transfer failures.
--

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

ata: pata_mpc52xx: fix NULL pointer dereference in IRQ handler

Assign priv->dmatsk before request_irq() to ensure the IRQ handler
does not dereference a NULL pointer. The handler accesses priv->dmatsk
via bcom_retrieve_buffer().

> diff --git a/drivers/ata/pata_mpc52xx.c b/drivers/ata/pata_mpc52xx.c
> index 210a63283f62b..fe445d6aaff62 100644
> --- a/drivers/ata/pata_mpc52xx.c
> +++ b/drivers/ata/pata_mpc52xx.c
> @@ -766,6 +766,8 @@ static int mpc52xx_ata_probe(struct platform_device *op)
>  		goto err1;
>  	}
>  
> +	priv->dmatsk = dmatsk;
> +
>  	task_irq = bcom_get_task_irq(dmatsk);
>  	rv = devm_request_irq(&op->dev, task_irq, &mpc52xx_ata_task_irq, 0,
>  				"ATA task", priv);

[Severity: High]
This isn't a bug introduced by this patch, but looking at the error path
for devm_request_irq(), does the cleanup order create a use-after-free?

If the probe fails and jumps to err2, or during driver removal in
mpc52xx_ata_remove(), bcom_ata_release(dmatsk) is called while the IRQ
is still managed by devres.

Because devm_request_irq() won't unregister the handler until after probe
returns or the device is removed, could a spurious interrupt trigger during
this window and cause mpc52xx_ata_task_irq() to dereference the freed
priv->dmatsk pointer?

[Severity: High]
This is a pre-existing issue, but does manually disposing of devm-managed
IRQ mappings cause a use-after-free of the IRQ descriptor?

Both task_irq and ata_irq are requested using devm_request_irq().
However, the driver explicitly calls irq_dispose_mapping() on both IRQs
during err2, err1, and in mpc52xx_ata_remove().

Because irq_dispose_mapping() destroys the underlying irq_desc while the
devres-managed handler is still active, could this lead to a use-after-free
when devres eventually executes free_irq()?

[Severity: High]
This isn't a bug introduced by this patch, but does the cleanup code cause
a double free of the task_irq mapping?

In both the err2 path and mpc52xx_ata_remove(), the driver does:

	irq_dispose_mapping(task_irq);
	bcom_ata_release(dmatsk);

Since bcom_ata_release() calls bcom_task_free(), which also executes
irq_dispose_mapping(tsk->irq) for the same IRQ, doesn't this result in
the irqdomain API being called twice for the same mapping?

[Severity: High]
This is a pre-existing issue, but is there a data race between the DMA
stop function and the IRQ handler?

The mpc52xx_ata_task_irq() handler accesses the task descriptors without
locks and performs a read-modify-write on priv->waiting_for_dma:

mpc52xx_ata_task_irq() {
    ...
	while (bcom_buffer_done(priv->dmatsk))
		bcom_retrieve_buffer(priv->dmatsk, NULL, NULL);

	priv->waiting_for_dma |= ATA_DMA_INTR;
    ...
}

Concurrently, mpc52xx_bmdma_stop() can reset the DMA buffers and zero
out priv->waiting_for_dma:

mpc52xx_bmdma_stop() {
    ...
	bcom_disable(priv->dmatsk);
	bcom_ata_reset_bd(priv->dmatsk);
	priv->waiting_for_dma = 0;
    ...
}

If these run concurrently without synchronization, could they corrupt the
DMA descriptor state or lose updates to waiting_for_dma?

[Severity: High]
This is a pre-existing issue, but does the resume path properly reset the
DMA FIFO direction cache?

During normal operation, mpc52xx_bmdma_setup() caches the transfer direction
to avoid unnecessary register writes:

mpc52xx_bmdma_setup() {
    ...
		/* Setup FIFO if direction changed */
		if (priv->mpc52xx_ata_dma_last_write != 0) {
    ...
}

When the system suspends, the hardware loses this FIFO state. On resume,
mpc52xx_ata_resume() calls mpc52xx_ata_hw_init() to reconfigure the
hardware, but it does not invalidate priv->mpc52xx_ata_dma_last_write
(e.g., setting it to -1).

If the first DMA transfer after resume matches the pre-suspend direction,
could this skip writing the necessary FIFO configuration registers and
break ATA I/O?

> @@ -773,7 +775,6 @@ static int mpc52xx_ata_probe(struct platform_device *op)
>  		dev_err(&op->dev, "error requesting DMA IRQ\n");
>  		goto err2;
>  	}
> -	priv->dmatsk = dmatsk;
>  
>  	/* Init the hw */
>  	rv = mpc52xx_ata_hw_init(priv);

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