Re: [PATCH net-next v10 04/10] net: mdio: realtek-rtl9300: extend controller structure lifetime

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Use-After-Free in error path when `devm_add_action_or_reset` fails.
- [High] The patch fails its objective because devres on `bus->dev` executes synchronously during unregistration, leaving the original UAF unfixed.
--

--- Patch [4]: [PATCH net-next v10 04/10] net: mdio: realtek-rtl9300: extend controller structure lifetime ---
commit a4fb5c9c89a12fe9697736a00e823a9f53ad38ca
Author: Markus Stockhausen <markus.stockhausen@gmx.de>

net: mdio: realtek-rtl9300: extend controller structure lifetime
    
Currently, the driver allocates the otto_emdio_priv structure using
devm_kzalloc(), tightly coupling its lifetime to the platform device.
Additionally the driver registers up to four MDIO buses. During unbind
or removal the devm teardown process might free the controller private
data before the MDIO buses are fully unregistered. This can lead to a
use-after-free if the MDIO subsystem accesses bus-specific operations
(which rely on controller->priv) during the teardown phase.

> diff --git a/drivers/net/mdio/mdio-realtek-rtl9300.c b/drivers/net/mdio/mdio-realtek-rtl9300.c
> index 73ac5fdcd2674..7dcfb72ed8188 100644
> --- a/drivers/net/mdio/mdio-realtek-rtl9300.c
> +++ b/drivers/net/mdio/mdio-realtek-rtl9300.c
[ ... ]
> @@ -621,6 +638,11 @@ static int otto_emdio_probe_one(struct device *dev, struct otto_emdio_priv *priv
>  	if (err)
>  		return dev_err_probe(dev, err, "cannot register MDIO bus\n");
>  
> +	kref_get(&priv->kref);
> +	err = devm_add_action_or_reset(&bus->dev, otto_emdio_cleanup_bus, priv);

[Severity: High]
Does registering this devm action on bus->dev defeat the purpose of the
kref decoupling?

The commit message states the goal is to decouple the lifetime of the private
data and free it after the final reference has been removed. However, devres
actions attached to a device are flushed synchronously inside device_del(),
not when the final reference is dropped via device_release().

Because mdiobus_unregister() calls device_del(), the devres action is flushed
synchronously during unregistration, dropping the kref exactly as it was
before this patch. If an external entity holds a reference to the bus and
accesses it after unregistration, won't it still hit a use-after-free on the
freed priv?

> +	if (err)
> +		return dev_err_probe(dev, err, "cannot register cleanup action\n");
> +
>  	return 0;
>  }
[ ... ]
> @@ -731,44 +753,54 @@ static int otto_emdio_probe(struct platform_device *pdev)
[ ... ]
>  	device_for_each_child_node_scoped(dev, child) {
>  		err = otto_emdio_probe_one(dev, priv, child);
>  		if (err)
> -			return err;
> +			goto err_out;
> +		buses_registered++;
>  	}
>  
> -	return 0;
> +	if (!buses_registered)
> +		err = dev_err_probe(dev, -ENODEV, "no MDIO buses registered\n");
> +err_out:
> +	kref_put(&priv->kref, otto_emdio_cleanup_controller);

[Severity: High]
If devm_add_action_or_reset() fails in otto_emdio_probe_one(), will this
cause a use-after-free during error unwinding?

When devm_add_action_or_reset() fails, it executes the callback immediately,
dropping the acquired kref. The probe function then returns an error, which
causes otto_emdio_probe() to jump here and drop the initial kref.

Since the refcount hits 0, priv is freed. However, the bus was just
registered successfully prior to the failure. The devres unwinding will
subsequently unregister the bus via mdiobus_unregister(). If any PHY teardown
logic accesses the MDIO bus during unregistration, it will invoke bus->write
(otto_emdio_write_c22), which accesses the already-freed priv->lock.

> +
> +	return err;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260731061400.78301-1-markus.stockhausen@gmx.de?part=4