[PATCH net v2 0/2] net: phy: xilinx-gmii2rgmii: Fix PHY data ownership and removal

[PATCH net v2 0/2] net: phy: xilinx-gmii2rgmii: Fix PHY data ownership and removal

[Vineeth Karumanchi]

The Xilinx GMII-to-RGMII converter copies the attached PHY driver and
replaces its read_status and set_loopback callbacks. This series addresses
two problems in that arrangement: overwriting driver data belonging to
the external PHY, and leaving phydev->drv pointing at freed converter
memory after converter removal.

Patch 1 retrieves the converter private data from its embedded phy_driver
using container_of_const(), preserving the external PHY's MDIO driver-data
field. It fixes the overwrite introduced by commit 168f7a161608 ("net: phy:
gmii2rgmii: Dont use priv field in phy device").

Patch 2 stores private data on the converter's own MDIO device and adds a
remove callback. It restores the original PHY driver under phydev->lock
only if the converter's copy is still installed, and releases the reference
acquired by of_phy_find_device(). This addresses the stale pointer observed
during PHY state-machine polling after converter-only unbind. The missing
removal cleanup dates back to commit f411a6160bd4 ("net: phy: Add
gmiitorgmii converter support"). Apply the patches in order.

Changes in v2:
- Patch 1: unchanged.
- Patch 2: restore the original PHY driver only if phydev->drv still
  points to the converter's copy.
- Patch 2: update the comment and commit message to describe serialization
  against PHY callbacks that hold phydev->lock.

Link to v1: https://lore.kernel.org/netdev/20261001074718.3944521-1-vineeth.karumanchi@amd.com/

Vineeth Karumanchi (2):
  net: phy: xilinx-gmii2rgmii: Avoid overwriting PHY drvdata
  net: phy: xilinx-gmii2rgmii: Restore PHY driver on remove

 drivers/net/phy/xilinx_gmii2rgmii.c | 29 +++++++++++++++++++++++++----
 1 file changed, 25 insertions(+), 4 deletions(-)


base-commit: 23609bce9e1de525d1d0e73fc68c6e7971d0b49e
-- 
2.43.0

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

[Vineeth Karumanchi]

The GMII-to-RGMII converter wraps the attached PHY driver by copying
its phy_driver structure and replacing the read_status and set_loopback
callbacks.

To access the converter private data from these callbacks, the driver
currently stores it in the attached PHY's MDIO driver-data field. This
field belongs to the underlying PHY driver and may already contain its
private data. Overwriting it can therefore cause the PHY driver to
retrieve an unexpected pointer and behave incorrectly.

The converter-specific phy_driver is embedded in struct gmii2rgmii and
installed as phydev->drv. Use container_of_const() to retrieve the
enclosing gmii2rgmii structure from phydev->drv instead of using the
PHY's driver-data field.

Update xgmiitorgmii_configure() to accept a const pointer accordingly.

Fixes: 168f7a161608 ("net: phy: gmii2rgmii: Dont use priv field in phy device")
Signed-off-by: Vineeth Karumanchi <vineeth.karumanchi@amd.com>
---
 drivers/net/phy/xilinx_gmii2rgmii.c | 11 +++++++----
 1 file changed, 7 insertions(+), 4 deletions(-)

diff --git a/drivers/net/phy/xilinx_gmii2rgmii.c b/drivers/net/phy/xilinx_gmii2rgmii.c
index 2024d8ef36d9..61f71e977a57 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;
@@ -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)
@@ -67,7 +69,9 @@ static int xgmiitorgmii_read_status(struct phy_device *phydev)
 static int xgmiitorgmii_set_loopback(struct phy_device *phydev, bool enable,
 				     int speed)
 {
-	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->set_loopback)
@@ -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;
 
 	return 0;
-- 
2.43.0

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

[netdev-bot+sashiko]

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

[PATCH net v2 2/2] net: phy: xilinx-gmii2rgmii: Restore PHY driver on remove

[Vineeth Karumanchi]

The GMII-to-RGMII converter replaces phydev->drv with a modified copy
of the attached PHY driver. This copied driver is embedded in the
converter's private data and is released when the converter is
removed.

Without a remove callback, phydev->drv continues to point to the freed
copy after the converter is unbound. A subsequent PHY operation can
dereference this stale pointer and result in a use-after-free.

With Generic KASAN enabled, unbinding only the converter while the
external PHY remains active produces the following report (abridged):

  BUG: KASAN: slab-use-after-free in phy_check_link_status+0x2d8/0x338
  Read of size 8 at addr ffff000006c359b0 by task kworker/2:0/27
  Workqueue: events_power_efficient phy_state_machine
  Call trace:
   phy_check_link_status+0x2d8/0x338
   _phy_state_machine+0xdc/0xa4c
   phy_state_machine+0x2c/0x70
   process_one_work+0x554/0xe44
   worker_thread+0x6d0/0x1180
   kthread+0x2e8/0x5d4
   ret_from_fork+0x10/0x20

  Allocated by task 55:
   ...
   devm_kmalloc+0xac/0x2ac
   xgmiitorgmii_probe+0xa0/0x37c
   mdio_probe+0x68/0xb4
   ...

  Freed by task 642:
   ...
   kfree+0x14c/0x38c
   release_nodes+0xb4/0x1e0
   devres_release_all+0x140/0x1f4
   device_unbind_cleanup+0x20/0x190
   device_release_driver_internal+0x344/0x460
   device_driver_detach+0x3c/0x54
   unbind_store+0xe0/0xf8
   ...

Store the converter private data in its own MDIO device and add a
remove callback. Restore the attached PHY's original driver only if
phydev->drv still points to the converter's copy. Hold phydev->lock to
serialize the restore against PHY callbacks that take that mutex.

Also release the device reference acquired by of_phy_find_device().

Fixes: f411a6160bd4 ("net: phy: Add gmiitorgmii converter support")
Signed-off-by: Vineeth Karumanchi <vineeth.karumanchi@amd.com>
---
Changes in v2:
- Restore the original PHY driver only if phydev->drv still points to
  the converter's copy.
- Update the comment and commit message to describe serialization against
  PHY callbacks that hold phydev->lock.

Link to v1: https://lore.kernel.org/netdev/20261001074718.3944521-1-vineeth.karumanchi@amd.com/

 drivers/net/phy/xilinx_gmii2rgmii.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/net/phy/xilinx_gmii2rgmii.c b/drivers/net/phy/xilinx_gmii2rgmii.c
index 61f71e977a57..9dcdb91cd441 100644
--- a/drivers/net/phy/xilinx_gmii2rgmii.c
+++ b/drivers/net/phy/xilinx_gmii2rgmii.c
@@ -128,10 +128,27 @@ static int xgmiitorgmii_probe(struct mdio_device *mdiodev)
 	priv->conv_phy_drv.read_status = xgmiitorgmii_read_status;
 	priv->conv_phy_drv.set_loopback = xgmiitorgmii_set_loopback;
 	priv->phy_dev->drv = &priv->conv_phy_drv;
+	mdiodev_set_drvdata(mdiodev, priv);
 
 	return 0;
 }
 
+static void xgmiitorgmii_remove(struct mdio_device *mdiodev)
+{
+	struct gmii2rgmii *priv = mdiodev_get_drvdata(mdiodev);
+
+	/*
+	 * Restore the original driver only if the converter's copy is still
+	 * installed. Serialize against PHY callbacks that hold phydev->lock.
+	 */
+	mutex_lock(&priv->phy_dev->lock);
+	if (priv->phy_dev->drv == &priv->conv_phy_drv)
+		priv->phy_dev->drv = priv->phy_drv;
+	mutex_unlock(&priv->phy_dev->lock);
+
+	put_device(&priv->phy_dev->mdio.dev);
+}
+
 static const struct of_device_id xgmiitorgmii_of_match[] = {
 	{ .compatible = "xlnx,gmii-to-rgmii-1.0" },
 	{},
@@ -140,6 +157,7 @@ MODULE_DEVICE_TABLE(of, xgmiitorgmii_of_match);
 
 static struct mdio_driver xgmiitorgmii_driver = {
 	.probe	= xgmiitorgmii_probe,
+	.remove	= xgmiitorgmii_remove,
 	.mdiodrv.driver = {
 		.name = "xgmiitorgmii",
 		.of_match_table = xgmiitorgmii_of_match,
-- 
2.43.0

Re: [PATCH net v2 2/2] net: phy: xilinx-gmii2rgmii: Restore PHY driver on remove

[netdev-bot+sashiko]

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

Critical: 0 · High: 3 · Medium: 1 (1) · 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).

- [High] The fix only serializes against code that holds phydev->lock, so
  the use-after-free it targets is still reachable.
- [High] phy: data race modifying phydev->drv without lock in probe
- [High] net: phy: missing phydev->lock in xgmiitorgmii_probe allows data
  race
- [Medium] The check-then-restore in xgmiitorgmii_remove() runs under
  phydev->lock, but phy_remove()…

Pre-existing issues:
- [Medium] xgmiitorgmii_probe() memcpy()s the PHY driver into
  priv->conv_phy_drv and then publishes `priv->phy_dev->drv =…

--

net: phy: xilinx-gmii2rgmii: Restore PHY driver on remove

This adds a remove callback to the GMII-to-RGMII converter driver. The
callback restores the attached PHY's original phydev->drv under
phydev->lock, but only if the converter's embedded copy is still
installed. It also drops the device reference that of_phy_find_device()
took in probe.

> diff --git a/drivers/net/phy/xilinx_gmii2rgmii.c b/drivers/net/phy/xilinx_gmii2rgmii.c
> index 61f71e977a574..9dcdb91cd441f 100644
> --- a/drivers/net/phy/xilinx_gmii2rgmii.c
> +++ b/drivers/net/phy/xilinx_gmii2rgmii.c
> @@ -128,10 +128,27 @@ static int xgmiitorgmii_probe(struct mdio_device *mdiodev)
>  	priv->conv_phy_drv.read_status = xgmiitorgmii_read_status;
>  	priv->conv_phy_drv.set_loopback = xgmiitorgmii_set_loopback;
>  	priv->phy_dev->drv = &priv->conv_phy_drv;

[Severity: Medium]
This is a pre-existing issue and was not introduced by this patch.
xgmiitorgmii_probe() memcpy()s the PHY driver into priv->conv_phy_drv.
It then publishes the pointer with a plain store. It doesn't hold
phydev->lock and has no release barrier (smp_store_release() or
rcu_assign_pointer()). The new restore in xgmiitorgmii_remove() does
take the lock.

Probe only requires phydev->drv to be non-NULL, so the PHY may already
be attached with its state machine running.

On a weakly ordered CPU such as arm64 Zynq MPSoC, could a reader see the
new phydev->drv before the copied contents of conv_phy_drv? Could it
then call through an uninitialized function pointer?

Nobody has shown this ordering problem in practice, and the patch
doesn't change this sequence.

> +	mdiodev_set_drvdata(mdiodev, priv);
>  
>  	return 0;
>  }
>  
> +static void xgmiitorgmii_remove(struct mdio_device *mdiodev)
> +{
> +	struct gmii2rgmii *priv = mdiodev_get_drvdata(mdiodev);
> +
> +	/*
> +	 * Restore the original driver only if the converter's copy is still
> +	 * installed. Serialize against PHY callbacks that hold phydev->lock.
> +	 */
> +	mutex_lock(&priv->phy_dev->lock);
> +	if (priv->phy_dev->drv == &priv->conv_phy_drv)
> +		priv->phy_dev->drv = priv->phy_drv;

[Severity: High]
Separately from mutual exclusion, the unlocked store in
xgmiitorgmii_probe() leaves nothing ordering the construction of the
copy against its publication. Probe memcpy()s the PHY driver into
priv->conv_phy_drv, overrides read_status and set_loopback, and then
assigns &priv->conv_phy_drv to phydev->drv with a plain store. There
is no smp_store_release(), and no mutex_unlock() that would give the
store release semantics.

priv comes from devm_kzalloc(), so conv_phy_drv starts out zeroed. On
a weakly ordered CPU, such as the arm64 Zynq MPSoC parts this IP is
used with, a reader holding phydev->lock on another CPU, for example
phy_read_status() from phy_check_link_status(), could observe the new
phydev->drv before all of the copied fields are visible. It would then
see zeroed or partially copied callbacks and flags for that call,
silently falling back to genphy behaviour, skipping the PHY driver's
hooks, or bypassing xgmiitorgmii_read_status() altogether.

With this patch the realistic trigger is rebinding the converter while
the PHY is attached and running, since remove now makes that sequence
work. Taking phydev->lock across the read of phydev->drv, the memcpy()
and the store in probe would give lock-holding readers the ordering
they need, mirroring what remove does. Readers that load phydev->drv
without the lock would still need something stronger. Is there a
reason probe was left unlocked in v2?

[Severity: High]
The new xgmiitorgmii_remove() takes phydev->lock so that restoring
phydev->drv is serialized against PHY callbacks that hold that mutex.
xgmiitorgmii_probe() writes the same field in the other direction,
replacing priv->phy_dev->drv with &priv->conv_phy_drv, but it does so
without taking the lock.

Before this patch, unbinding the converter left phydev->drv dangling,
so a later rebind wasn't something that could work anyway. Now that
remove puts the original driver back, unbind followed by bind through
sysfs becomes a sequence people will actually use. There is no device
link tying the converter's probe to the PHY's attach state, so on
rebind the PHY can already be attached to an interface that is up,
with phy_state_machine() running on another CPU.

In that case probe changes phydev->drv while phy_check_link_status()
holds phydev->lock and is inside phy_read_status(), which loads
phydev->drv more than once. A single locked call can then test the
callback on one driver and invoke it through another, which is the
kind of mid-call switch the locking in remove exists to prevent.

Should xgmiitorgmii_probe() hold phydev->lock around reading
phydev->drv, the memcpy() into conv_phy_drv and the store, so that both
directions follow the rule described in the new comment in remove?

[Severity: Medium]
Can this check-then-restore race with phy_remove() on the external PHY?
phy_remove() writes phydev->drv without taking phydev->lock:

drivers/net/phy/phy_device.c:phy_remove() {
    ...
	if (phydev->drv && phydev->drv->remove)
		phydev->drv->remove(phydev);
    ...
	phydev->drv = NULL;
    ...
}

The converter and the external PHY are separate mdio_devices.
mdio_bus_type does not set need_parent_lock, so the driver core holds
only each device's own device_lock. Sysfs unbinds of the two devices can
therefore run at the same time.

Consider this interleaving:

converter                             PHY
xgmiitorgmii_remove()
  drv == &priv->conv_phy_drv (true)
                                      phy_remove()
                                        phydev->drv = NULL;
  phydev->drv = priv->phy_drv;

The PHY is now unbound, but phydev->drv still points at its old driver.
That pointer dangles once the driver's module is unloaded, and later
if (!phydev->drv) checks in phylib would follow it.

The reverse order has a problem too. phy_remove() can be calling through
the converter copy via phydev->drv->remove while xgmiitorgmii_remove()
restores the pointer and returns, and devres then frees priv. Is that
also a use-after-free?

> +	mutex_unlock(&priv->phy_dev->lock);
> +
> +	put_device(&priv->phy_dev->mdio.dev);
> +}

[Severity: High]
Does this fully close the use-after-free described in the commit
message? The restore only serializes against code that holds
phydev->lock.

After xgmiitorgmii_remove() returns,
device_unbind_cleanup()->devres_release_all() frees priv, and the
embedded conv_phy_drv goes with it. Nothing waits for phylib code that
loaded phydev->drv without the lock, and there is no RCU or refcount
protecting it.

One example is ip link set dev X down, which goes through phy_stop():

drivers/net/phy/phy.c:phy_stop() {
    ...
	state_work = _phy_state_machine(phydev);
	mutex_unlock(&phydev->lock);

	_phy_state_machine_post_work(phydev, state_work);
    ...
}

For PHY_HALTED this returns PHY_STATE_WORK_SUSPEND, so phy_suspend()
runs after the lock has been dropped:

drivers/net/phy/phy_device.c:phy_suspend() {
	const struct phy_driver *phydrv = phydev->drv;
    ...
	if (phydev->wol_enabled && !(phydrv->flags & PHY_ALWAYS_CALL_SUSPEND))
		return -EBUSY;
    ...
	ret = phydrv->suspend(phydev);
    ...
}

Suppose the converter is unbound after phy_suspend() loads phydev->drv
but before it uses phydrv. Wouldn't phy_suspend() then read freed slab
memory and make an indirect call through it? phy_detach()->phy_suspend()
has the same window.

Other functions that read phydev->drv without the lock look similar:

  - phy_ethtool_get_sset_count() checks get_sset_count, get_strings and
    get_stats through phydev->drv before taking phydev->lock
  - phy_ethtool_get_wol() reads drv->get_wol before taking the mutex
  - phy_init_hw() dereferences drv->soft_reset, config_init and
    config_intr without the lock. It is reachable from
    phy_attach_direct(), from resume, and from the SIOCSMIIREG
    BMCR_RESET path in phy_mii_ioctl()
  - phy_remove() reads drv->remove without the lock

RTNL doesn't help here either, because unbinding the converter never
takes it.

With this patch, the long-lived dangling pointer becomes a narrow race
window, but it is still there. Would a full fix need to keep the copy
alive for as long as phylib can use it? Or would every phydev->drv
reader need to synchronize with the converter's removal?

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