Re: [PATCH net-next v19 08/13] ax88179_178a: Add AX179A/AX279 multicast configuration

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] net: ax88179: TOCTOU race in AX_RX_CTL hardware update
--

commit 81a7ae4b83e41763b94eb2bd27f7e79602b4f8d5
Author: Birger Koblitz <mail@birger-koblitz.de>

ax88179_178a: Add AX179A/AX279 multicast configuration

Add support for conditionally setting the ip_alignement flag
AX_RX_CTL_IPE in AX_RX_CTL and make sure that AX_RX_CTL_DROPCRCERR
is also set to be consistent with the initial configuration in
ax88179_reset()

> diff --git a/drivers/net/usb/ax88179_lib.c b/drivers/net/usb/ax88179_lib.c
> index a314db4c97666..b4b00c8b4b623 100644
> --- a/drivers/net/usb/ax88179_lib.c
> +++ b/drivers/net/usb/ax88179_lib.c
[ ... ]
> @@ -351,9 +356,10 @@ void ax88179_set_multicast(struct net_device *net)
>  					AX_MCAST_FLTSIZE, AX_MCAST_FLTSIZE,
>  					m_filter);
>  
> -		data->rxctl |= AX_RX_CTL_AM;
> +		rxctl |= AX_RX_CTL_AM;
>  	}
>  
> +	WRITE_ONCE(data->rxctl, rxctl);
>  	ax88179_write_cmd_async(dev, AX_ACCESS_MAC, AX_RX_CTL,
>  				2, 2, &data->rxctl);
>  }

[Severity: High]
Is it possible for this asynchronous USB write to race with the synchronous
hardware update in ax88179a_mac_link_up()?

Since ndo_set_rx_mode (which calls ax88179_set_multicast) holds
netif_addr_lock_bh() while the phylink state machine runs in a workqueue
without this lock, these two functions can execute concurrently.

This asynchronous write is queued without holding medium_status_mutex.

Meanwhile, ax88179a_mac_link_up() reads data->rxctl, acquires
medium_status_mutex, and executes a synchronous USB write:

    ax88179_write_u16(dev, AX_ACCESS_MAC, AX_RX_CTL, 2,
                      READ_ONCE(ax179_data->rxctl));
    mutex_unlock(&ax179_data->medium_status_mutex);

If ax88179a_mac_link_up() reads data->rxctl before it is updated here, but
its synchronous USB URB is submitted after this asynchronous URB, will the
USB controller write the new value then immediately overwrite it with the
old value?

Could this leave the hardware state out of sync with the software shadow
state, causing the device to silently drop multicast or promiscuous
configuration?

> diff --git a/drivers/net/usb/ax88179a_devices.c b/drivers/net/usb/ax88179a_devices.c
> index 4207d2cc2b865..a6b0a3f47118c 100644
> --- a/drivers/net/usb/ax88179a_devices.c
> +++ b/drivers/net/usb/ax88179a_devices.c
> @@ -678,6 +678,7 @@ static const struct net_device_ops ax88179a_netdev_ops = {
>  	.ndo_set_mac_address	= ax88179_set_mac_addr,
>  	.ndo_validate_addr	= eth_validate_addr,
>  	.ndo_eth_ioctl		= ax88179a_mii_ioctl,
> +	.ndo_set_rx_mode	= ax88179_set_multicast,
>  	.ndo_set_features	= ax88179a_set_features,
>  	.ndo_vlan_rx_add_vid	= ax88179a_vlan_rx_add_vid,
>  	.ndo_vlan_rx_kill_vid	= ax88179a_vlan_rx_kill_vid,

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261010-ax88179a-v19-0-7475173cfb56@birger-koblitz.de?part=8