* [PATCH v2 1/2] net: fman: move IRQ registration after init to prevent NULL deref and UAF
2026-07-02 9:28 ` ZhaoJinming
@ 2026-07-02 9:28 ` ZhaoJinming
2026-07-02 20:42 ` Andrew Lunn
0 siblings, 1 reply; 6+ messages in thread
From: ZhaoJinming @ 2026-07-02 9:28 UTC (permalink / raw)
To: pabeni
Cc: andrew+netdev, andrew, davem, edumazet, horms, kuba, linux-kernel,
madalin.bucur, netdev, sean.anderson, 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
^ permalink raw reply related [flat|nested] 6+ messages in thread