[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?

[PATCH net v2 0/2] net: fman: fix IRQ-related bugs and add probe cleanup

[ZhaoJinming]

Hi all,

This v2 series addresses the feedback from the previous irq_ready flag
approach (v1) and the separate UAF fix patch (v3). It replaces both
with a single root-cause fix: moving IRQ registration after driver
initialization is complete.

Issues fixed in this series:

1. Pre-init NULL dereference (crash on shared IRQ): is_init_done(NULL)
   returns true, so the guard in fman_irq()/fman_err_irq() is bypassed
   when fman->cfg is NULL before fman_config() allocates it. Moving
   devm_request_irq() to after fman_init() eliminates this window.

2. Use-after-free on probe failure (UAF): kfree(fman) was called in
   read_dts_node() error paths while devm IRQ handlers were still
   registered. Moving IRQ registration later eliminates this window.

3. Interrupt storm risk on shared IRQ line: deferred enable() to after
   IRQ registration, so the hardware is never active without a handler.

4. Memory leaks on probe failure: added fman_free_resources() and
   fman_muram_finish() to properly release all sub-resources on
   error paths after fman_config() succeeds.

5. Double-free in free_init_resources(): cleared fifo_offset and
   cam_offset after each call in fman_init() error paths, and added
   IS_ERR_VALUE() guards to prevent -ENOMEM from being treated as a
   valid offset.

Pre-existing issues NOT addressed in this series
(suggested as follow-up patches):

- Missing .remove callback: the driver has no remove/unbind path,
  causing all kzalloc'd resources to leak on device removal. This
  is a larger issue affecting the entire driver lifecycle.

- Child device probe ordering: of_platform_populate() is called
  before dev_set_drvdata(), so MAC drivers probing synchronously
  will find no parent driver data. This is a pre-existing design
  issue in the probe sequence.

- Hardware not disabled on cleanup: if enable() or devm_request_irq()
  fails after fman_init() completes, the hardware remains active
  while all software structures are freed. The driver lacks a
  disable/stop function.

- fman_muram_alloc() failure stores -ENOMEM in fifo_offset: this is
  a pre-existing issue that is now mitigated by the IS_ERR_VALUE()
  guard in free_init_resources(), but the root cause (error code
  stored in an unsigned long field) remains.

v2 changes:
- Move devm_request_irq() to fman_probe() after init (replaces
  irq_ready + READ_ONCE approach from v1)
- Defer enable() to after IRQ registration to prevent interrupt storm
- Add proper error cleanup with fman_free_resources()
- Add fman_muram_finish() for complete MURAM teardown
- Clear fifo_offset/cam_offset after free_init_resources() to
  prevent double-free
- Add IS_ERR_VALUE() guards in free_init_resources()
- Supersede the separate UAF fix patch (v3)

v1: https://lore.kernel.org/netdev/20260629084529.3709393-1-zhaojinming@uniontech.com/

Signed-off-by: ZhaoJinming <zhaojinming@uniontech.com>

[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.

Additionally, defer the hardware enable() call to after IRQ
registration to prevent a potential interrupt storm on the shared
IRQ line. Previously, fman_init() called enable() to activate the
hardware before the IRQ handler was registered. If the hardware
asserted an interrupt in this window, no handler would be present
to clear it, potentially causing the shared IRQ line to be
permanently disabled. Now, enable() is called after all handlers
are registered and the is_init_done guard is valid, so the handler
is always ready to service interrupts when the hardware is active.

Change enable() to read qmi_def_tnums_thresh from fman->state
instead of fman->cfg, since cfg is freed before enable() is called.

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

v2:
- move devm_request_irq() to fman_probe() after init (replaces
  irq_ready + READ_ONCE approach from v1)
- defer enable() to after IRQ registration to prevent interrupt
  storm on shared IRQ line
- 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 | 78 +++++++++++++---------
 drivers/net/ethernet/freescale/fman/fman.h |  1 +
 2 files changed, 49 insertions(+), 30 deletions(-)

diff --git a/drivers/net/ethernet/freescale/fman/fman.c b/drivers/net/ethernet/freescale/fman/fman.c
index 299bab043175..13913f152147 100644
--- a/drivers/net/ethernet/freescale/fman/fman.c
+++ b/drivers/net/ethernet/freescale/fman/fman.c
@@ -924,7 +924,7 @@ static void hwp_init(struct fman_hwp_regs __iomem *hwp_rg)
 	iowrite32be(HWP_RPIMAC_PEN, &hwp_rg->fmprrpimac);
 }
 
-static int enable(struct fman *fman, struct fman_cfg *cfg)
+static int enable(struct fman *fman)
 {
 	u32 cfg_reg = 0;
 
@@ -936,7 +936,8 @@ static int enable(struct fman *fman, struct fman_cfg *cfg)
 	cfg_reg = QMI_CFG_EN_COUNTERS;
 
 	/* Set enqueue and dequeue thresholds */
-	cfg_reg |= (cfg->qmi_def_tnums_thresh << 8) | cfg->qmi_def_tnums_thresh;
+	cfg_reg |= (fman->state->qmi_def_tnums_thresh << 8) |
+		   fman->state->qmi_def_tnums_thresh;
 
 	iowrite32be(BMI_INIT_START, &fman->bmi_regs->fmbm_init);
 	iowrite32be(cfg_reg | QMI_CFG_ENQ_EN | QMI_CFG_DEQ_EN,
@@ -2000,15 +2001,8 @@ static int fman_init(struct fman *fman)
 		return -EINVAL;
 	}
 
-	err = enable(fman, cfg);
-	if (err != 0)
-		return err;
-
 	enable_time_stamp(fman);
 
-	kfree(fman->cfg);
-	fman->cfg = NULL;
-
 	return 0;
 }
 
@@ -2695,7 +2689,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 +2711,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 +2767,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 +2823,49 @@ 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;
+		}
+	}
+
+	/* Free the config structure before enabling the hardware.
+	 * is_init_done() uses cfg == NULL to indicate init is complete,
+	 * so the IRQ handlers will properly process interrupts once
+	 * the hardware is enabled below.
+	 */
+	kfree(fman->cfg);
+	fman->cfg = NULL;
+
+	err = enable(fman);
+	if (err != 0) {
+		dev_err(dev, "%s: FMan enable failed\n", __func__);
+		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 or enable() 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.

Clear fman->fifo_offset and fman->cam_offset after each
free_init_resources() call in fman_init() to prevent a double-free
when fman_free_resources() calls free_init_resources() again on the
same error paths.

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
- clear fifo_offset/cam_offset after free_init_resources() in
  fman_init() to prevent double-free

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

diff --git a/drivers/net/ethernet/freescale/fman/fman.c b/drivers/net/ethernet/freescale/fman/fman.c
index 13913f152147..374f5b7305f8 100644
--- a/drivers/net/ethernet/freescale/fman/fman.c
+++ b/drivers/net/ethernet/freescale/fman/fman.c
@@ -1190,10 +1190,10 @@ static bool is_init_done(struct fman_cfg *cfg)
 
 static void free_init_resources(struct fman *fman)
 {
-	if (fman->cam_offset)
+	if (fman->cam_offset && !IS_ERR_VALUE(fman->cam_offset))
 		fman_muram_free_mem(fman->muram, fman->cam_offset,
 				    fman->cam_size);
-	if (fman->fifo_offset)
+	if (fman->fifo_offset && !IS_ERR_VALUE(fman->fifo_offset))
 		fman_muram_free_mem(fman->muram, fman->fifo_offset,
 				    fman->fifo_size);
 }
@@ -1963,6 +1963,8 @@ static int fman_init(struct fman *fman)
 	err = dma_init(fman);
 	if (err != 0) {
 		free_init_resources(fman);
+		fman->fifo_offset = 0;
+		fman->cam_offset = 0;
 		return err;
 	}
 
@@ -1975,6 +1977,8 @@ static int fman_init(struct fman *fman)
 					     fman->state->total_fifo_size);
 	if (IS_ERR_VALUE(fman->fifo_offset)) {
 		free_init_resources(fman);
+		fman->fifo_offset = 0;
+		fman->cam_offset = 0;
 		dev_err(fman->dev, "%s: MURAM alloc for BMI FIFO failed\n",
 			__func__);
 		return -ENOMEM;
@@ -1998,6 +2002,8 @@ static int fman_init(struct fman *fman)
 	fman->keygen = keygen_init(fman->kg_regs);
 	if (!fman->keygen) {
 		free_init_resources(fman);
+		fman->fifo_offset = 0;
+		fman->cam_offset = 0;
 		return -EINVAL;
 	}
 
@@ -2800,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;
@@ -2820,7 +2844,7 @@ static int fman_probe(struct platform_device *of_dev)
 
 	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.
@@ -2838,7 +2862,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) {
@@ -2848,7 +2872,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;
 		}
 	}
 
@@ -2863,7 +2887,7 @@ static int fman_probe(struct platform_device *of_dev)
 	err = enable(fman);
 	if (err != 0) {
 		dev_err(dev, "%s: FMan enable failed\n", __func__);
-		return err;
+		goto err_irq;
 	}
 
 	if (fman->dts_params.err_irq == 0) {
@@ -2891,6 +2915,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 0/2] net: fman: fix IRQ-related bugs and add probe cleanup

[Jakub Kicinski]

On Tue, 21 Jul 2026 16:58:39 +0800 ZhaoJinming wrote:
> This v2 series addresses the feedback from the previous irq_ready flag
> approach (v1) and the separate UAF fix patch (v3). It replaces both
> with a single root-cause fix: moving IRQ registration after driver
> initialization is complete.

The cover letter must explain how the issues were discovered and how
they were tested (what HW platform you tested on etc). Also:

Warning: drivers/net/ethernet/freescale/fman/fman_muram.c:133 missing initial short description on line:
 * fman_muram_finish
Warning: drivers/net/ethernet/freescale/fman/fman_muram.c:133 missing initial short description on line:
 * fman_muram_finish
-- 
pw-bot: cr