Re: [PATCH net-next v18 07/13] ax88179_178a: Add VLAN offload support for AX88179A

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] net: usb: ax88179a: missing WE command in VLAN filter table init loop
- [Medium] net: usb: ax88179a: ignored USB write errors in VLAN filter init
- [Medium] net: usb: ax88179a: uninitialized stack memory use on short USB read
--

commit 86960e6590c9a326bcedbb3ba8fde9f53c488e20
Author: Birger Koblitz <mail@birger-koblitz.de>

    ax88179_178a: Add VLAN offload support for AX88179A
    
    The AX88179A-based chips support VLAN offload. Add configuration
    support in netdev_ops. Features supported are:
    NETIF_F_HW_VLAN_CTAG_TX, NETIF_F_HW_VLAN_CTAG_RX
    and NETIF_F_HW_VLAN_CTAG_FILTER.

> diff --git a/drivers/net/usb/ax88179a_devices.c b/drivers/net/usb/ax88179a_devices.c
> index c41d9c2dc737a..bdc67cbc17287 100644
> --- a/drivers/net/usb/ax88179a_devices.c
> +++ b/drivers/net/usb/ax88179a_devices.c
> @@ -258,6 +258,88 @@ static const struct ethtool_ops ax88179a_ethtool_ops = {
>  	.get_ts_info		= ethtool_op_get_ts_info,
>  };
>  
> +static int ax88179a_vlan_rx_kill_vid(struct net_device *net, __be16 proto, u16 vid)
> +{
> +	struct usbnet *dev = netdev_priv(net);
> +	u8 vlan_ctrl;
> +	int ret = 0;
> +	u16 reg16;
> +	u8 reg8;
> +
> +	ret = ax88179_read_cmd(dev, AX_ACCESS_MAC, AX88179A_VLAN_ID_CONTROL, 1, 1, &reg8);
> +	if (ret < 0)
> +		return ret;

[Severity: Medium]
If a malicious or buggy USB device responds to the read request with a
zero-length packet, ax88179_read_cmd() will return 0 and skip copying data,
leaving reg8 uninitialized.

Since the error check only verifies if ret < 0, does this allow uninitialized
stack memory to be used for hardware control and eventually written back to
the MAC? This also applies to the reads in ax88179a_vlan_rx_add_vid() and
ax88179a_set_features().

[ ... ]
> +static int ax88179a_set_features(struct net_device *net, netdev_features_t features)
> +{
> +	struct usbnet *dev = netdev_priv(net);
> +	netdev_features_t changed;
> +	int ret;
> +	u8 tmp;
> +
> +	changed = net->features ^ features;
> +
> +	ret = ax88179_set_features(net, features);
> +	if (ret)
> +		return ret;
> +
> +	if (changed & NETIF_F_HW_VLAN_CTAG_FILTER) {
> +		if (features & NETIF_F_HW_VLAN_CTAG_FILTER) {
> +			for (int i = 0; i < 256; i++) {
> +				/* Address */
> +				tmp = i;
> +				ax88179_write_cmd(dev, AX_ACCESS_MAC, AX88179A_VLAN_ID_ADDRESS,
> +						  1, 1, &tmp);
> +				/* Data */
> +				ax88179_write_u16(dev, AX_ACCESS_MAC, AX88179A_VLAN_ID_DATA0,
> +						  2, 0);
> +			}

[Severity: Medium]
These ax88179_write_cmd() and ax88179_write_u16() calls lack error checking.
Could this prevent an early abort on device disconnect or hardware failure,
resulting in the thread blocking through 512 failing USB control transfers?

[Severity: High]
Does this loop fail to actually initialize the hardware filter table for the
first 255 entries?

The write enable command (AX_VLAN_CONTROL_WE) is only sent once after the
loop finishes, which suggests that only the last address (255) is written
to the SRAM while the first 255 entries are ignored. Should the WE trigger be
included inside the loop?

> +			ret = ax88179_read_cmd(dev, AX_ACCESS_MAC,
> +					       AX88179A_VLAN_ID_CONTROL, 1, 1, &tmp);
> +			if (ret < 0)
> +				return ret;
> +			tmp |= AX_VLAN_CONTROL_VFE | AX_VLAN_CONTROL_WE;
> +			ret = ax88179_write_cmd(dev, AX_ACCESS_MAC,
> +						AX88179A_VLAN_ID_CONTROL, 1, 1, &tmp);
> +			if (ret < 0)
> +				return ret;
> +		} else {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261008-ax88179a-v18-0-6fa1d4c40422@birger-koblitz.de?part=7