Re: [PATCH phy-next 09/13] phy: lynx-28g: common probe() and remove()

[Vladimir Oltean <vladimir.oltean@nxp.com>]

On Thu, May 28, 2026 at 06:02:15PM +0000, sashiko-bot@kernel.org wrote:
> Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
> 
> Pre-existing issues:
> - [High] Missing NULL check for the result of `of_device_get_match_data(dev)`.
> --
> 
> commit 4307af42a82313eaf19153e90d7c48e0bd5f9626
> Author: Vladimir Oltean <vladimir.oltean@nxp.com>
> 
> phy: lynx-28g: common probe() and remove()
> 
> Factor the device-agnostic logic from lynx_28g_probe() and
> lynx_28g_remove() into lynx_probe() and lynx_remove() inside
> phy-fsl-lynx-core.c. These will be shared with the 10G Lynx driver.
> 
> > diff --git a/drivers/phy/freescale/phy-fsl-lynx-28g.c b/drivers/phy/freescale/phy-fsl-lynx-28g.c
> > --- a/drivers/phy/freescale/phy-fsl-lynx-28g.c
> > +++ b/drivers/phy/freescale/phy-fsl-lynx-28g.c
> [ ... ]
> >  static int lynx_28g_probe(struct platform_device *pdev)
> >  {
> >  	struct device *dev = &pdev->dev;
> > +	const struct lynx_info *info;
> >  
> >  	/*
> >  	 * If we get here it means we probed on a device tree where
> >  	 * "fsl,lynx-28g" wasn't the fallback, but the sole compatible string.
> >  	 */
> > +	info = of_device_get_match_data(dev);
> 
> [Severity: High]
> This is a pre-existing issue, but should we check if info is NULL before
> proceeding?
> 
> If a user binds the driver to a device using the sysfs bind interface, and
> the device's OF node does not match the driver's of_match_table,
> of_device_get_match_data() will return NULL.
> 
> > +	if (info == &lynx_info_compat)
> >  		dev_warn(dev, "Please update device tree to use per-device compatible strings\n");
> >  
> > +	return lynx_probe(pdev, info, &lynx_28g_ops);
> >  }
> 
> [ ... ]
> > diff --git a/drivers/phy/freescale/phy-fsl-lynx-core.c b/drivers/phy/freescale/phy-fsl-lynx-core.c
> > --- a/drivers/phy/freescale/phy-fsl-lynx-core.c
> > +++ b/drivers/phy/freescale/phy-fsl-lynx-core.c
> [ ... ]
> > +int lynx_probe(struct platform_device *pdev, const struct lynx_info *info,
> > +	       const struct phy_ops *phy_ops)
> > +{
> [ ... ]
> > +	priv->dev = dev;
> > +	priv->info = info;
> > +	dev_set_drvdata(dev, priv);
> > +	spin_lock_init(&priv->pcc_lock);
> > +	INIT_DELAYED_WORK(&priv->cdr_check, lynx_cdr_lock_check);
> > +
> > +	priv->lane = devm_kcalloc(dev, priv->info->num_lanes,
> 
> [Severity: High]
> This isn't a bug introduced by this patch, but if info was NULL as mentioned
> above, would this unconditionally dereference it and cause a kernel panic?
> 
> > +				  sizeof(*priv->lane), GFP_KERNEL);
> > +	if (!priv->lane)
> > +		return -ENOMEM;
> [ ... ]
> 
> -- 
> Sashiko AI review · https://sashiko.dev/#/patchset/20260528172404.733196-1-vladimir.oltean@nxp.com?part=9

This seems to be true. I was able to trigger a kernel oops by running:

$ echo 1ea0000.phy > /sys/bus/platform/drivers/lynx-10g/unbind
$ echo lynx-28g > /sys/bus/platform/devices/1ea0000.phy/driver_override
$ echo 1ea0000.phy > /sys/bus/platform/drivers/lynx-28g/bind
Internal error: Oops: 0000000096000004 [#1]  SMP
Hardware name: LS1028A RDB Board (DT)
pc : lynx_probe+0x118/0x4fc
lr : lynx_probe+0x110/0x4fc
Call trace:
 lynx_probe+0x118/0x4fc (P)
 lynx_28g_probe+0x54/0x7c
 platform_probe+0x68/0xa4
 really_probe+0x14c/0x2ec
 __driver_probe_device+0xc8/0x170
 device_driver_attach+0x58/0xa8
 bind_store+0xd8/0x118
 drv_attr_store+0x24/0x38

It seems driver_override is a huge foot gun, which probably many drivers
are vulnerable to. I will include a patch protecting from this in the
next series.

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