[PATCHv2 0/5] ata: pata_mpc52xx: fix cleanup ordering and modernize

[PATCHv2 0/5] ata: pata_mpc52xx: fix cleanup ordering and modernize

[Rosen Penev]

Fix several lifecycle bugs in the MPC52xx ATA driver:

 - The BestComm task IRQ was managed by devm, creating a use-after-free
   and teardown inversion.  Switch to non-devm request_irq/free_irq so
   the interrupt is unregistered before the task is freed.

 - The error and remove paths called irq_dispose_mapping() explicitly,
   duplicating the one in bcom_task_free().  Remove the redundant calls.

 - Replace irq_of_parse_and_map() with platform_get_irq() for proper
   error handling and integration with the platform device model.

 - Replace the open-coded of_address_to_resource()/devm_request_mem_region()
   /devm_ioremap() sequence with the standard helper.

v2: add extra patch from sashiko review.

Rosen Penev (5):
  ata: pata_mpc52xx: fix NULL pointer dereference in IRQ handler
  ata: pata_mpc52xx: switch to non-devm request_irq for proper ordering
  ata: pata_mpc52xx: remove redundant irq_dispose_mapping calls
  ata: pata_mpc52xx: convert to platform_get_irq()
  ata: pata_mpc52xx: use devm_platform_get_and_ioremap_resource

 drivers/ata/pata_mpc52xx.c | 70 +++++++++++++-------------------------
 1 file changed, 23 insertions(+), 47 deletions(-)

--
2.54.0

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

[Rosen Penev]

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().

Assisted-by: opencode:big-pickle
Fixes: 6b61e69e7bc1 ("powerpc/mpc5200: Add MDMA/UDMA support to MPC5200 ATA driver")
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/ata/pata_mpc52xx.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/drivers/ata/pata_mpc52xx.c b/drivers/ata/pata_mpc52xx.c
index 210a63283f62..fe445d6aaff6 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;
 	}
-	priv->dmatsk = dmatsk;
 
 	/* Init the hw */
 	rv = mpc52xx_ata_hw_init(priv);
-- 
2.54.0

[PATCHv2 2/5] ata: pata_mpc52xx: switch to non-devm request_irq for proper ordering

[Rosen Penev]

Use non-devm request_irq/free_irq to ensure proper ordering in
error/remove paths: free_irq() before bcom_ata_release(), preventing
a use-after-free where the IRQ handler accesses freed BestComm
resources.

Assisted-by: opencode:big-pickle
Fixes: d01159dffa15 ("drivers/ata/pata_mpc52xx.c: clean up error handling code")
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/ata/pata_mpc52xx.c | 15 +++++++++------
 1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/drivers/ata/pata_mpc52xx.c b/drivers/ata/pata_mpc52xx.c
index fe445d6aaff6..b46b83df03d1 100644
--- a/drivers/ata/pata_mpc52xx.c
+++ b/drivers/ata/pata_mpc52xx.c
@@ -769,18 +769,18 @@ static int mpc52xx_ata_probe(struct platform_device *op)
 	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);
+	rv = request_irq(task_irq, &mpc52xx_ata_task_irq, 0,
+			  "ATA task", priv);
 	if (rv) {
 		dev_err(&op->dev, "error requesting DMA IRQ\n");
-		goto err2;
+		goto err_free_task;
 	}
 
 	/* Init the hw */
 	rv = mpc52xx_ata_hw_init(priv);
 	if (rv) {
 		dev_err(&op->dev, "error initializing hardware\n");
-		goto err2;
+		goto err_free_irq;
 	}
 
 	/* Register ourselves to libata */
@@ -788,13 +788,15 @@ static int mpc52xx_ata_probe(struct platform_device *op)
 				  mwdma_mask, udma_mask);
 	if (rv) {
 		dev_err(&op->dev, "error registering with ATA layer\n");
-		goto err2;
+		goto err_free_irq;
 	}
 
 	return 0;
 
- err2:
+ err_free_irq:
+	free_irq(task_irq, priv);
 	irq_dispose_mapping(task_irq);
+ err_free_task:
 	bcom_ata_release(dmatsk);
  err1:
 	irq_dispose_mapping(ata_irq);
@@ -812,6 +814,7 @@ static void mpc52xx_ata_remove(struct platform_device *op)
 
 	/* Clean up DMA */
 	task_irq = bcom_get_task_irq(priv->dmatsk);
+	free_irq(task_irq, priv);
 	irq_dispose_mapping(task_irq);
 	bcom_ata_release(priv->dmatsk);
 	irq_dispose_mapping(priv->ata_irq);
-- 
2.54.0

[PATCHv2 3/5] ata: pata_mpc52xx: remove redundant irq_dispose_mapping calls

[Rosen Penev]

bcom_ata_release() calls bcom_task_free(), which already calls
irq_dispose_mapping(tsk->irq).  The explicit irq_dispose_mapping()
calls in the probe error path and remove function are redundant
and cause a double-free of the same mapping.

Remove them.

Assisted-by: opencode:big-pickle
Fixes: d01159dffa15 ("drivers/ata/pata_mpc52xx.c: clean up error handling code")
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/ata/pata_mpc52xx.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/ata/pata_mpc52xx.c b/drivers/ata/pata_mpc52xx.c
index b46b83df03d1..d13cfaf285dc 100644
--- a/drivers/ata/pata_mpc52xx.c
+++ b/drivers/ata/pata_mpc52xx.c
@@ -795,7 +795,6 @@ static int mpc52xx_ata_probe(struct platform_device *op)
 
  err_free_irq:
 	free_irq(task_irq, priv);
-	irq_dispose_mapping(task_irq);
  err_free_task:
 	bcom_ata_release(dmatsk);
  err1:
@@ -815,7 +814,6 @@ static void mpc52xx_ata_remove(struct platform_device *op)
 	/* Clean up DMA */
 	task_irq = bcom_get_task_irq(priv->dmatsk);
 	free_irq(task_irq, priv);
-	irq_dispose_mapping(task_irq);
 	bcom_ata_release(priv->dmatsk);
 	irq_dispose_mapping(priv->ata_irq);
 }
-- 
2.54.0

[PATCHv2 4/5] ata: pata_mpc52xx: convert to platform_get_irq()

[Rosen Penev]

Replace irq_of_parse_and_map() with platform_get_irq(), which returns
a negative errno on failure and integrates with the platform device
model. Since platform_get_irq() does not create a separately managed
mapping, the corresponding irq_dispose_mapping() calls in the probe
error path and remove function are no longer needed.

Remove most gotos from probe. Simpler to return directly.

Assisted-by: opencode:big-pickle
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/ata/pata_mpc52xx.c | 21 ++++++---------------
 1 file changed, 6 insertions(+), 15 deletions(-)

diff --git a/drivers/ata/pata_mpc52xx.c b/drivers/ata/pata_mpc52xx.c
index d13cfaf285dc..84a4cf42ef54 100644
--- a/drivers/ata/pata_mpc52xx.c
+++ b/drivers/ata/pata_mpc52xx.c
@@ -21,7 +21,6 @@
 #include <linux/libata.h>
 #include <linux/of.h>
 #include <linux/of_address.h>
-#include <linux/of_irq.h>
 #include <linux/platform_device.h>
 #include <linux/types.h>
 
@@ -730,18 +729,14 @@ static int mpc52xx_ata_probe(struct platform_device *op)
 	if ((prop) && (proplen >= 4))
 		udma_mask = ATA_UDMA2 & ((1 << (*prop + 1)) - 1);
 
-	ata_irq = irq_of_parse_and_map(op->dev.of_node, 0);
-	if (!ata_irq) {
-		dev_err(&op->dev, "error mapping irq\n");
-		return -EINVAL;
-	}
+	ata_irq = platform_get_irq(op, 0);
+	if (ata_irq < 0)
+		return ata_irq;
 
 	/* Prepare our private structure */
 	priv = devm_kzalloc(&op->dev, sizeof(*priv), GFP_KERNEL);
-	if (!priv) {
-		rv = -ENOMEM;
-		goto err1;
-	}
+	if (!priv)
+		return -ENOMEM;
 
 	priv->ipb_period = 1000000000 / (ipb_freq / 1000);
 	priv->ata_regs = ata_regs;
@@ -762,8 +757,7 @@ static int mpc52xx_ata_probe(struct platform_device *op)
 	dmatsk = bcom_ata_init(MAX_DMA_BUFFERS, MAX_DMA_BUFFER_SIZE);
 	if (!dmatsk) {
 		dev_err(&op->dev, "bestcomm initialization failed\n");
-		rv = -ENOMEM;
-		goto err1;
+		return -ENOMEM;
 	}
 
 	priv->dmatsk = dmatsk;
@@ -797,8 +791,6 @@ static int mpc52xx_ata_probe(struct platform_device *op)
 	free_irq(task_irq, priv);
  err_free_task:
 	bcom_ata_release(dmatsk);
- err1:
-	irq_dispose_mapping(ata_irq);
 	return rv;
 }
 
@@ -815,7 +807,6 @@ static void mpc52xx_ata_remove(struct platform_device *op)
 	task_irq = bcom_get_task_irq(priv->dmatsk);
 	free_irq(task_irq, priv);
 	bcom_ata_release(priv->dmatsk);
-	irq_dispose_mapping(priv->ata_irq);
 }
 
 #ifdef CONFIG_PM_SLEEP
-- 
2.54.0

[PATCHv2 5/5] ata: pata_mpc52xx: use devm_platform_get_and_ioremap_resource

[Rosen Penev]

Replace the open-coded sequence of of_address_to_resource(),
devm_request_mem_region(), and devm_ioremap() with the standard
devm_platform_get_and_ioremap_resource() helper.

Assisted-by: opencode:big-pickle
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/ata/pata_mpc52xx.c | 29 ++++++-----------------------
 1 file changed, 6 insertions(+), 23 deletions(-)

diff --git a/drivers/ata/pata_mpc52xx.c b/drivers/ata/pata_mpc52xx.c
index 84a4cf42ef54..d3ee117b8aa0 100644
--- a/drivers/ata/pata_mpc52xx.c
+++ b/drivers/ata/pata_mpc52xx.c
@@ -20,7 +20,6 @@
 #include <linux/delay.h>
 #include <linux/libata.h>
 #include <linux/of.h>
-#include <linux/of_address.h>
 #include <linux/platform_device.h>
 #include <linux/types.h>
 
@@ -671,7 +670,7 @@ static int mpc52xx_ata_init_one(struct device *dev,
 static int mpc52xx_ata_probe(struct platform_device *op)
 {
 	unsigned int ipb_freq;
-	struct resource res_mem;
+	struct resource *res_mem;
 	int ata_irq = 0;
 	struct mpc52xx_ata __iomem *ata_regs;
 	struct mpc52xx_ata_priv *priv = NULL;
@@ -688,25 +687,9 @@ static int mpc52xx_ata_probe(struct platform_device *op)
 		return -ENODEV;
 	}
 
-	/* Get device base address from device tree, request the region
-	 * and ioremap it. */
-	rv = of_address_to_resource(op->dev.of_node, 0, &res_mem);
-	if (rv) {
-		dev_err(&op->dev, "could not determine device base address\n");
-		return rv;
-	}
-
-	if (!devm_request_mem_region(&op->dev, res_mem.start,
-				     sizeof(*ata_regs), DRV_NAME)) {
-		dev_err(&op->dev, "error requesting register region\n");
-		return -EBUSY;
-	}
-
-	ata_regs = devm_ioremap(&op->dev, res_mem.start, sizeof(*ata_regs));
-	if (!ata_regs) {
-		dev_err(&op->dev, "error mapping device registers\n");
-		return -ENOMEM;
-	}
+	ata_regs = devm_platform_get_and_ioremap_resource(op, 0, &res_mem);
+	if (IS_ERR(ata_regs))
+		return PTR_ERR(ata_regs);
 
 	/*
 	 * By default, all DMA modes are disabled for the MPC5200.  Some
@@ -740,7 +723,7 @@ static int mpc52xx_ata_probe(struct platform_device *op)
 
 	priv->ipb_period = 1000000000 / (ipb_freq / 1000);
 	priv->ata_regs = ata_regs;
-	priv->ata_regs_pa = res_mem.start;
+	priv->ata_regs_pa = res_mem->start;
 	priv->ata_irq = ata_irq;
 	priv->csel = -1;
 	priv->mpc52xx_ata_dma_last_write = -1;
@@ -778,7 +761,7 @@ static int mpc52xx_ata_probe(struct platform_device *op)
 	}
 
 	/* Register ourselves to libata */
-	rv = mpc52xx_ata_init_one(&op->dev, priv, res_mem.start,
+	rv = mpc52xx_ata_init_one(&op->dev, priv, res_mem->start,
 				  mwdma_mask, udma_mask);
 	if (rv) {
 		dev_err(&op->dev, "error registering with ATA layer\n");
-- 
2.54.0

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

[sashiko-bot]

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