[PATCH] memory: fsl_ifc: Clear fsl_ifc_ctrl_dev on probe failure and removal

[PATCH] memory: fsl_ifc: Clear fsl_ifc_ctrl_dev on probe failure and removal

[Krzysztof Kozlowski]

Global fsl_ifc_ctrl_dev is allocated with devm_kzalloc(), so it is freed
when probe fails or when the device is unbound, but the exported global
pointer is never reset.  Subsequent calls to fsl_ifc_find() or probing
of fsl_ifc_nand would then pass the NULL check and dereference freed
memory.  Issue is mostly theoretical, because there are no users/calls
to fsl_ifc_find().

Rework the logic, so the global fsl_ifc_ctrl_dev is assigned only after
device state is properly initialized in probe(), except requesting
interrupts.  This makes sure that early call to fsl_ifc_find() will not
use the device driver while it is not fully initialized.

Clear the global pointer on exit and error paths, to fix the possible
use-after-free.

Reported-by: Reid Downing <skibletz@gmail.com>
Closes: https://lore.kernel.org/all/20261025170833.1316443-1-skibletz@gmail.com/
Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://sashiko.dev/#/bug/linux-72f58ab4-9f9d-4b00-949c-00929918a3b3
Fixes: 8e0d09b1232d ("memory: fsl_ifc: fix leak of private memory on probe failure")
Fixes: a20cbdeffce2 ("powerpc/fsl: Add support for Integrated Flash Controller")
Cc: stable@vger.kernel.org
Signed-off-by: Krzysztof Kozlowski <krzysztof.kozlowski@oss.qualcomm.com>
---
 drivers/memory/fsl_ifc.c | 73 +++++++++++++++++++++-------------------
 1 file changed, 38 insertions(+), 35 deletions(-)

diff --git a/drivers/memory/fsl_ifc.c b/drivers/memory/fsl_ifc.c
index 6dcc317d95d4..ab188f151c94 100644
--- a/drivers/memory/fsl_ifc.c
+++ b/drivers/memory/fsl_ifc.c
@@ -92,6 +92,7 @@ static void fsl_ifc_ctrl_remove(struct platform_device *dev)
 	if (ctrl->nand_irq)
 		free_irq(ctrl->nand_irq, ctrl);
 	free_irq(ctrl->irq, ctrl);
+	fsl_ifc_ctrl_dev = NULL;
 
 	irq_dispose_mapping(ctrl->nand_irq);
 	irq_dispose_mapping(ctrl->irq);
@@ -205,82 +206,82 @@ static int fsl_ifc_ctrl_probe(struct platform_device *dev)
 	int ret = 0;
 	int version, banks;
 	void __iomem *addr;
+	struct fsl_ifc_ctrl *ctrl;
 
 	dev_info(&dev->dev, "Freescale Integrated Flash Controller\n");
 
-	fsl_ifc_ctrl_dev = devm_kzalloc(&dev->dev, sizeof(*fsl_ifc_ctrl_dev),
-					GFP_KERNEL);
-	if (!fsl_ifc_ctrl_dev)
+	ctrl = devm_kzalloc(&dev->dev, sizeof(*ctrl), GFP_KERNEL);
+	if (!ctrl)
 		return -ENOMEM;
 
-	dev_set_drvdata(&dev->dev, fsl_ifc_ctrl_dev);
+	dev_set_drvdata(&dev->dev, ctrl);
 
 	/* IOMAP the entire IFC region */
-	fsl_ifc_ctrl_dev->gregs = of_iomap(dev->dev.of_node, 0);
-	if (!fsl_ifc_ctrl_dev->gregs) {
+	ctrl->gregs = of_iomap(dev->dev.of_node, 0);
+	if (!ctrl->gregs) {
 		dev_err(&dev->dev, "failed to get memory region\n");
 		return -ENODEV;
 	}
 
 	if (of_property_read_bool(dev->dev.of_node, "little-endian")) {
-		fsl_ifc_ctrl_dev->little_endian = true;
+		ctrl->little_endian = true;
 		dev_dbg(&dev->dev, "IFC REGISTERS are LITTLE endian\n");
 	} else {
-		fsl_ifc_ctrl_dev->little_endian = false;
+		ctrl->little_endian = false;
 		dev_dbg(&dev->dev, "IFC REGISTERS are BIG endian\n");
 	}
 
-	version = ifc_in32(&fsl_ifc_ctrl_dev->gregs->ifc_rev) &
-			FSL_IFC_VERSION_MASK;
+	version = ifc_in32(&ctrl->gregs->ifc_rev) & FSL_IFC_VERSION_MASK;
 
 	banks = (version == FSL_IFC_VERSION_1_0_0) ? 4 : 8;
 	dev_info(&dev->dev, "IFC version %d.%d, %d banks\n",
 		version >> 24, (version >> 16) & 0xf, banks);
 
-	fsl_ifc_ctrl_dev->version = version;
-	fsl_ifc_ctrl_dev->banks = banks;
+	ctrl->version = version;
+	ctrl->banks = banks;
 
-	addr = fsl_ifc_ctrl_dev->gregs;
+	addr = ctrl->gregs;
 	if (version >= FSL_IFC_VERSION_2_0_0)
 		addr += PGOFFSET_64K;
 	else
 		addr += PGOFFSET_4K;
-	fsl_ifc_ctrl_dev->rregs = addr;
+	ctrl->rregs = addr;
 
 	/* get the Controller level irq */
-	fsl_ifc_ctrl_dev->irq = irq_of_parse_and_map(dev->dev.of_node, 0);
-	if (fsl_ifc_ctrl_dev->irq == 0) {
+	ctrl->irq = irq_of_parse_and_map(dev->dev.of_node, 0);
+	if (ctrl->irq == 0) {
 		dev_err(&dev->dev, "failed to get irq resource for IFC\n");
 		ret = -ENODEV;
 		goto err;
 	}
 
 	/* get the nand machine irq */
-	fsl_ifc_ctrl_dev->nand_irq =
-			irq_of_parse_and_map(dev->dev.of_node, 1);
+	ctrl->nand_irq = irq_of_parse_and_map(dev->dev.of_node, 1);
 
-	fsl_ifc_ctrl_dev->dev = &dev->dev;
+	ctrl->dev = &dev->dev;
 
-	ret = fsl_ifc_ctrl_init(fsl_ifc_ctrl_dev);
+	ret = fsl_ifc_ctrl_init(ctrl);
 	if (ret < 0)
 		goto err_unmap_nandirq;
 
-	init_waitqueue_head(&fsl_ifc_ctrl_dev->nand_wait);
+	init_waitqueue_head(&ctrl->nand_wait);
 
-	ret = request_irq(fsl_ifc_ctrl_dev->irq, fsl_ifc_ctrl_irq, IRQF_SHARED,
-			  "fsl-ifc", fsl_ifc_ctrl_dev);
+	fsl_ifc_ctrl_dev = ctrl;
+
+	ret = request_irq(ctrl->irq, fsl_ifc_ctrl_irq, IRQF_SHARED,
+			  "fsl-ifc", ctrl);
 	if (ret != 0) {
 		dev_err(&dev->dev, "failed to install irq (%d)\n",
-			fsl_ifc_ctrl_dev->irq);
-		goto err_unmap_nandirq;
+			ctrl->irq);
+		goto err_clear_global;
 	}
 
-	if (fsl_ifc_ctrl_dev->nand_irq) {
-		ret = request_irq(fsl_ifc_ctrl_dev->nand_irq, fsl_ifc_nand_irq,
-				0, "fsl-ifc-nand", fsl_ifc_ctrl_dev);
+	if (ctrl->nand_irq) {
+		ret = request_irq(ctrl->nand_irq, fsl_ifc_nand_irq,
+				0, "fsl-ifc-nand", ctrl);
 		if (ret != 0) {
 			dev_err(&dev->dev, "failed to install irq (%d)\n",
-				fsl_ifc_ctrl_dev->nand_irq);
+				ctrl->nand_irq);
 			goto err_free_irq;
 		}
 	}
@@ -293,15 +294,17 @@ static int fsl_ifc_ctrl_probe(struct platform_device *dev)
 	return 0;
 
 err_free_nandirq:
-	if (fsl_ifc_ctrl_dev->nand_irq)
-		free_irq(fsl_ifc_ctrl_dev->nand_irq, fsl_ifc_ctrl_dev);
+	if (ctrl->nand_irq)
+		free_irq(ctrl->nand_irq, ctrl);
 err_free_irq:
-	free_irq(fsl_ifc_ctrl_dev->irq, fsl_ifc_ctrl_dev);
+	free_irq(ctrl->irq, ctrl);
+err_clear_global:
+	fsl_ifc_ctrl_dev = NULL;
 err_unmap_nandirq:
-	irq_dispose_mapping(fsl_ifc_ctrl_dev->nand_irq);
-	irq_dispose_mapping(fsl_ifc_ctrl_dev->irq);
+	irq_dispose_mapping(ctrl->nand_irq);
+	irq_dispose_mapping(ctrl->irq);
 err:
-	iounmap(fsl_ifc_ctrl_dev->gregs);
+	iounmap(ctrl->gregs);
 	return ret;
 }
 
-- 
2.53.0

Re: [PATCH] memory: fsl_ifc: Clear fsl_ifc_ctrl_dev on probe failure and removal

[Krzysztof Kozlowski]

On Sun, 01 Nov 2026 13:08:39 +0100, Krzysztof Kozlowski wrote:
> Global fsl_ifc_ctrl_dev is allocated with devm_kzalloc(), so it is freed
> when probe fails or when the device is unbound, but the exported global
> pointer is never reset.  Subsequent calls to fsl_ifc_find() or probing
> of fsl_ifc_nand would then pass the NULL check and dereference freed
> memory.  Issue is mostly theoretical, because there are no users/calls
> to fsl_ifc_find().
> 
> [...]

Applied, thanks!

[1/1] memory: fsl_ifc: Clear fsl_ifc_ctrl_dev on probe failure and removal
      https://git.kernel.org/krzk/linux-mem-ctrl/c/a78ee2e05f7c7dd09e58ea615960105f78dc3c75

Best regards,
-- 
Krzysztof Kozlowski <krzk@kernel.org>

[PATCH] memory: fsl_ifc: Do not dereference the unset global pointer in probe

[Sasha Levin]

Commit a78ee2e05f7c ("memory: fsl_ifc: Clear fsl_ifc_ctrl_dev on probe failure
and removal") assigns fsl_ifc_ctrl_dev near the end of probe, but probe still
reads the IFC revision and fsl_ifc_ctrl_init() still programs the event
registers before that. Both go through ifc_in32() and ifc_out32(), which take
the register endianness from fsl_ifc_ctrl_dev, so the first probe dereferences
a NULL pointer.

Use local accessors that take the endianness from the controller being probed
for these register accesses.

Fixes: a78ee2e05f7c ("memory: fsl_ifc: Clear fsl_ifc_ctrl_dev on probe failure and removal")
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
 drivers/memory/fsl_ifc.c | 34 +++++++++++++++++++++++++++-------
 1 file changed, 27 insertions(+), 7 deletions(-)

diff --git a/drivers/memory/fsl_ifc.c b/drivers/memory/fsl_ifc.c
index 83991c05a6a5c..abd033bd94c5a 100644
--- a/drivers/memory/fsl_ifc.c
+++ b/drivers/memory/fsl_ifc.c
@@ -63,6 +63,23 @@ int fsl_ifc_find(phys_addr_t addr_base)
 }
 EXPORT_SYMBOL(fsl_ifc_find);
 
+static u32 fsl_ifc_ctrl_in32(struct fsl_ifc_ctrl *ctrl, void __iomem *addr)
+{
+	if (ctrl->little_endian)
+		return ioread32(addr);
+
+	return ioread32be(addr);
+}
+
+static void fsl_ifc_ctrl_out32(struct fsl_ifc_ctrl *ctrl, u32 val,
+			       void __iomem *addr)
+{
+	if (ctrl->little_endian)
+		iowrite32(val, addr);
+	else
+		iowrite32be(val, addr);
+}
+
 static int fsl_ifc_ctrl_init(struct fsl_ifc_ctrl *ctrl)
 {
 	struct fsl_ifc_global __iomem *ifc = ctrl->gregs;
@@ -70,16 +87,18 @@ static int fsl_ifc_ctrl_init(struct fsl_ifc_ctrl *ctrl)
 	/*
 	 * Clear all the common status and event registers
 	 */
-	if (ifc_in32(&ifc->cm_evter_stat) & IFC_CM_EVTER_STAT_CSER)
-		ifc_out32(IFC_CM_EVTER_STAT_CSER, &ifc->cm_evter_stat);
+	if (fsl_ifc_ctrl_in32(ctrl, &ifc->cm_evter_stat) & IFC_CM_EVTER_STAT_CSER)
+		fsl_ifc_ctrl_out32(ctrl, IFC_CM_EVTER_STAT_CSER,
+				   &ifc->cm_evter_stat);
 
 	/* enable all error and events */
-	ifc_out32(IFC_CM_EVTER_EN_CSEREN, &ifc->cm_evter_en);
+	fsl_ifc_ctrl_out32(ctrl, IFC_CM_EVTER_EN_CSEREN, &ifc->cm_evter_en);
 
 	/* enable all error and event interrupts */
-	ifc_out32(IFC_CM_EVTER_INTR_EN_CSERIREN, &ifc->cm_evter_intr_en);
-	ifc_out32(0x0, &ifc->cm_erattr0);
-	ifc_out32(0x0, &ifc->cm_erattr1);
+	fsl_ifc_ctrl_out32(ctrl, IFC_CM_EVTER_INTR_EN_CSERIREN,
+			   &ifc->cm_evter_intr_en);
+	fsl_ifc_ctrl_out32(ctrl, 0x0, &ifc->cm_erattr0);
+	fsl_ifc_ctrl_out32(ctrl, 0x0, &ifc->cm_erattr1);
 
 	return 0;
 }
@@ -231,7 +250,8 @@ static int fsl_ifc_ctrl_probe(struct platform_device *dev)
 		dev_dbg(&dev->dev, "IFC REGISTERS are BIG endian\n");
 	}
 
-	version = ifc_in32(&ctrl->gregs->ifc_rev) & FSL_IFC_VERSION_MASK;
+	version = fsl_ifc_ctrl_in32(ctrl, &ctrl->gregs->ifc_rev) &
+		  FSL_IFC_VERSION_MASK;
 
 	banks = (version == FSL_IFC_VERSION_1_0_0) ? 4 : 8;
 	dev_info(&dev->dev, "IFC version %d.%d, %d banks\n",
-- 
2.53.0