Re: [PATCH net-next v21 04/13] ax88179_178a: Add HW support for AX179A-based chips

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 9 potential
issues to consider:
- [High] net: usb: ax88179_178a: missing usbnet_link_change() in phylink
  callbacks
- [High] net: usb: ax88179: UAF on unbind when down due to
  FLAG_AVOID_UNLINK_URBS
- [Medium] net: usb: ax88179_178a: byte-swapped pause waterlevels in
  ax88179a_mac_link_up()
- [Medium] net: usb: ax88179_178a: positive return from
  ax88179_mdiobus_write() and c45
- [Medium] net: usb: ax88179a: ax88179a_reset() clobbers custom MAC and
  perm_addr on open
- [Medium] net: usb: ax88179_178a: missing wol_supported init in
  ax88179a_bind()
- [Medium] net: usb: ax88179_178a: phylink_start() not unwound if
  usbnet_open() fails
- [Medium] net: usb: ax88179: ax88179a_stop() powers off PHY while
  attached to phylink
- [Low] net: usb: ax88179a: unaligned u16 pointer cast of u8 buf[5] in
  ax88179a_reset()
--

commit 2a91332d4bb64576008617e61fef57f6ed964420
Author: Birger Koblitz <mail@birger-koblitz.de>

ax88179_178a: Add HW support for AX179A-based chips

This commit adds hardware support and USB device IDs for the AX88179A,
AX88772D/E, and AX88279 USB Ethernet controllers using phylink.

> diff --git a/drivers/net/usb/ax88179a_devices.c b/drivers/net/usb/ax88179a_devices.c
> new file mode 100644
> index 0000000000000..1aa8479422517
> --- /dev/null
> +++ b/drivers/net/usb/ax88179a_devices.c
[ ... ]
> +static int ax88179_mdiobus_write(struct mii_bus *bus, int phy_id, int regnum, u16 val)
> +{
> +	struct usbnet *dev = bus->priv;
> +
> +	return ax88179_write_u16(dev, AX_ACCESS_PHY, phy_id, (__u16)regnum, val);
> +}
> +
> +static int ax179a_read_mmd(struct usbnet *dev, u16 dev_addr, u16 reg)
> +{
> +	u16 res;
> +	int ret;
> +
> +	ret = ax88179_read_cmd(dev, AX88179A_PHY_CLAUSE45, dev_addr, reg, 2, &res);
> +	if (ret < 0)
> +		return ret;
> +	return res;
> +}
> +
> +static int ax179a_write_mmd(struct usbnet *dev, u16 dev_addr, u16 reg, u16 data)
> +{
> +	return ax88179_write_cmd(dev, AX88179A_PHY_CLAUSE45, dev_addr, reg, 2, &data);
> +}
> +
> +static int ax88179_mdiobus_read_c45(struct mii_bus *bus, int addr, int devnum, int regnum)
> +{
> +	struct usbnet *dev = bus->priv;
> +
> +	if (addr != AX88179_PHY_ID)
> +		return -EINVAL;
> +
> +	return ax179a_read_mmd(dev, devnum, regnum);
> +}
> +
> +static int ax88179_mdiobus_write_c45(struct mii_bus *bus, int addr, int devnum,
> +				     int regnum, u16 val)
> +{
> +	struct usbnet *dev = bus->priv;
> +
> +	if (addr != AX88179_PHY_ID)
> +		return -EINVAL;
> +
> +	return ax179a_write_mmd(dev, devnum, regnum, val);
> +}

[Severity: Medium]
Do ax88179_mdiobus_write() and ax88179_mdiobus_write_c45() return 2 instead
of 0 on a successful write?

ax88179_write_u16() and ax88179_write_cmd() return the positive byte count
from usb_control_msg(), whereas mii_bus .write and .write_c45 callbacks are
expected to return 0 on success and a negative errno on failure.  Callers
that check for a non-zero return value, such as phylink_mii_ioctl() for
SIOCSMIIREG or phy_suspend() when a PHY driver returns phy_write()
directly, will treat the positive return value as an unusual return or
failure.

[ ... ]

> +static void ax88179a_mac_link_down(struct phylink_config *config,
> +				   unsigned int mode, phy_interface_t interface)
> +{
> +	/* Nothing to do */
> +}
> +
> +static void ax88179a_mac_link_up(struct phylink_config *config,
> +				 struct phy_device *phy,
> +				 unsigned int phy_mode, phy_interface_t interface,
> +				 int speed, int duplex,
> +				 bool tx_pause, bool rx_pause)
> +{
> +	struct usbnet *dev = netdev_priv(to_net_dev(config->dev));
> +	struct ax88179_data *ax179_data = dev->driver_priv;
> +	u8 link_sts = 0, reg8[3];
> +	u8 bulk_config_speed = 0;
> +	u16 mode;
> +
> +	/* Stop RX/TX for link configuration */
> +	ax88179_write_u16(dev, AX_ACCESS_MAC, AX_RX_CTL, 2, AX_RX_CTL_STOP);
> +	ax88179_write_u8(dev, AX_ACCESS_MAC, AX88179A_MAC_PATH, 1, 0);
> +
> +	ax88179_write_u8(dev, AX_ACCESS_MAC, AX88179A_MAC_CDC_DELAY_TX, 1, 0xa5);
> +
> +	ax88179_write_u16(dev, AX_ACCESS_MAC, AX_PAUSE_WATERLVL_LOW, 2, 0x0410);

[Severity: Medium]
Does this 16-bit write program AX_PAUSE_WATERLVL_LOW and
AX_PAUSE_WATERLVL_HIGH with their values swapped?

ax88179a_reset() writes 0x04 to AX_PAUSE_WATERLVL_LOW (0x54) and 0x10 to
AX_PAUSE_WATERLVL_HIGH (0x55), but ax88179_write_u16() converts 0x0410 to
little-endian via cpu_to_le16s(), sending 0x10 to register 0x54 and 0x04 to
register 0x55 so the low pause threshold exceeds the high threshold.

[ ... ]

> +	ax88179_write_u8(dev, AX_ACCESS_MAC, AX88179A_MAC_PATH, 1,
> +			 AX_MAC_RX_PATH_READY | AX_MAC_TX_PATH_READY);
> +}

[Severity: High]
Should ax88179a_mac_link_up() and ax88179a_mac_link_down() call
usbnet_link_change(dev, true/false, false) like ax88772_mac_link_up() and
ax88772_mac_link_down() in drivers/net/usb/asix_devices.c?

During usbnet_open(), ax88179a_reset() calls phylink_start(), which leaves
netif_carrier_ok(dev->net) false while PHY autonegotiation runs in the
phylink resolve workqueue.  When usbnet_open() then schedules dev->bh_work,
usbnet_bh() skips rx_alloc_submit() because carrier is off, leaving
dev->rxq empty:

drivers/net/usb/usbnet.c:usbnet_bh() {
    ...
	} else if (netif_running (dev->net) &&
		   netif_device_present (dev->net) &&
		   netif_carrier_ok(dev->net) &&
		   !timer_pending(&dev->delay) &&
		   !test_bit(EVENT_RX_KILL, &dev->flags) &&
		   !test_bit(EVENT_RX_HALT, &dev->flags)) {
		int	temp = dev->rxq.qlen;

		if (temp < RX_QLEN(dev)) {
			if (rx_alloc_submit(dev, GFP_ATOMIC) == -ENOLINK)
				return;
    ...
}

When phylink later brings the link up and calls netif_carrier_on(), usbnet
is never notified via usbnet_link_change(), so __handle_link_change() never
runs to schedule dev->bh_work and submit RX URBs or update max queue
lengths, and pending RX URBs are never unlinked when the link drops.

[ ... ]

> +static int ax88179a_bind(struct usbnet *dev, struct usb_interface *intf)
> +{
[ ... ]
> +	ret = ax88179a_init_phy(dev);
> +	if (ret)
> +		goto initphy_err;
> +
> +	return 0;

[Severity: Medium]
Should ax88179a_bind() initialize ax179_data->wol_supported before the
net_device is registered?

Unlike ax88179_bind(), which calls ax88179_reset() during bind,
ax88179a_bind() leaves ax179_data->wol_supported at 0 until usbnet_open()
calls ax88179a_reset().  Any ethtool get_wol or set_wol request issued
while the interface is still down before the first open (such as udev or
systemd-networkd applying WakeOnLan link settings at probe time) sees
wol_supported == 0 and fails with -EINVAL.

[ ... ]

> +static int ax88179a_reset(struct usbnet *dev)
> +{
> +	struct ax88179_data *ax179_data = dev->driver_priv;
> +	u16 *tmp16;
> +	u8 buf[5];
> +	u16 rxctl;
> +	u8 *tmp;
> +
> +	tmp16 = (u16 *)buf;
> +	tmp = (u8 *)buf;
[ ... ]
> +	/* Read MAC address from DTB or asix chip */
> +	ax88179_get_mac_addr(dev);
> +	memcpy(dev->net->perm_addr, dev->net->dev_addr, ETH_ALEN);

[Severity: Medium]
Does calling ax88179_get_mac_addr() and overwriting dev->net->perm_addr in
ax88179a_reset() clobber a user-configured MAC address or the permanent MAC
address on open?

ax88179_set_mac_addr() requires the interface to be down and writes the new
address to dev->net->dev_addr and AX_NODE_ID.  When usbnet_open() then
calls ax88179a_reset(), ax88179_get_mac_addr() either overwrites the custom
MAC address with the Device Tree MAC address if one is present, or reads
the custom MAC back from AX_NODE_ID, resets addr_assign_type to
NET_ADDR_PERM, and copies it into dev->net->perm_addr.

[ ... ]

> +	/* Configure default medium type => giga */
> +	*tmp16 = AX_MEDIUM_RECEIVE_EN | AX_MEDIUM_TXFLOW_CTRLEN |
> +		 AX_MEDIUM_RXFLOW_CTRLEN | AX_MEDIUM_FULL_DUPLEX;
> +	if (!ax179_data->is_ax88772d)
> +		*tmp16 |= AX_MEDIUM_GIGAMODE;
> +	ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_MEDIUM_STATUS_MODE, 2, 2, tmp16);

[Severity: Low]
As reported by Jakub Kicinski on v12 [1], casting u8 buf[5] to u16 *tmp16
and dereferencing it performs 16-bit accesses on a 1-byte-aligned stack
array.  Can this use ax88179_write_u16() with a u16 local variable like the
other 16-bit writes in this function?

> +
> +	/* Check if WoL is supported */
> +	ax179_data->wol_supported = 0;
> +	if (ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_MONITOR_MOD,
> +			     1, 1, tmp) > 0)
> +		ax179_data->wol_supported = WAKE_MAGIC | WAKE_PHY;
> +
> +	/* Ensure EEE state is off, consistent with phylink setup */
> +	ax88179_write_cmd(dev, AX_GPHY_CTL, AX_GPHY_EEE_CTRL, false, 0, NULL);
> +
> +	phylink_start(ax179_data->phylink);
> +
> +	return 0;
> +}

[Severity: Medium]
If usbnet_status_start() fails in usbnet_open() after info->reset() has
succeeded, does phylink remain started while the interface stays down?

usbnet_open() calls info->reset() before usbnet_status_start(), and its
error path does not call info->stop().  Leaving phylink running when open
fails leaves phydev in PHY_UP/PHY_NOLINK/PHY_RUNNING, which triggers a
WARN_ON in phy_start() on the next open attempt.

> +
> +static int ax88179a_stop(struct usbnet *dev)
> +{
> +	struct ax88179_data *ax179_data = dev->driver_priv;
> +	u16 reg16 = 0;
> +
> +	phylink_stop(ax179_data->phylink);
> +
> +	ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_MEDIUM_STATUS_MODE, 2, 2, &reg16);
> +	reg16 &= ~AX_MEDIUM_RECEIVE_EN;
> +	ax88179_write_cmd(dev, AX_ACCESS_MAC, AX_MEDIUM_STATUS_MODE, 2, 2, &reg16);
> +
> +	ax88179_write_u16(dev, AX_ACCESS_MAC, AX_RX_CTL, 2, 0);
> +
> +	ax88179_write_u8(dev, AX_ACCESS_MAC, AX88179A_PHY_POWER, 0, 0, 0);
> +
> +	return 0;
> +}

[Severity: Medium]
Does powering off AX88179A_PHY_POWER in ax88179a_stop() break MDIO accesses
while the interface is down and lose PHY state across down/up transitions?

phylink_stop() already suspends the PHY via BMCR_PDOWN while keeping it
powered for MDIO requests.  Cutting power to the PHY while it remains
attached to phylink causes ethtool and MII ioctl operations performed while
the interface is down to access an unpowered PHY, and when ax88179a_reset()
later restores power, phylink_start() -> phy_start() only calls
__phy_resume() without re-running phy_init_hw().

> +
> +const struct driver_info ax88179a_info = {
> +	.description = "ASIX AX88179A USB 3.2 Gigabit Ethernet",
> +	.bind = ax88179a_bind,
> +	.unbind = ax88179a_unbind,
> +	.status = ax88179a_status,
> +	.reset = ax88179a_reset,
> +	.stop = ax88179a_stop,
> +	.flags = FLAG_ETHER | FLAG_FRAMING_AX | FLAG_MULTI_PACKET | FLAG_AVOID_UNLINK_URBS,
> +	.rx_fixup = ax88179a_rx_fixup,
> +	.tx_fixup = ax88179a_tx_fixup,
> +};
> +
> +const struct driver_info ax88772d_info = {
> +	.description = "ASIX AX88772D/E USB 2.0 Fast Ethernet",
> +	.bind = ax88179a_bind,
> +	.unbind = ax88179a_unbind,
> +	.status = ax88179a_status,
> +	.reset = ax88179a_reset,
> +	.stop = ax88179a_stop,
> +	.flags = FLAG_ETHER | FLAG_FRAMING_AX | FLAG_MULTI_PACKET | FLAG_AVOID_UNLINK_URBS,
> +	.rx_fixup = ax88179a_rx_fixup,
> +	.tx_fixup = ax88179a_tx_fixup,
> +};
> +
> +const struct driver_info ax88279_info = {
> +	.description = "ASIX AX88279 USB 3.2 2.5Gigabit Ethernet",
> +	.bind = ax88179a_bind,
> +	.unbind = ax88179a_unbind,
> +	.status = ax88179a_status,
> +	.reset = ax88179a_reset,
> +	.stop = ax88179a_stop,
> +	.flags = FLAG_ETHER | FLAG_FRAMING_AX | FLAG_MULTI_PACKET | FLAG_AVOID_UNLINK_URBS,
> +	.rx_fixup = ax88179a_rx_fixup,
> +	.tx_fixup = ax88179a_tx_fixup,
> +};

[Severity: High]
Can setting FLAG_AVOID_UNLINK_URBS on these driver_info structs cause a
use-after-free in usbnet_bh_work() if the device is unplugged or unbound
after the interface has been brought down?

When FLAG_AVOID_UNLINK_URBS is set, usbnet_stop() skips
usbnet_terminate_urbs(), leaving submitted RX URBs queued in dev->rxq after
ndo_stop completes.  If the USB device is then disconnected while the
interface is already down, unregister_netdev() skips usbnet_stop() and
usbnet_disconnect() frees the net_device without cancelling dev->bh_work:

CPU0                                  CPU1 (system_bh_wq)
usb_unbind_interface()
  usb_disable_interface()
    ...
      rx_complete()
        defer_bh()
          queue_work(..., &dev->bh_work)
  usbnet_disconnect()
    unregister_netdev(net)
    free_netdev(net)
                                      usbnet_bh_work()
                                        usbnet_bh()  <-- use-after-free

[1] https://lore.kernel.org/r/20260919151247.1a340644@kernel.org

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261114-ax88179a-v21-0-c2d34410bcbe@birger-koblitz.de?part=4