On 7/1/26 14:26, [email protected] wrote:
Thank you for your contribution! Sashiko AI review found 2 potential issue(s)
to consider:
Pre-existing issues:
- [High] Missing NULL check for `dev->platform_data` before dereferencing.
- [High] Missing `remove` callback to clean up PHY lookup leads to a memory
leak and use-after-free.
--
commit a1a96e429cf3345472aae1b06f6ffe5b31edee81
Author: AngeloGioacchino Del Regno <[email protected]>
phy: phy-mtk-dp: Rename regs to regmap in struct mtk_dp_phy
In preparation to perform further cleanups and to extend the driver to
support more SoCs, rename the regs member to regmap to improve readability,
as this is a common name across many kernel drivers for a struct regmap.
diff --git a/drivers/phy/mediatek/phy-mtk-dp.c
b/drivers/phy/mediatek/phy-mtk-dp.c
index d7024a1443358..bf7b3a95e72d2 100644
--- a/drivers/phy/mediatek/phy-mtk-dp.c
+++ b/drivers/phy/mediatek/phy-mtk-dp.c
[ ... ]
@@ -176,7 +176,7 @@ static int mtk_dp_phy_probe(struct platform_device *pdev)
[Severity: High]
This is a pre-existing issue, but is it possible for dev->platform_data to be
NULL here?
Earlier in mtk_dp_phy_probe(), dev->platform_data is dereferenced directly
without a check:
drivers/phy/mediatek/phy-mtk-dp.c:mtk_dp_phy_probe() {
...
struct regmap *regs;
regs = *(struct regmap **)dev->platform_data;
if (!regs)
...
}
If the driver is manually bound via sysfs (or otherwise instantiated without
platform data), dev->platform_data will be NULL.
Should the code validate that dev->platform_data is not NULL before
dereferencing it to avoid a kernel panic?
No, that's not crashing it, because there's a NULL check anyway just after that.
[Severity: High]
This isn't a bug introduced by this patch, but does this driver leak the
phy_lookup struct and leave a dangling pointer if it gets unbound?
In mtk_dp_phy_probe(), when !dev->of_node is true, a lookup entry is
registered:
drivers/phy/mediatek/phy-mtk-dp.c:mtk_dp_phy_probe() {
...
if (!dev->of_node)
phy_create_lookup(phy, "dp", dev_name(dev));
return 0;
}
However, mtk_dp_phy_driver does not have a .remove callback to call
phy_remove_lookup():
drivers/phy/mediatek/phy-mtk-dp.c:
static struct platform_driver mtk_dp_phy_driver = {
.probe = mtk_dp_phy_probe,
.driver = {
.name = "mediatek-dp-phy",
},
};
If the driver is unbound, the phy_lookup struct is leaked and remains linked
in the global phys list. Since the phy struct gets freed by the
device-managed subsystem, subsequent device probes iterating the PHY lookup
list would trigger a use-after-free.
Could a .remove callback be added to unregister the lookup and prevent this?
That's right, but I didn't create this issue. I can fix that in a later commit
just for that, even though this code path is dead when using new style
devicetree.
if (!dp_phy)
return -ENOMEM;
- dp_phy->regs = regs;
+ dp_phy->regmap = regs;
phy = devm_phy_create(dev, NULL, &mtk_dp_phy_dev_ops);
if (IS_ERR(phy))
return dev_err_probe(dev, PTR_ERR(phy),
