Re: [PATCH net-next v3 1/2] net: phy: sfp: free mii_bus in sfp_i2c_mdiobus_destroy

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The delayed works (`sfp->poll` and `sfp->timeout`) can execute after their dependent GPIO descriptors and IRQs are released, leading to Use-After-Free.
--

commit 318c6861220a57086d547ce894cf1c029a4d3639
Author: Petr Wozniak <petr.wozniak@gmail.com>

net: phy: sfp: free mii_bus in sfp_i2c_mdiobus_destroy

sfp_i2c_mdiobus_create() allocates the I2C MDIO bus with mdio_i2c_alloc(),
a plain (non-devm) allocation, and registers it. sfp_i2c_mdiobus_destroy()
only unregisters the bus and clears sfp->i2c_mii without calling
mdiobus_free(). As the only reference to the bus is then cleared, the
struct mii_bus is leaked.

> diff --git a/drivers/net/phy/sfp.c b/drivers/net/phy/sfp.c
> index 03bfd8640db96..c4d274ab651e3 100644
> --- a/drivers/net/phy/sfp.c
> +++ b/drivers/net/phy/sfp.c
> @@ -963,6 +963,7 @@ static int sfp_i2c_mdiobus_create(struct sfp *sfp)
>  static void sfp_i2c_mdiobus_destroy(struct sfp *sfp)
>  {
>  	mdiobus_unregister(sfp->i2c_mii);
> +	mdiobus_free(sfp->i2c_mii);
>  	sfp->i2c_mii = NULL;
>  }

[Severity: High]
This isn't a bug introduced by this patch, but while reviewing the teardown
path I noticed a potential use-after-free during device unbind or module
removal.

In sfp_probe(), sfp_cleanup() is registered before allocating the GPIO
descriptors:

drivers/net/phy/sfp.c:sfp_probe() {
    ...
	err = devm_add_action_or_reset(sfp->dev, sfp_cleanup, sfp);
	if (err < 0)
		return err;
    ...
	for (i = 0; i < GPIO_MAX; i++)
		if (sff->gpios & BIT(i)) {
			sfp->gpio[i] = devm_gpiod_get_optional(sfp->dev,
					   gpio_names[i], gpio_flags[i]);
    ...
}

During unbind, devres runs cleanup actions in reverse LIFO order, meaning
the GPIO descriptors are freed first.

sfp_remove() doesn't cancel sfp->poll. If sfp->need_poll is true, the poll
worker could fire and access the freed GPIO descriptors before sfp_cleanup()
finally cancels it:

drivers/net/phy/sfp.c:sfp_cleanup() {
    ...
	cancel_delayed_work_sync(&sfp->poll);
	cancel_delayed_work_sync(&sfp->timeout);
    ...
}

Could this lead to a panic when unbinding the device or removing the module?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260623080538.7646-1-petr.wozniak@gmail.com?part=1

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