[PATCH net v2 1/2] net: fman: move IRQ registration after init to prevent NULL deref and UAF

[PATCH net v2 1/2] net: fman: move IRQ registration after init to prevent NULL deref and UAF

[ZhaoJinming]

read_dts_node() registers shared interrupt handlers via
devm_request_irq() with fman as dev_id. Two bugs exist in the
current code:

1) Pre-init NULL dereference: at registration time fman is only
partially initialized -- kzalloc_obj() zero-initializes all fields,
so fman->cfg and fman->fpm_regs are NULL. The handlers check
is_init_done(fman->cfg) to guard against incomplete init, but
is_init_done(NULL) returns true (intended to mean cfg was freed
after successful init), so the guard is bypassed and fpm_regs is
dereferenced. If another device on the same shared IRQ line fires
during the window between devm_request_irq() and fman_init(), the
handler accesses NULL fpm_regs via ioread32be(), causing a crash.

2) Use-after-free on probe failure: fman is allocated with
kzalloc_obj() (not devm), so on error paths in read_dts_node()
(ioremap failure, of_platform_populate failure) and fman_config(),
kfree(fman) is called while the devm IRQ handlers remain
registered. The driver core's subsequent devres_release_all() frees
the IRQ handlers, but during the window between kfree(fman) and
devm_free_irq(), a shared-IRQ spurious firing will dereference
the already-freed fman.

A previous attempt to fix issue #1 with an irq_ready flag protected
by READ_ONCE()/WRITE_ONCE() is insufficient on weakly-ordered
architectures. READ_ONCE()/WRITE_ONCE() only prevent compiler
optimization; they do not provide the memory ordering guarantees
(e.g., smp_store_release/smp_load_acquire) needed to ensure that
writes to register pointers are visible to the IRQ handler before
it observes the flag as true.

Fix both issues by moving devm_request_irq() out of read_dts_node()
and into fman_probe(), after both fman_config() and fman_init()
have completed. This eliminates both race windows: by the time the
handlers are registered, all register pointers are initialized
(preventing the NULL dereference), and since fman is never freed
after this point, the use-after-free cannot occur either.

Add an 'irq' field to struct fman_dts_params so that the primary IRQ
number parsed in read_dts_node() is available to fman_probe().

This replaces the previous approach (irq_ready flag with READ_ONCE)
and also supersedes the separate UAF fix patch ("fsl_fman: fix
use-after-free on IRQF_SHARED handler after probe failure" v3),
as moving IRQ registration after init resolves both issues
in a single change.

v2:
- move devm_request_irq() to fman_probe() after init (replaces
  irq_ready + READ_ONCE approach from v1)
- supersede the separate UAF fix patch (v3), as this patch
  resolves both issues in a single change

Fixes: 414fd46e7762 ("fsl/fman: Add FMan support")
Link: https://lore.kernel.org/netdev/20260626162323.GE1310988@horms.kernel.org/
Signed-off-by: ZhaoJinming <zhaojinming@uniontech.com>
---
 drivers/net/ethernet/freescale/fman/fman.c | 52 +++++++++++++---------
 drivers/net/ethernet/freescale/fman/fman.h |  1 +
 2 files changed, 32 insertions(+), 21 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fman/fman.c b/drivers/net/ethernet/freescale/fman/fman.c
index 299bab043175..6947f3bc7c87 100644
--- a/drivers/net/ethernet/freescale/fman/fman.c
+++ b/drivers/net/ethernet/freescale/fman/fman.c
@@ -2695,7 +2695,7 @@ static struct fman *read_dts_node(struct platform_device *of_dev)
 	void __iomem *base_addr;
 	struct resource *res;
 	u32 val, range[2];
-	int err, irq;
+	int err;
 	struct clk *clk;
 	u32 clk_rate;
 
@@ -2717,7 +2717,7 @@ static struct fman *read_dts_node(struct platform_device *of_dev)
 	err = platform_get_irq(of_dev, 0);
 	if (err < 0)
 		goto fman_node_put;
-	irq = err;
+	fman->dts_params.irq = err;
 
 	/* Get the FM error interrupt */
 	err = platform_get_irq(of_dev, 1);
@@ -2773,25 +2773,6 @@ static struct fman *read_dts_node(struct platform_device *of_dev)
 
 	of_node_put(muram_node);
 
-	err = devm_request_irq(&of_dev->dev, irq, fman_irq, IRQF_SHARED,
-			       "fman", fman);
-	if (err < 0) {
-		dev_err(&of_dev->dev, "%s: irq %d allocation failed (error = %d)\n",
-			__func__, irq, err);
-		goto fman_free;
-	}
-
-	if (fman->dts_params.err_irq != 0) {
-		err = devm_request_irq(&of_dev->dev, fman->dts_params.err_irq,
-				       fman_err_irq, IRQF_SHARED,
-				       "fman-err", fman);
-		if (err < 0) {
-			dev_err(&of_dev->dev, "%s: irq %d allocation failed (error = %d)\n",
-				__func__, fman->dts_params.err_irq, err);
-			goto fman_free;
-		}
-	}
-
 	base_addr = devm_platform_get_and_ioremap_resource(of_dev, 0, &res);
 	if (IS_ERR(base_addr)) {
 		err = PTR_ERR(base_addr);
@@ -2848,6 +2829,35 @@ static int fman_probe(struct platform_device *of_dev)
 		return -EINVAL;
 	}
 
+	/* Register IRQ handlers only after initialization is complete.
+	 * This prevents two issues:
+	 * 1) Pre-init NULL dereference: is_init_done(NULL) returns true,
+	 *    so a shared-IRQ spurious firing before fpm_regs is set would
+	 *    dereference NULL.
+	 * 2) Use-after-free on probe failure: fman was kzalloc'd (not devm),
+	 *    so on error paths kfree(fman) ran before devm_free_irq, leaving
+	 *    a window where the handler could fire with a freed dev_id.
+	 * By registering here, both problems are eliminated.
+	 */
+	err = devm_request_irq(dev, fman->dts_params.irq, fman_irq,
+			       IRQF_SHARED, "fman", fman);
+	if (err < 0) {
+		dev_err(dev, "%s: irq %d allocation failed (error = %d)\n",
+			__func__, fman->dts_params.irq, err);
+		return err;
+	}
+
+	if (fman->dts_params.err_irq != 0) {
+		err = devm_request_irq(dev, fman->dts_params.err_irq,
+				       fman_err_irq, IRQF_SHARED,
+				       "fman-err", fman);
+		if (err < 0) {
+			dev_err(dev, "%s: irq %d allocation failed (error = %d)\n",
+				__func__, fman->dts_params.err_irq, err);
+			return err;
+		}
+	}
+
 	if (fman->dts_params.err_irq == 0) {
 		fman_set_exception(fman, FMAN_EX_DMA_BUS_ERROR, false);
 		fman_set_exception(fman, FMAN_EX_DMA_READ_ECC, false);
diff --git a/drivers/net/ethernet/freescale/fman/fman.h b/drivers/net/ethernet/freescale/fman/fman.h
index 74eb62eba0d7..630d57c3144c 100644
--- a/drivers/net/ethernet/freescale/fman/fman.h
+++ b/drivers/net/ethernet/freescale/fman/fman.h
@@ -286,6 +286,7 @@ struct fman_dts_params {
 	struct resource *res;                   /* FMan memory resource */
 	u8 id;                                  /* FMan ID */
 
+	int irq;                                /* FMan IRQ */
 	int err_irq;                            /* FMan Error IRQ */
 
 	u16 clk_freq;                           /* FMan clock freq (In Mhz) */
-- 
2.20.1

[PATCH net v2 2/2] net: fman: add error cleanup path in fman_probe

[ZhaoJinming]

fman_init() and devm_request_irq() failure paths in fman_probe()
do not free fman and its sub-resources (keygen, muram allocations,
state, cfg), causing memory leaks on probe failure.

Add fman_muram_finish() to properly tear down a MURAM partition
(gen_pool_destroy + iounmap + kfree), complementing the existing
fman_muram_init().

Add fman_free_resources() that releases all fman sub-resources
in the correct order:
- devm_free_irq() for any already-registered IRQ handlers
- kfree(fman->keygen)
- free_init_resources() for MURAM CAM/FIFO allocations
- kfree(fman->cfg)
- fman_muram_finish(fman->muram) for the MURAM management object
- kfree(fman->state)
- kfree(fman)

Use two goto labels in fman_probe():
- err_irq: main IRQ registered but err_irq failed -- free main IRQ
  then fall through to release resources
- err_no_irq: no IRQ registered -- just release resources

The IRQ handlers must be explicitly freed before kfree(fman) to
avoid a window where a shared-IRQ spurious firing could dereference
the freed dev_id.

Note: fman_config() is not changed -- it already frees fman
internally on all its error paths, so fman_probe() must not touch
fman after fman_config() fails.

v2:
- add explicit devm_free_irq() before kfree(fman) to eliminate
  a potential UAF window on the cleanup path
- add fman_muram_finish() for complete MURAM teardown
- add kfree(fman->cfg) to release config structure

Fixes: 414fd46e7762 ("fsl/fman: Add FMan support")
Signed-off-by: ZhaoJinming <zhaojinming@uniontech.com>
---
 drivers/net/ethernet/freescale/fman/fman.c    | 31 +++++++++++++++++--
 .../net/ethernet/freescale/fman/fman_muram.c  | 15 +++++++++
 .../net/ethernet/freescale/fman/fman_muram.h  |  2 ++
 3 files changed, 45 insertions(+), 3 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fman/fman.c b/drivers/net/ethernet/freescale/fman/fman.c
index 6947f3bc7c87..752c0df0e17c 100644
--- a/drivers/net/ethernet/freescale/fman/fman.c
+++ b/drivers/net/ethernet/freescale/fman/fman.c
@@ -2806,6 +2806,24 @@ static struct fman *read_dts_node(struct platform_device *of_dev)
 	return ERR_PTR(err);
 }
 
+static void fman_free_resources(struct fman *fman, struct device *dev,
+				bool irq_registered)
+{
+	/* Free IRQs first while fman is still valid */
+	if (irq_registered) {
+		if (fman->dts_params.err_irq != 0)
+			devm_free_irq(dev, fman->dts_params.err_irq, fman);
+		devm_free_irq(dev, fman->dts_params.irq, fman);
+	}
+
+	kfree(fman->keygen);
+	free_init_resources(fman);
+	kfree(fman->cfg);
+	fman_muram_finish(fman->muram);
+	kfree(fman->state);
+	kfree(fman);
+}
+
 static int fman_probe(struct platform_device *of_dev)
 {
 	struct fman *fman;
@@ -2821,12 +2839,13 @@ static int fman_probe(struct platform_device *of_dev)
 	err = fman_config(fman);
 	if (err) {
 		dev_err(dev, "%s: FMan config failed\n", __func__);
+		/* fman_config() frees fman internally on all error paths */
 		return -EINVAL;
 	}
 
 	if (fman_init(fman) != 0) {
 		dev_err(dev, "%s: FMan init failed\n", __func__);
-		return -EINVAL;
+		goto err_no_irq;
 	}
 
 	/* Register IRQ handlers only after initialization is complete.
@@ -2844,7 +2863,7 @@ static int fman_probe(struct platform_device *of_dev)
 	if (err < 0) {
 		dev_err(dev, "%s: irq %d allocation failed (error = %d)\n",
 			__func__, fman->dts_params.irq, err);
-		return err;
+		goto err_no_irq;
 	}
 
 	if (fman->dts_params.err_irq != 0) {
@@ -2854,7 +2873,7 @@ static int fman_probe(struct platform_device *of_dev)
 		if (err < 0) {
 			dev_err(dev, "%s: irq %d allocation failed (error = %d)\n",
 				__func__, fman->dts_params.err_irq, err);
-			return err;
+			goto err_irq;
 		}
 	}
 
@@ -2883,6 +2902,12 @@ static int fman_probe(struct platform_device *of_dev)
 	dev_dbg(dev, "FMan%d probed\n", fman->dts_params.id);
 
 	return 0;
+
+err_irq:
+	devm_free_irq(dev, fman->dts_params.irq, fman);
+err_no_irq:
+	fman_free_resources(fman, dev, false);
+	return err ?: -EINVAL;
 }
 
 static const struct of_device_id fman_match[] = {
diff --git a/drivers/net/ethernet/freescale/fman/fman_muram.c b/drivers/net/ethernet/freescale/fman/fman_muram.c
index 6ac7c2b0cb19..6c2b4f7a02b8 100644
--- a/drivers/net/ethernet/freescale/fman/fman_muram.c
+++ b/drivers/net/ethernet/freescale/fman/fman_muram.c
@@ -129,3 +129,18 @@ void fman_muram_free_mem(struct muram_info *muram, unsigned long offset,
 
 	gen_pool_free(muram->pool, addr, size);
 }
+
+/**
+ * fman_muram_finish
+ * @muram:	FM-MURAM module pointer.
+ *
+ * Frees all resources associated with a MURAM partition.
+ */
+void fman_muram_finish(struct muram_info *muram)
+{
+	if (!muram)
+		return;
+	iounmap(muram->vbase);
+	gen_pool_destroy(muram->pool);
+	kfree(muram);
+}
diff --git a/drivers/net/ethernet/freescale/fman/fman_muram.h b/drivers/net/ethernet/freescale/fman/fman_muram.h
index 3643af61bae2..a5cb544c0f08 100644
--- a/drivers/net/ethernet/freescale/fman/fman_muram.h
+++ b/drivers/net/ethernet/freescale/fman/fman_muram.h
@@ -23,4 +23,6 @@ unsigned long fman_muram_alloc(struct muram_info *muram, size_t size);
 void fman_muram_free_mem(struct muram_info *muram, unsigned long offset,
 			 size_t size);
 
+void fman_muram_finish(struct muram_info *muram);
+
 #endif /* __FM_MURAM_EXT */
-- 
2.20.1

Re: [PATCH net v2 1/2] net: fman: move IRQ registration after init to prevent NULL deref and UAF

[Simon Horman]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev

Simon says:

I think we can treat the pre-existing issue flagged below in the context of
possible follow-up. But I'm wondering if you could take a look over
the first issue.

---
net: fman: move IRQ registration after init to prevent NULL deref and UAF

This commit moves IRQ registration in the fman driver from read_dts_node() to
fman_probe() after fman_init() has completed. The intention is to avoid a NULL
dereference or use-after-free caused by shared interrupts firing before the
driver state is fully initialized.

Link: https://lore.kernel.org/netdev/20260626162323.GE1310988@horms.kernel.org/

> diff --git a/drivers/net/ethernet/freescale/fman/fman.c b/drivers/net/ethernet/freescale/fman/fman.c
> index 299bab043175b..6947f3bc7c87e 100644
> --- a/drivers/net/ethernet/freescale/fman/fman.c
> +++ b/drivers/net/ethernet/freescale/fman/fman.c

[ ... ]

> @@ -2848,6 +2829,35 @@ static int fman_probe(struct platform_device *of_dev)
>  		return -EINVAL;
>  	}
>  
> +	/* Register IRQ handlers only after initialization is complete.
> +	 * This prevents two issues:
> +	 * 1) Pre-init NULL dereference: is_init_done(NULL) returns true,
> +	 *    so a shared-IRQ spurious firing before fpm_regs is set would
> +	 *    dereference NULL.
> +	 * 2) Use-after-free on probe failure: fman was kzalloc'd (not devm),
> +	 *    so on error paths kfree(fman) ran before devm_free_irq, leaving
> +	 *    a window where the handler could fire with a freed dev_id.
> +	 * By registering here, both problems are eliminated.
> +	 */
> +	err = devm_request_irq(dev, fman->dts_params.irq, fman_irq,
> +			       IRQF_SHARED, "fman", fman);

Simon says:

It seems to me that one possible way to address the raced flagged
below would be to register the handler earlier, but have a flag
so that that the handler is effectively an noop until initialisation
is complete.

[Severity: High]
Can this race condition lead to an unhandled interrupt storm and disable the
shared IRQ line?

By moving the IRQ registration here, the handler is registered after
fman_init() has completed. The fman_init() function configures the hardware
and enables its interrupts (e.g. via fpm_init() and enable()). Because the
interrupt is shared (IRQF_SHARED), the IRQ line might already be unmasked by
another device.

If the FMan hardware asserts an interrupt in the window between fman_init()
completing and this handler being registered, the shared IRQ will fire. The
other device's handler will return IRQ_NONE, and since the FMan handler is
absent, no one will clear the FMan interrupt.

This interrupt storm would cause the core IRQ subsystem to permanently
disable the shared IRQ line.

[Severity: High]
This is a pre-existing issue, but does the driver leak dynamically allocated
resources when unbound?

The FMan driver allocates several non-devm-managed resources (e.g., fman
via kzalloc_obj, fman->state, fman->cfg, and MURAM allocations). However,
the platform_driver structure completely lacks a .remove callback.

If the device is unbound via sysfs or the module is unloaded, the core
driver subsystem tears down devm resources like IRQs, but the dynamically
allocated FMan structures and MURAM pools appear to be permanently leaked.

[Severity: High]
This is a pre-existing issue, but can child devices (MACs) probe before the
FMan parent initialization is complete, leading to permanent probe failures?

In fman_probe(), read_dts_node() is called first, which calls
of_platform_populate() to create child MAC devices before the FMan parent
has finished initializing:

fman_probe() {
    fman = read_dts_node(of_dev); /* Calls of_platform_populate() */
    ...
    err = fman_config(fman);
    ...
    if (fman_init(fman) != 0) {
    ...
    dev_set_drvdata(dev, fman);
}

If the MAC driver is already registered when of_platform_populate() is
called, the MAC devices will probe synchronously. Their probe routine
calls fman_bind(), which reads the parent's driver data via
dev_get_drvdata().

Since dev_set_drvdata() is not called until the very end of fman_probe(),
fman_bind() would return NULL, causing the MAC probe to fail with -ENODEV.

> +	if (err < 0) {
> +		dev_err(dev, "%s: irq %d allocation failed (error = %d)\n",
> +			__func__, fman->dts_params.irq, err);
> +		return err;
> +	}

Re: [PATCH net v2 2/2] net: fman: add error cleanup path in fman_probe

[Simon Horman]

This is an AI-generated review of your patch. The human sending this
email has considered the AI review valid, or at least plausible.
Full review at: https://sashiko.dev
---
net: fman: add error cleanup path in fman_probe

This commit introduces a new error cleanup path in fman_probe() to prevent
memory leaks when fman_init() or devm_request_irq() fail. It adds
fman_muram_finish() and fman_free_resources() to properly tear down
allocations and states.

> diff --git a/drivers/net/ethernet/freescale/fman/fman.c b/drivers/net/ethernet/freescale/fman/fman.c
> index 6947f3bc7c87e..752c0df0e17c5 100644
> --- a/drivers/net/ethernet/freescale/fman/fman.c
> +++ b/drivers/net/ethernet/freescale/fman/fman.c

[ ... ]

> +static void fman_free_resources(struct fman *fman, struct device *dev,
> +				bool irq_registered)
> +{
> +	/* Free IRQs first while fman is still valid */
> +	if (irq_registered) {
> +		if (fman->dts_params.err_irq != 0)
> +			devm_free_irq(dev, fman->dts_params.err_irq, fman);
> +		devm_free_irq(dev, fman->dts_params.irq, fman);
> +	}
> +
> +	kfree(fman->keygen);
> +	free_init_resources(fman);

[Severity: High]
If fman_init() fails internally (for example, in dma_init() or
keygen_init()), it calls free_init_resources(fman) on its own failure path.
However, it does not clear fman->fifo_offset or fman->cam_offset.

When fman_probe() catches this error, it jumps to err_no_irq and calls
fman_free_resources(), which will invoke free_init_resources(fman) a second
time.

Does this cause a double-free of the MURAM resources?

[Severity: High]
This is a pre-existing issue, but if fman_muram_alloc() fails during
fman_init(), it returns an error code (such as -ENOMEM) which is stored
directly in fman->fifo_offset.

Since the error code is non-zero, free_init_resources() will evaluate
if (fman->fifo_offset) as true and pass the error pointer to
fman_muram_free_mem().

Does attempting to free this pseudo-random error code cause corruption in
the gen_pool allocator?

> +	kfree(fman->cfg);
> +	fman_muram_finish(fman->muram);
> +	kfree(fman->state);
> +	kfree(fman);
> +}

[ ... ]

> @@ -2844,7 +2863,7 @@ static int fman_probe(struct platform_device *of_dev)
>  	if (err < 0) {
>  		dev_err(dev, "%s: irq %d allocation failed (error = %d)\n",
>  			__func__, fman->dts_params.irq, err);
> -		return err;
> +		goto err_no_irq;
>  	}
>  
>  	if (fman->dts_params.err_irq != 0) {

[Severity: High]
If devm_request_irq() fails here, fman_init() has already succeeded. At the
end of fman_init(), the FMan hardware is fully enabled via enable(fman, cfg).

Jumping to err_no_irq calls fman_free_resources(), which destroys the
structures and unmaps the MURAM memory. However, there doesn't appear to be
any call to stop or disable the active hardware.

Will the hardware continue to operate and perform DMA to the freed MURAM
addresses, potentially causing memory corruption?