Re: [PATCH net v2 1/2] net: phy: xilinx-gmii2rgmii: Avoid overwriting PHY drvdata

[netdev-bot+sashiko@kernel.org]

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

Critical: 1 · High: 1 (1) · Medium: 0 (3) · Low: 0

If you believe none of the issues are legitimate, please reply to each
issue of Medium severity or higher and explain why the AI is wrong.
If any of them are legitimate and you plan to respin, please let patchwork
know by including "pw-bot: cr" as a separate line at the end of your reply
(one such reply per series is enough).

- [Critical] net: phy: data race and type confusion computing priv from
  phydev->drv
- [High] net: phy: TOCTOU race in xgmiitorgmii_read_status yielding
  invalid pointer

Pre-existing issues:
- [High] At this commit the converter has no .remove callback.
- [Medium] xgmiitorgmii_configure() stores the int result of
  mdiobus_read(bus, addr, XILINX_GMII2RGMII_REG) in a u16 without…
- [Medium] At this commit, the device reference that of_phy_find_device()
  takes in xgmiitorgmii_probe() is dropped only on the…
- [Medium] xgmiitorgmii_probe() makes the wrapper visible with a plain
  store `priv->phy_dev->drv = &priv->conv_phy_drv`.

--

net: phy: xilinx-gmii2rgmii: Avoid overwriting PHY drvdata

This commit stops the GMII-to-RGMII converter from storing its private
data in the attached PHY's MDIO drvdata field, because that field belongs
to the underlying PHY driver. The read_status and set_loopback wrappers
now get struct gmii2rgmii by calling container_of_const() on phydev->drv.
xgmiitorgmii_configure() now takes a const pointer.

> diff --git a/drivers/net/phy/xilinx_gmii2rgmii.c b/drivers/net/phy/xilinx_gmii2rgmii.c
> index 2024d8ef36d98..61f71e977a574 100644
> --- a/drivers/net/phy/xilinx_gmii2rgmii.c
> +++ b/drivers/net/phy/xilinx_gmii2rgmii.c
> @@ -28,7 +28,7 @@ struct gmii2rgmii {
>  	struct mdio_device *mdio;
>  };
>  
> -static void xgmiitorgmii_configure(struct gmii2rgmii *priv, int speed)
> +static void xgmiitorgmii_configure(const struct gmii2rgmii *priv, int speed)
>  {
>  	struct mii_bus *bus = priv->mdio->bus;
>  	int addr = priv->mdio->addr;

[Severity: Medium]
This isn't a bug introduced by this patch, but this function is being
touched here. What happens if the MDIO read fails?

drivers/net/phy/xilinx_gmii2rgmii.c:xgmiitorgmii_configure() {
    ...
	val = mdiobus_read(bus, addr, XILINX_GMII2RGMII_REG);
	val &= ~XILINX_GMII2RGMII_SPEED_MASK;
    ...
	mdiobus_write(bus, addr, XILINX_GMII2RGMII_REG, val);
}

mdiobus_read() returns a negative errno on failure. Since val is a u16,
-EIO becomes 0xfffb.

Only the speed bits are cleared before the write. Wouldn't that set other
bits of the converter register to values the hardware never reported?

The mdiobus_write() result is also ignored. Because the function returns
void, xgmiitorgmii_read_status() and xgmiitorgmii_set_loopback() always
return 0.

Could the PHY layer then believe the converter is set to phydev->speed
when it is not?

> @@ -49,7 +49,9 @@ static void xgmiitorgmii_configure(struct gmii2rgmii *priv, int speed)
>  
>  static int xgmiitorgmii_read_status(struct phy_device *phydev)
>  {
> -	struct gmii2rgmii *priv = mdiodev_get_drvdata(&phydev->mdio);
> +	const struct gmii2rgmii *priv = container_of_const(phydev->drv,
> +							   struct gmii2rgmii,
> +							   conv_phy_drv);
>  	int err;
>  
>  	if (priv->phy_drv->read_status)

[Severity: High]
Can a caller that does not hold phydev->lock see phydev->drv change
between selecting this callback and evaluating container_of_const()?

phy_read_status() takes no lock and loads phydev->drv several times:

include/linux/phy.h:phy_read_status() {
    ...
	if (phydev->drv->read_status)
		return phydev->drv->read_status(phydev);
    ...
}

xgmiitorgmii_read_status() then loads phydev->drv once more to compute
priv. phy_state_machine() calls in with phydev->lock held. Some MAC
drivers instead call phy_read_status() directly from their own link
polling or self-test paths, without holding that lock.

Suppose such a caller has already fetched xgmiitorgmii_read_status() as
the function pointer and is preempted. Meanwhile the converter is
unbound through sysfs. The xgmiitorgmii_remove() added by the next
patch in this series restores phydev->drv to priv->phy_drv, and devres
frees priv.

When the caller resumes, container_of_const() is applied to the
original phy_driver rather than to conv_phy_drv. priv is then a pointer
into unrelated memory. priv->phy_drv->read_status is loaded through it
and called.

xgmiitorgmii_remove() takes phydev->lock, but that only serializes
against callers that take the same lock. It does not close this window
for lockless callers.

Is the intent that every phy_read_status() caller must hold
phydev->lock when this converter is in use? If not, how should the
callbacks recover priv safely once phydev->drv can change under a
running callback?

[Severity: High]
This is a pre-existing issue, but is phydev->drv still valid here after
the converter has been unbound?

xgmiitorgmii_probe() allocates priv with devm_kzalloc() on the converter's
own device. It then installs &priv->conv_phy_drv as the attached PHY's
phydev->drv.

The driver has no .remove callback, so nothing restores the original
driver:

static struct mdio_driver xgmiitorgmii_driver = {
	.probe	= xgmiitorgmii_probe,
	...
};

After a sysfs unbind or rmmod of the converter, devres frees priv, but
phydev->drv still points into it.

The next call through phy_state_machine()->phy_read_status() would reach
this function. Here container_of_const() resolves into freed memory, and
priv->phy_drv and priv->mdio are then dereferenced.

A rebind would also memcpy() from the freed copy in xgmiitorgmii_probe().

The next patch in this series, "net: phy: xilinx-gmii2rgmii: Restore PHY
driver on remove", adds xgmiitorgmii_remove(). Under phydev->lock, that
function sets priv->phy_dev->drv back to priv->phy_drv, so this appears
to be fixed there.

[ ... ]

> @@ -123,7 +127,6 @@ static int xgmiitorgmii_probe(struct mdio_device *mdiodev)
>  	       sizeof(struct phy_driver));
>  	priv->conv_phy_drv.read_status = xgmiitorgmii_read_status;
>  	priv->conv_phy_drv.set_loopback = xgmiitorgmii_set_loopback;
> -	mdiodev_set_drvdata(&priv->phy_dev->mdio, priv);
>  	priv->phy_dev->drv = &priv->conv_phy_drv;

[Severity: Critical]
Does deriving priv from phydev->drv make the wrapper callbacks depend
on a pointer that nothing synchronizes?

xgmiitorgmii_probe() installs &priv->conv_phy_drv here with a plain
store, without taking phydev->lock. The attached PHY may already be
connected to a MAC with its state machine running. In that case
phydev->drv is rewritten while other code is using it.

With this patch, xgmiitorgmii_read_status() and
xgmiitorgmii_set_loopback() no longer get priv from a field owned by
the converter. They apply container_of_const() to phydev->drv, which
they reload on entry, separately from the load the caller used to pick
the callback.

That is only correct while phydev->drv still equals
&priv->conv_phy_drv. If it has been switched to another phy_driver in
the meantime, the result is that driver's address minus
offsetof(struct gmii2rgmii, conv_phy_drv). The remove path added in
the next patch, which restores the original driver, is one way this
can happen. The resulting pointer is not a struct gmii2rgmii at all.

priv->phy_drv is then read from whatever memory precedes that
phy_driver, and priv->phy_drv->read_status(phydev) becomes an indirect
call through an arbitrary pointer. Wouldn't that crash the kernel, or
let the call be steered somewhere else entirely?

Even within this patch alone, phy_remove() clears phydev->drv when the
attached PHY's own driver is unbound. Could a caller that is already
inside xgmiitorgmii_read_status() then compute priv from NULL?

Could the swap of phydev->drv be done under phydev->lock, with every
path that calls into these callbacks holding it as well? Or could priv
be found in a way that does not depend on the current value of
phydev->drv?

[Severity: Medium]
This isn't a bug introduced by this patch, but is this store ordered
after the setup of the wrapper?

The store to priv->phy_dev->drv is a plain store, done without
phydev->lock and without smp_store_release(). It comes after the stores to
priv->mdio and priv->phy_drv and the memcpy() into conv_phy_drv.

The PHY may already be attached with its state machine running, for
example when the converter module loads after the MAC opened the
interface. On a weakly ordered CPU such as the arm64 Zynq parts, could
another CPU see the new phydev->drv before those earlier stores?

If so, phy_read_status() or phy_loopback() would call
xgmiitorgmii_read_status() or xgmiitorgmii_set_loopback(). Those could
read priv->phy_drv or priv->mdio while they are still NULL from
devm_kzalloc().

>  
>  	return 0;

[Severity: Medium]
This is a pre-existing issue, but does a successful probe leak the
reference on the phy_device?

	priv->phy_dev = of_phy_find_device(phy_node);

of_phy_find_device() takes a device reference. That reference is only
dropped on the !priv->phy_dev->drv deferral path.

There is no teardown callback, so nothing calls
put_device(&priv->phy_dev->mdio.dev) after a successful probe. Each
bind/unbind cycle would leak one reference and keep the phy_device from
being freed.

The next patch in this series, "net: phy: xilinx-gmii2rgmii: Restore PHY
driver on remove", appears to fix this by calling put_device() in the new
xgmiitorgmii_remove().

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261007054844.529363-1-vineeth.karumanchi%40amd.com