Re: [net-next,v15,06/13] net: mdio: realtek-rtl9300: suppress sysfs bind/unbind attributes

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

Critical: 0 · High: 1 · Medium: 0 · Low: 0

- [High] Incomplete fix: `.suppress_bind_attrs = true` in
  `otto_emdio_driver` (drivers/net/mdio/mdio-realtek-rtl9300.c:847)…

--

net: mdio: realtek-rtl9300: suppress sysfs bind/unbind attributes

This sets suppress_bind_attrs in otto_emdio_driver so the driver cannot be
unbound through sysfs. The stated goal is to mitigate a use-after-free of
the devm allocated otto_emdio_priv structure, which the reference counted
mii_bus objects keep pointing at after the controller devres is released.

[Severity: High]

Does this flag actually cover the paths named in the commit message?  In
drivers/base/bus.c the only consumers are bus_add_driver() and
bus_remove_driver():

	if (!drv->suppress_bind_attrs) {
		error = add_bind_files(drv);

plus the CONFIG_DEBUG_TEST_DRIVER_REMOVE check in drivers/base/dd.c.  So
the sysfs bind/unbind files disappear, but device_release_driver() and the
devres unwind can still be reached by other means.

The commit message names the probe failure case as a trigger:

    "A corrupt devicetree might produce a load failure and a cleanup too."

That path is still open.  otto_emdio_probe() registers buses one child at a
time and returns on the first failure:

	device_for_each_child_node_scoped(dev, child) {
		err = otto_emdio_probe_one(dev, priv, child);
		if (err)
			return err;
	}

If the second child hits "undefined smi bus number" or
"illegal (dangling) smi bus number", the first bus was already published by
devm_of_mdiobus_register(), and the driver core then releases priv (and
destroys priv->lock) while the mii_bus can survive on a phy_device
reference held by a MAC/DSA consumer.

Can the bus callbacks then run against the freed priv?  otto_emdio_probe_one()
stores a raw back pointer into the refcounted bus:

	chan = bus->priv;
	chan->mdio_bus = mdio_bus;
	chan->priv = priv;

and otto_emdio_bus_to_priv() hands it back with no lifetime or bus->state
check, so otto_emdio_read_c22() and friends do:

	scoped_guard(mutex, &priv->lock)
		ret = priv->info->read_c22(bus, port, regnum, &value);

which is a mutex_lock plus an indirect call through freed memory.  Note that
phy_detach_internal() in drivers/net/phy/phy_device.c calls the new hook
unconditionally:

	if (notify_bus && phydev->mdio.bus->notify_phy_detach)
		phydev->mdio.bus->notify_phy_detach(phydev);

and __mdiobus_read()/__mdiobus_write() never test bus->state either.

The commit message also states:

    "The driver is usually integrated into the kernel and not loaded as a
    module."

Is that enforced anywhere?  drivers/net/mdio/Kconfig has:

	config MDIO_REALTEK_RTL9300
		tristate "Realtek RTL9300 MDIO interface support"

and the file still ends with module_platform_driver(otto_emdio_driver) and
MODULE_DEVICE_TABLE(), so rmmod reaches driver_unregister() ->
driver_detach() and the same devres teardown.  Removal of the parent node
via of_platform_depopulate() has the same effect.

Given that, should the Fixes: tag and "This mitigates issues for the known
use cases" be softened, or would one of the other two options listed in the
commit message (making bus->priv self contained, or gating the callbacks and
the notify hooks on bus->state) be the change to make instead?  Those would
replace this line rather than build on it.

The end of the series (c965284c5acebf7c11c9214f77cbc28873491728) still has
devm_kzalloc() for priv, chan->priv = priv and module_platform_driver(), so
no later patch changes the ownership model.

 	},
 };

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260831143439.2404484-1-markus.stockhausen%40gmx.de