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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 7 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: positive return from
  ax88179_mdiobus_write()
- [Medium] net: usb: ax88179_178a: byte-swapped pause waterlevels in
  ax88179a_mac_link_up()
- [Medium] net: usb: ax88179a: ax88179a_reset() clobbers custom MAC and
  perm_addr on open
- [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 745d111b6d1daea09390fe18d715cc6358070656
Author: Birger Koblitz <mail@birger-koblitz.de>

ax88179_178a: Add HW support for AX179A-based chips

This commit adds bindings and hardware support for the AX179A family of
USB-Ethernet controllers (AX88179A/B, AX88772D/E, and AX88279) 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
> @@ -0,0 +1,983 @@

[ ... ]

> +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);
> +}

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

ax88179_write_u16() and ax88179_write_cmd() return the number of bytes
transferred by usb_control_msg() (2), whereas mii_bus->write and
mii_bus->write_c45 must return 0 on success and a negative errno on
failure.  Callers checking for non-zero return values, such as
phylink_mii_ioctl(SIOCSMIIREG), will receive 2 instead of 0.

[ ... ]

> +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 swap AX_PAUSE_WATERLVL_LOW and
AX_PAUSE_WATERLVL_HIGH?

ax88179_write_u16() converts 0x0410 to little-endian via cpu_to_le16s(),
writing 0x10 to AX_PAUSE_WATERLVL_LOW (0x54) and 0x04 to
AX_PAUSE_WATERLVL_HIGH (0x55), whereas ax88179a_reset() writes 0x04 to
AX_PAUSE_WATERLVL_LOW and 0x10 to AX_PAUSE_WATERLVL_HIGH.  Setting the low
pause threshold above the high threshold breaks hardware flow control on
link-up.

[ ... ]

> +	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()?

During usbnet_open(), ax88179a_reset() calls phylink_start(), which turns
carrier off while autonegotiation starts asynchronously.  When
usbnet_open() schedules dev->bh_work, usbnet_bh() sees
!netif_carrier_ok(dev->net) and does not submit any RX URBs.  When phylink
later brings the link up, nothing calls usbnet_link_change(dev, true,
false) to schedule dev->bh_work or update queue lengths via
usbnet_update_max_qlen(), leaving dev->rxq empty so no packets can be
received.  Likewise, ax88179a_mac_link_down() never calls
usbnet_link_change(dev, false, false) to unlink pending RX URBs when the
link drops.

[ ... ]

> +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 copying dev_addr into perm_addr in
ax88179a_reset() clobber a user-configured MAC address or permanent MAC
address on open?

ax88179_set_mac_addr() only allows changing the MAC address while the
interface is down, writing the new address to dev->net->dev_addr and
AX_NODE_ID and setting addr_assign_type to NET_ADDR_SET.  When the
interface is then brought up, usbnet_open() calls ax88179a_reset(): if a
Device Tree MAC address is present, ax88179_get_mac_addr() overwrites the
user-configured MAC address with the DT address; otherwise it reads the
user-configured address back from AX_NODE_ID, resets addr_assign_type to
NET_ADDR_PERM, and overwrites dev->net->perm_addr with it.

[ ... ]

> +	/* 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 the 1-byte-aligned stack
array u8 buf[5] to u16 *tmp16 performs unaligned 16-bit accesses here.
Using ax88179_write_u16() as done earlier in ax88179a_reset() would avoid
both tmp16 and buf.

[ ... ]

> +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, AX88179A_PHY_POWER, 0, 0, 0);
> +
> +	return 0;
> +}

[Severity: Medium]
Should ax88179a_stop() cut AX88179A_PHY_POWER while the PHY is still
attached to phylink?

phylink_stop() already suspends the PHY via BMCR_PDOWN while keeping it
powered for MDIO access.  Writing 0 to AX88179A_PHY_POWER powers down the
PHY hardware, so ethtool or MII ioctl operations issued while the
interface is down access an unpowered PHY, and bringing the interface back
up in ax88179a_reset() -> phylink_start() only calls __phy_resume()
without re-running phy_init_hw() after the power cycle.

> +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 here cause a use-after-free on
struct usbnet if the device is unplugged while the interface is down?

When FLAG_AVOID_UNLINK_URBS is set, usbnet_stop() skips
usbnet_terminate_urbs(dev), and ax88179a_stop() disables MAC RX without
flushing pending bulk-in transfers, leaving submitted RX URBs queued in
dev->rxq after the interface is down.  If the device is then unplugged or
unbound while !netif_running(net):

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

Because the interface is already down, unregister_netdev() does not call
usbnet_stop(), and usbnet_disconnect() frees net without cancelling
dev->bh_work or deleting dev->delay.

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

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261106-ax88179a-v20-0-37b5a1189983@birger-koblitz.de?part=4