Re: [PATCH v2 phy-next 01/15] phy: lynx-28g: reject probing on devices with unsupported OF nodes

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:

Pre-existing issues:
- [High] The lynx_28g_xlate callback can return NULL for uninitialized PHY lanes, leading to a NULL pointer dereference in the PHY framework.
- [High] The PHY device retains a pointer to an OF child node after its reference count has been dropped, leading to a potential Use-After-Free.
--

commit 74be4f3f82272eac244d345f53d8340e9a230084
Author: Vladimir Oltean <vladimir.oltean@nxp.com>

phy: lynx-28g: reject probing on devices with unsupported OF nodes

This patch fixes a crash triggered via the driver_override mechanism when
of_device_get_match_data() returns NULL for a device with a different
compatible string. It adds a check to return -ENODEV if the match data
is missing.

> diff --git a/drivers/phy/freescale/phy-fsl-lynx-28g.c b/drivers/phy/freescale/phy-fsl-lynx-28g.c
> index 92bfc5f65e0b8..4461b47a16ad6 100644
> --- a/drivers/phy/freescale/phy-fsl-lynx-28g.c
> +++ b/drivers/phy/freescale/phy-fsl-lynx-28g.c
> @@ -1477,6 +1477,9 @@ static int lynx_28g_probe(struct platform_device *pdev)
>  
>  	priv->dev = dev;
>  	priv->info = of_device_get_match_data(dev);
> +	if (!priv->info)
> +		return -ENODEV;
> +
>  	dev_set_drvdata(dev, priv);
>  	spin_lock_init(&priv->pcc_lock);
>  	INIT_DELAYED_WORK(&priv->cdr_check, lynx_28g_cdr_lock_check);

[Severity: High]
This isn't a bug introduced by this patch, but does this function leave the
PHY device with a pointer to a freed child node later in the probe process?

Further down in lynx_28g_probe():

    for_each_available_child_of_node(dn, child) {
        ...
        err = lynx_28g_probe_lane(priv, reg, child);
    ...

The for_each_available_child_of_node() macro automatically handles
reference counting for the child node.

When lynx_28g_probe_lane() creates the PHY, phy_create() stores this
child node in phy->dev.of_node, but it does not call of_node_get() to
acquire an additional reference.

Once the loop advances, the iterator automatically calls of_node_put(),
dropping the reference. If the node is later dynamically freed, such as
during a DT overlay unload, could this cause a use-after-free when
phy->dev.of_node is accessed?

[Severity: High]
This is a pre-existing issue, but can the xlate callback return NULL for
uninitialized PHY lanes and cause a crash in the PHY framework?

If a requested lane index is within bounds but the lane was not probed
(due to a missing subnode in the device tree), priv->lane[idx].phy remains
NULL and is returned directly by lynx_28g_xlate().

When this NULL is passed to of_phy_get() by _of_phy_get():

drivers/phy/phy-core.c:of_phy_get() {
    ...
    if (IS_ERR(phy))
        return phy;

    if (!try_module_get(phy->ops->owner))
    ...
}

Since IS_ERR(NULL) evaluates to false, it proceeds to dereference the NULL
pointer via phy->ops->owner. Should the callback return ERR_PTR(-ENODEV)
instead for missing lanes?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260529171509.1163787-1-vladimir.oltean@nxp.com?part=1

-- 
linux-phy mailing list
linux-phy@lists.infradead.org
https://lists.infradead.org/mailman/listinfo/linux-phy