Re: [PATCHv3 1/6] ata: pata_mpc52xx: fix NULL pointer dereference in IRQ handler

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Improper resource teardown of the devm-managed BestComm DMA task IRQ, leading to Double Dispose and devres Use-After-Free.
--

commit a85012a47d328b796b5de199b02e054be9b1d391
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);
> @@ -773,7 +775,6 @@ static int mpc52xx_ata_probe(struct platform_device *op)
>  		dev_err(&op->dev, "error requesting DMA IRQ\n");
>  		goto err2;

[Severity: High]
This is a pre-existing issue, but I noticed a potential problem with the
resource teardown when jumping to the err2 label.

When failures occur later in mpc52xx_ata_probe() (like hw_init failing), the
code jumps to the err2 label and manually disposes the IRQ mapping:

drivers/ata/pata_mpc52xx.c:mpc52xx_ata_probe() {
    ...
 err2:
	irq_dispose_mapping(task_irq);
	bcom_ata_release(dmatsk);
    ...
}

Because the IRQ was successfully requested with devm_request_irq() earlier,
does manual disposal destroy the IRQ descriptor before devres runs its own
cleanup, leading to a use-after-free when the IRQ core attempts to free
the action?

Additionally, right after this manual disposal, bcom_ata_release() is called,
which delegates to bcom_task_free(). This function also disposes the mapping
natively:

drivers/dma/bestcomm/bestcomm.c:bcom_task_free() {
    ...
	/* Free everything */
	irq_dispose_mapping(tsk->irq);
	bcom_sram_free(tsk->bd);
    ...
}

Since task_irq and tsk->irq are the same, could this double disposal of the
IRQ mapping corrupt the IRQ domain state and trigger internal kernel warnings?
The same pattern appears to exist in mpc52xx_ata_remove() during module
removal.

>  	}
> -	priv->dmatsk = dmatsk;
>  
>  	/* Init the hw */
>  	rv = mpc52xx_ata_hw_init(priv);

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