Re: [PATCH net 1/2] net: usb: smsc95xx: propagate MDIO reset errors

[netdev-bot+sashiko@kernel.org]

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

Critical: 1 · High: 0 · Medium: 0 · 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] The patch makes smsc95xx_mdiobus_reset()
  (drivers/net/usb/smsc95xx.c) return `ret` instead of 0.

--

net: usb: smsc95xx: propagate MDIO reset errors

This patch changes smsc95xx_mdiobus_reset() so it returns the result of
the PM_CTRL register accesses instead of always returning 0. The goal is
to report a failed external PHY reset to the caller.

> diff --git a/drivers/net/usb/smsc95xx.c b/drivers/net/usb/smsc95xx.c
> index 130a460c1f546..f85ee5f8a5394 100644
> --- a/drivers/net/usb/smsc95xx.c
> +++ b/drivers/net/usb/smsc95xx.c
> @@ -291,7 +291,7 @@ static int smsc95xx_mdiobus_reset(struct mii_bus *bus)
>  reset_out:
>  	mutex_unlock(&dev->phy_mutex);
>  
> -	return 0;
> +	return ret;
>  }

[Severity: Critical]
What value does ret hold here when the reset succeeds?

smsc95xx_write_reg() passes back the result of usbnet_write_cmd()
unchanged. That result is the byte count from usb_control_msg():

drivers/net/usb/smsc95xx.c:smsc95xx_write_reg() {
    ...
	ret = fn(dev, USB_VENDOR_REQUEST_WRITE_REGISTER, USB_DIR_OUT
		 | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
		 0, index, &buf, 4);
	if (ret < 0 && ret != -ENODEV)
		netdev_warn(dev->net, "Failed to write reg index 0x%08x: %d\n",
			    index, ret);

	return ret;
}

smsc95xx_read_reg() also returns 4 on success. After a successful
external PHY reset, both "if (ret < 0)" checks pass, and the function
now returns 4 instead of 0.

__mdiobus_register() treats any non-zero value as a failure:

drivers/net/phy/mdio_bus_provider.c:__mdiobus_register() {
    ...
	if (bus->reset) {
		err = bus->reset(bus);
		if (err)
			goto error_reset_gpiod;
	}
    ...
}

It calls device_del() and returns 4. smsc95xx_bind() then takes its
error path:

	ret = mdiobus_register(pdata->mdiobus);
	if (ret) {
		netdev_err(dev->net, "Could not register MDIO bus\n");
		goto free_mdio;
	}

That path runs mdiobus_free(), irq_dispose_mapping(),
irq_domain_remove(), irq_domain_free_fwnode() and kfree(pdata). It
then returns 4 and leaves dev->driver_priv pointing at the freed pdata.

usbnet_probe() only aborts on negative values:

	status = info->bind(dev, udev);
	if (status < 0)
		goto out1;

Probe therefore goes on to register_netdev() with these leftovers:

  - dev->driver_priv pointing at freed memory
  - a freed mii_bus
  - a NULL dev->net->phydev

Wouldn't later driver callbacks then use pdata after it was freed? One
example is the pdata->pm_task dereference in smsc95xx_read_reg() and
smsc95xx_write_reg().

smsc95xx_unbind() runs on disconnect, or when probe fails after bind
and calls info->unbind. In that case it would:

  - call phy_disconnect() on a NULL phydev
  - call mdiobus_unregister() and mdiobus_free() on the freed mii_bus
  - free the irqdomain and fwnode a second time
  - call kfree(pdata) a second time

Can this double free pdata?

This path is taken whenever HW_CFG_PSEL_ is set. That seems to break
every probe of a device with an external PHY, which is the setup
809ff97a677f was fixing. The bit is read from the device, so an
emulated or malicious USB device could trigger this as well.

The next patch in the series, "net: usb: smsc95xx: propagate MDIO write
errors", does not touch smsc95xx_mdiobus_reset(). The problem would
still be there at the end of the series.

Would something like "return ret < 0 ? ret : 0;", or setting ret = 0
after the successful write, keep the error propagation and still return
0 on success?

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20261114050954.275495-1-artem122novak%40gmail.com