[PATCH] net: fman: guard IRQ handlers against pre-init interrupt

[PATCH] net: fman: guard IRQ handlers against pre-init interrupt

[ZhaoJinming]

read_dts_node() registers shared interrupt handlers via
devm_request_irq() with fman as the dev_id, passing it to fman_irq()
and fman_err_irq(). 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 guard against incomplete initialization via:

    if (!is_init_done(fman->cfg))
        return IRQ_NONE;

However, is_init_done(NULL) returns true (intended to indicate that
fman_init() has completed and cfg has been freed). This means when
cfg is NULL before fman_config() allocates it, the guard does not
take effect:

    is_init_done(NULL)  -> returns true
    !true            -> false
    guard skipped     -> proceeds to dereference NULL fpm_regs

If another device on the same shared IRQ line fires during the window
between devm_request_irq() and fman_init(), the handler accesses
NULL fman->fpm_regs via ioread32be(), causing a crash. The window
includes of_platform_populate() which can be slow.

Add an irq_ready flag to struct fman that is set to true only after
fman_init() completes in fman_probe(). Check this flag at the start
of fman_irq() and fman_err_irq() before any register access.

Use READ_ONCE()/WRITE_ONCE() for the flag accesses since it is a
cross-context shared variable written in process context and read
in interrupt context.

This issue was identified in code review during the discussion of a
separate IRQF_SHARED UAF fix patch:
https://lore.kernel.org/netdev/20260626162323.GE1310988@horms.kernel.org/

Signed-off-by: ZhaoJinming <zhaojinming@uniontech.com>
---
 drivers/net/ethernet/freescale/fman/fman.c | 5 +++++
 drivers/net/ethernet/freescale/fman/fman.h | 1 +
 2 files changed, 6 insertions(+)

diff --git a/drivers/net/ethernet/freescale/fman/fman.c b/drivers/net/ethernet/freescale/fman/fman.c
index 013273a2de32..f14cb02d85a4 100644
--- a/drivers/net/ethernet/freescale/fman/fman.c
+++ b/drivers/net/ethernet/freescale/fman/fman.c
@@ -2510,6 +2510,8 @@ static irqreturn_t fman_err_irq(int irq, void *handle)
 	struct fman_fpm_regs __iomem *fpm_rg;
 	irqreturn_t single_ret, ret = IRQ_NONE;
 
+	if (!READ_ONCE(fman->irq_ready))
+		return IRQ_NONE;
 	if (!is_init_done(fman->cfg))
 		return IRQ_NONE;
 
@@ -2608,6 +2610,8 @@ static irqreturn_t fman_irq(int irq, void *handle)
 	struct fman_fpm_regs __iomem *fpm_rg;
 	irqreturn_t single_ret, ret = IRQ_NONE;
 
+	if (!READ_ONCE(fman->irq_ready))
+		return IRQ_NONE;
 	if (!is_init_done(fman->cfg))
 		return IRQ_NONE;
 
@@ -2845,6 +2849,7 @@ static int fman_probe(struct platform_device *of_dev)
 		dev_err(dev, "%s: FMan init failed\n", __func__);
 		return -EINVAL;
 	}
+	WRITE_ONCE(fman->irq_ready, true);
 
 	if (fman->dts_params.err_irq == 0) {
 		fman_set_exception(fman, FMAN_EX_DMA_BUS_ERROR, false);
diff --git a/drivers/net/ethernet/freescale/fman/fman.h b/drivers/net/ethernet/freescale/fman/fman.h
index 74eb62eba0d7..ce06d5867a50 100644
--- a/drivers/net/ethernet/freescale/fman/fman.h
+++ b/drivers/net/ethernet/freescale/fman/fman.h
@@ -314,6 +314,7 @@ struct fman {
 	struct fman_state_struct *state;
 
 	struct fman_cfg *cfg;
+	bool irq_ready;                  /* true after fman_init() completes */
 	struct muram_info *muram;
 	struct fman_keygen *keygen;
 	/* cam section in muram */
-- 
2.20.1

Re: [PATCH] net: fman: guard IRQ handlers against pre-init interrupt

[Paolo Abeni]

On 6/29/26 10:45 AM, ZhaoJinming wrote:
> read_dts_node() registers shared interrupt handlers via
> devm_request_irq() with fman as the dev_id, passing it to fman_irq()
> and fman_err_irq(). 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 guard against incomplete initialization via:
> 
>     if (!is_init_done(fman->cfg))
>         return IRQ_NONE;
> 
> However, is_init_done(NULL) returns true (intended to indicate that
> fman_init() has completed and cfg has been freed). This means when
> cfg is NULL before fman_config() allocates it, the guard does not
> take effect:
> 
>     is_init_done(NULL)  -> returns true
>     !true            -> false
>     guard skipped     -> proceeds to dereference NULL fpm_regs
> 
> If another device on the same shared IRQ line fires during the window
> between devm_request_irq() and fman_init(), the handler accesses
> NULL fman->fpm_regs via ioread32be(), causing a crash. The window
> includes of_platform_populate() which can be slow.
> 
> Add an irq_ready flag to struct fman that is set to true only after
> fman_init() completes in fman_probe(). Check this flag at the start
> of fman_irq() and fman_err_irq() before any register access.
> 
> Use READ_ONCE()/WRITE_ONCE() for the flag accesses since it is a
> cross-context shared variable written in process context and read
> in interrupt context.
> 
> This issue was identified in code review during the discussion of a
> separate IRQF_SHARED UAF fix patch:
> https://lore.kernel.org/netdev/20260626162323.GE1310988@horms.kernel.org/
> 
> Signed-off-by: ZhaoJinming <zhaojinming@uniontech.com>
> ---
>  drivers/net/ethernet/freescale/fman/fman.c | 5 +++++
>  drivers/net/ethernet/freescale/fman/fman.h | 1 +
>  2 files changed, 6 insertions(+)
> 
> diff --git a/drivers/net/ethernet/freescale/fman/fman.c b/drivers/net/ethernet/freescale/fman/fman.c
> index 013273a2de32..f14cb02d85a4 100644
> --- a/drivers/net/ethernet/freescale/fman/fman.c
> +++ b/drivers/net/ethernet/freescale/fman/fman.c
> @@ -2510,6 +2510,8 @@ static irqreturn_t fman_err_irq(int irq, void *handle)
>  	struct fman_fpm_regs __iomem *fpm_rg;
>  	irqreturn_t single_ret, ret = IRQ_NONE;
>  
> +	if (!READ_ONCE(fman->irq_ready))
> +		return IRQ_NONE;

Sashiko noted that on weakly ordered arch this could not be enough:

https://sashiko.dev/#/patchset/20260629084529.3709393-1-zhaojinming%40uniontech.com

Also I have the feeling this is papering over the real issue. Why
initializing the irq when the driver is not yet ready? Why don't move
IRQ initialization later?

/P

Re: [PATCH] net: fman: guard IRQ handlers against pre-init interrupt

[ZhaoJinming]

Hi Paolo,

Thanks for the review. You are right -- the irq_ready approach was
fundamentally flawed, and moving IRQ registration after init is the
correct fix.

I have updated the series to address your feedback:

Patch 1/2: Move devm_request_irq() out of read_dts_node() and into
fman_probe(), after fman_config() and fman_init() have completed.
This eliminates both the pre-init NULL dereference and the UAF on
probe failure in a single change. A separate UAF fix patch is no
longer needed.

Patch 2/2: Add proper error cleanup in fman_probe(). The existing
driver had no unified cleanup path -- fman_init() and devm_request_irq()
failure paths leaked fman and its sub-resources. This patch adds
fman_free_resources() and fman_muram_finish() to release everything
correctly, including explicitly freeing IRQs before kfree(fman) to
avoid reintroducing the UAF window on the cleanup path itself.

Patches are against net-next/master.

v1 patch (irq_ready + READ_ONCE) is dropped. v3 UAF fix patch is
superseded.

Thanks,
Jinming

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

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 013273a2de32..a33c9c26f99c 100644
--- a/drivers/net/ethernet/freescale/fman/fman.c
+++ b/drivers/net/ethernet/freescale/fman/fman.c
@@ -2693,7 +2693,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;
 
@@ -2715,7 +2715,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);
@@ -2771,25 +2771,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);
@@ -2846,6 +2827,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


From 6f2bd949c043ba7a3bb650430014ef933c698573 Mon Sep 17 00:00:00 2001
From: ZhaoJinming <zhaojinming@uniontech.com>
Date: Thu, 2 Jul 2026 16:34:06 +0800
Subject: [PATCH v2 2/2] net: fman: add error cleanup path in fman_probe

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.

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 a33c9c26f99c..a8ca0998b752 100644
--- a/drivers/net/ethernet/freescale/fman/fman.c
+++ b/drivers/net/ethernet/freescale/fman/fman.c
@@ -2804,6 +2804,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;
@@ -2819,12 +2837,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.
@@ -2842,7 +2861,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) {
@@ -2852,7 +2871,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;
 		}
 	}
 
@@ -2881,6 +2900,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 v2 1/2] net: fman: move IRQ registration after init to prevent NULL deref and UAF

[Andrew Lunn]

On Thu, Jul 02, 2026 at 05:28:15PM +0800, ZhaoJinming wrote:
> read_dts_node() registers shared interrupt handlers via
> devm_request_irq() with fman as dev_id. Two bugs exist in the
> current code:

Please start a new thread with a new version of the patch. The CI
system just thinks this is part of the discussion, not something it
must test.

Please also read:

https://www.kernel.org/doc/html/latest/process/maintainer-netdev.html

and set the Subject: line correctly, etc.

     Andrew