Linux-PHY Archive on lore.kernel.org

* Re: [PATCH v2 phy-next 01/15] phy: lynx-28g: reject probing on devices with unsupported OF nodes
From: Vladimir Oltean @ 2026-06-02 14:52 UTC (permalink / raw)
  To: sashiko-reviews; +Cc: vkoul, linux-phy, olteanv, neil.armstrong
In-Reply-To: <20260529173544.908FE1F00893@smtp.kernel.org>

On Fri, May 29, 2026 at 05:35:43PM +0000, sashiko-bot@kernel.org wrote:
> 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?
>

This is a problem within phy_create() not calling of_node_get() when
assigning phy->dev.of_node. It is out of scope for this series.

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

Yeah, ok, _of_phy_get() does not work when phy_provider->of_xlate()
returns NULL, it needs to be an error-encoded pointer instead.
Will add one patch for v3.

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

^ permalink raw reply