* [PATCH] memory: fsl_ifc: fix stale global pointer on probe failure/remove
@ 2026-10-17 10:24 Reid Downing (b1nv0id)
2026-10-17 14:51 ` Krzysztof Kozlowski
0 siblings, 1 reply; 3+ messages in thread
From: Reid Downing (b1nv0id) @ 2026-10-17 10:24 UTC (permalink / raw)
To: krzk; +Cc: linux-kernel, Reid Downing
fsl_ifc_ctrl_dev is a global pointer. It was set at the start of
fsl_ifc_ctrl_probe(), before anything could fail. If probe failed
partway through, or the device was later removed, the global was
never cleared. fsl_ifc_find() only checks it for non-NULL, so it
could end up dereferencing freed memory or an unmapped register
region.
Use a local variable while setting up the controller struct in
probe, and only publish it to the global once probe has fully
succeeded. Clear the global as the first step of remove, before
any teardown runs. Also guard the free_irq() of the optional
nand_irq, which may be zero.
Signed-off-by: Reid Downing (b1nv0id) <skibletz@gmail.com>
---
diff --git a/fsl_ifc.c b/fsl_ifc.c
index e89e0c6..ff52cae 100644
--- a/fsl_ifc.c
+++ b/fsl_ifc.c
@@ -88,8 +88,11 @@ static void fsl_ifc_ctrl_remove(struct platform_device *dev)
{
struct fsl_ifc_ctrl *ctrl = dev_get_drvdata(&dev->dev);
+ fsl_ifc_ctrl_dev = NULL;
+
of_platform_depopulate(&dev->dev);
- free_irq(ctrl->nand_irq, ctrl);
+ if (ctrl->nand_irq)
+ free_irq(ctrl->nand_irq, ctrl);
free_irq(ctrl->irq, ctrl);
irq_dispose_mapping(ctrl->nand_irq);
@@ -201,85 +204,82 @@ static irqreturn_t fsl_ifc_ctrl_irq(int irqno, void *data)
*/
static int fsl_ifc_ctrl_probe(struct platform_device *dev)
{
+ struct fsl_ifc_ctrl *ctrl;
int ret = 0;
int version, banks;
void __iomem *addr;
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);
+ 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);
+ dev_err(&dev->dev, "failed to install irq (%d)\n", ctrl->irq);
goto err_unmap_nandirq;
}
- 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;
}
}
@@ -289,17 +289,21 @@ static int fsl_ifc_ctrl_probe(struct platform_device *dev)
if (ret)
goto err_free_nandirq;
+ /* Only publish the global once probe has fully succeeded. */
+ fsl_ifc_ctrl_dev = ctrl;
+
return 0;
err_free_nandirq:
- 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_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.43.0
^ permalink raw reply [flat|nested] 3+ messages in thread
* Re: [PATCH] memory: fsl_ifc: fix stale global pointer on probe failure/remove
2026-10-17 10:24 [PATCH] memory: fsl_ifc: fix stale global pointer on probe failure/remove Reid Downing (b1nv0id)
@ 2026-10-17 14:51 ` Krzysztof Kozlowski
0 siblings, 0 replies; 3+ messages in thread
From: Krzysztof Kozlowski @ 2026-10-17 14:51 UTC (permalink / raw)
To: Reid Downing (b1nv0id); +Cc: linux-kernel
On 17/10/2026 12:24, Reid Downing (b1nv0id) wrote:
> fsl_ifc_ctrl_dev is a global pointer. It was set at the start of
> fsl_ifc_ctrl_probe(), before anything could fail. If probe failed
> partway through, or the device was later removed, the global was
> never cleared. fsl_ifc_find() only checks it for non-NULL, so it
> could end up dereferencing freed memory or an unmapped register
> region.
>
> Use a local variable while setting up the controller struct in
> probe, and only publish it to the global once probe has fully
> succeeded. Clear the global as the first step of remove, before
> any teardown runs. Also guard the free_irq() of the optional
> nand_irq, which may be zero.
>
> Signed-off-by: Reid Downing (b1nv0id) <skibletz@gmail.com>
Please use real identity and skip custom names. See submitting patches
documents if in doubts.
I received two patches from you - please version them correctly, e.g.
use b4, plus changelog under ---.
> ---
> diff --git a/fsl_ifc.c b/fsl_ifc.c
> index e89e0c6..ff52cae 100644
> --- a/fsl_ifc.c
> +++ b/fsl_ifc.c
I don't think you use git... Why? This cannot be applied. Or you just
send LLM slop.
Best regards,
Krzysztof
^ permalink raw reply [flat|nested] 3+ messages in thread
* [PATCH] memory: fsl_ifc: fix stale global pointer on probe failure/remove
@ 2026-10-17 10:09 Reid D
0 siblings, 0 replies; 3+ messages in thread
From: Reid D @ 2026-10-17 10:09 UTC (permalink / raw)
To: krzk; +Cc: linux-kernel
memory: fsl_ifc: fix stale global pointer on probe failure/remove
fsl_ifc_ctrl_dev is a global pointer. It was set at the start of
fsl_ifc_ctrl_probe(), before anything could fail. If probe failed
partway through, or the device was later removed, the global was
never cleared. fsl_ifc_find() only checks it for non-NULL, so it
could end up dereferencing freed memory or an unmapped register
region.
Use a local variable while setting up the controller struct in
probe, and only publish it to the global once probe has fully
succeeded. Clear the global as the first step of remove, before
any teardown runs. Also guard the free_irq() of the optional
nand_irq, which may be zero.
Signed-off-by: Reid Downing (b1nv0id) <skibletz@gmail.com>
---
diff --git a/drivers/memory/fsl_ifc.c b/drivers/memory/fsl_ifc.c
index e89e0c6..ff52cae 100644
--- a/drivers/memory/fsl_ifc.c
+++ b/drivers/memory/fsl_ifc.c
@@ -88,8 +88,11 @@ static void fsl_ifc_ctrl_remove(struct platform_device *dev)
{
struct fsl_ifc_ctrl *ctrl = dev_get_drvdata(&dev->dev);
+ fsl_ifc_ctrl_dev = NULL;
+
of_platform_depopulate(&dev->dev);
- free_irq(ctrl->nand_irq, ctrl);
+ if (ctrl->nand_irq)
+ free_irq(ctrl->nand_irq, ctrl);
free_irq(ctrl->irq, ctrl);
irq_dispose_mapping(ctrl->nand_irq);
@@ -201,85 +204,82 @@ static irqreturn_t fsl_ifc_ctrl_irq(int irqno, void *data)
*/
static int fsl_ifc_ctrl_probe(struct platform_device *dev)
{
+ struct fsl_ifc_ctrl *ctrl;
int ret = 0;
int version, banks;
void __iomem *addr;
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);
+ 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);
+ dev_err(&dev->dev, "failed to install irq (%d)\n", ctrl->irq);
goto err_unmap_nandirq;
}
- 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;
}
}
@@ -289,17 +289,21 @@ static int fsl_ifc_ctrl_probe(struct platform_device *dev)
if (ret)
goto err_free_nandirq;
+ /* Only publish the global once probe has fully succeeded. */
+ fsl_ifc_ctrl_dev = ctrl;
+
return 0;
err_free_nandirq:
- 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_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;
}
^ permalink raw reply [flat|nested] 3+ messages in thread
end of thread, other threads:[~2026-10-17 14:51 UTC | newest]
Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2026-10-17 10:24 [PATCH] memory: fsl_ifc: fix stale global pointer on probe failure/remove Reid Downing (b1nv0id)
2026-10-17 14:51 ` Krzysztof Kozlowski
-- strict thread matches above, loose matches on Subject: below --
2026-10-17 10:09 Reid D
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox
all inboxes | Powered by JetHome®