Re: [PATCH 02/12] phy: phy-mtk-dp: Rename regs to regmap in struct mtk_dp_phy

[sashiko-bot@kernel.org]

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 <angelogioacchino.delregno@collabora.com>

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?

[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?

>  	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),

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260701122008.19509-1-angelogioacchino.delregno@collabora.com?part=2