Re: [PATCH net-next v11 14/15] ax88179_178a: Add support for AX88179A/772D/279 EEPROM access

[netdev-bot+sashiko@kernel.org]

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

Critical: 0 · High: 0 · Medium: 3 · Low: 1

- [Medium] Undisclosed behaviour change to the already-supported
  AX88179/AX88178A devices: ax88179_bind()…
- [Medium] The EEPROM access method is selected by three different,
  mutually inconsistent predicates over the same unvalidated device-…
- [Medium] For the AX88179A/AX88772D efuse path the new
  ax88179_eeprom_access_params() (drivers/net/usb/ax88179_lib.c:210-212)…
- [Low] Dead store on the new error path of ax88179_bind(): after
  `kfree(ax179_data);` the code clears the local variable (`ax179_data =…

--

ax88179_178a: Add support for AX88179A/772D/279 EEPROM access

This adds ethtool EEPROM read support for the AX88179A/772D efuses and the
AX88279 flash by generalising ax88179_get_eeprom() over a per-chip block
size and address encoding.  ax88179_bind() now samples the chip version and
initialises the new eeprom_* fields, and ax88179_set_eeprom() refuses
writes on the newer parts.

A few questions below.

> diff --git a/drivers/net/usb/ax88179_178a.c b/drivers/net/usb/ax88179_178a.c
> index 1d35ef90eceb7..55564aa0ca63f 100644
> --- a/drivers/net/usb/ax88179_178a.c
> +++ b/drivers/net/usb/ax88179_178a.c
> @@ -687,6 +687,19 @@ static int ax88179_bind(struct usbnet *dev, struct usb_interface *intf)
>  	ax179_data->resume = ax88179_resume;
>  	ax179_data->suspend = ax88179_suspend;
>  
> +	ret = ax88179_read_cmd(dev, AX_ACCESS_MAC, AX_CHIP_STATUS,
> +			       1, 1, &ax179_data->chip_version);
> +	if (ret < 0)
> +		goto err_nodev;

[Severity: Medium]
Is the behaviour change for the already-supported parts intentional?  Before
this patch ax88179_bind() had no register read that could abort the probe;
now a single failing control transfer for AX_CHIP_STATUS makes bind return
an error for every driver_info entry using ax88179_bind() (AX88179,
AX88178A, Cypress GX3, D-Link, Sitecom, Samsung, Lenovo, Belkin, Toshiba,
MCT, AT-UMC*), and usbnet_probe() then goes to out1 -> free_netdev(), so no
netdev is created at all.

The commit message only says:

    The AX88179A/772D devices have 32 efuses with 20 bytes each,
    which can be randomly programmed. The AX88279 has 16K FLASH.

    Provide ethtool read capability for these devices.

Could the message mention that the legacy probe path now depends on this
read succeeding, and that ethtool -E on the legacy parts becomes conditional
on the device-reported version nibble (see the new gate in
ax88179_set_eeprom() below)?

> +
> +	ax179_data->chip_version = (ax179_data->chip_version & 0xf0) >> 4;
> +	ax179_data->is_ax88772d = 0;
> +	ax179_data->ip_align = 1;
> +	ax179_data->eeprom_read_cmd = AX_ACCESS_EEPROM;
> +	ax179_data->eeprom_write_cmd = AX_ACCESS_EEPROM;
> +	ax179_data->eeprom_block = 2;
> +	ax179_data->eeprom_wen = 0;
> +
>  	dev->net->netdev_ops = &ax88179_netdev_ops;
>  	dev->net->ethtool_ops = &ax88179_ethtool_ops;
>  	dev->net->needed_headroom = 8;
> @@ -711,6 +724,12 @@ static int ax88179_bind(struct usbnet *dev, struct usb_interface *intf)
>  	ax88179_reset(dev);
>  
>  	return 0;
> +
> +err_nodev:
> +	kfree(ax179_data);
> +	ax179_data = NULL;
       ^^^^^^^^^^^^^^^^^^

[Severity: Low]
Does this assignment do anything?  ax179_data is a local that goes out of
scope immediately, while dev->driver_priv was published earlier in
ax88179_bind():

	dev->driver_priv = ax179_data;

so dev->driver_priv keeps pointing at the freed allocation.  Was
dev->driver_priv = NULL; intended here, or can the store simply be dropped?

There is no use-after-free today: usbnet_probe() jumps to out1 on bind
failure, which skips info->unbind and frees the netdev (and therefore the
private area) right away.

> +
> +	return ret;
>  }
>  
>  static void ax88179_unbind(struct usbnet *dev, struct usb_interface *intf)
> diff --git a/drivers/net/usb/ax88179_lib.c b/drivers/net/usb/ax88179_lib.c
> index 3b498206c50d5..db77b7d4539ec 100644
> --- a/drivers/net/usb/ax88179_lib.c
> +++ b/drivers/net/usb/ax88179_lib.c
> @@ -194,36 +194,61 @@ int ax88179_set_wol(struct net_device *net, struct ethtool_wolinfo *wolinfo)
>  	return 0;
>  }
>  
> +static void
> +ax88179_eeprom_access_params(struct ax88179_data *ax179_data, int i, u16 *value, u16 *idx)
> +{
> +	/* AX88179 has a word-addressable EEPROM
> +	 * AX88179A uses EFUSES with 20 bytes length
> +	 * AX88279 has an EEPROM addressable in 256 byte blocks
> +	 */
> +	if (ax179_data->chip_version < AX_VERSION_AX88179A) {
> +		*value = i;
> +		*idx = 1;
> +	} else if (ax179_data->chip_version >= AX_VERSION_AX88279) {
> +		*value = (i * ax179_data->eeprom_block) >> 16;
> +		*idx = (i * ax179_data->eeprom_block) & 0xffff;
> +	} else {
> +		*value = i << 4;
> +		*idx = 0;
> +	}
> +}

[Severity: Medium]
Can the three predicates over chip_version disagree?  The address encoding
is chosen here with range tests, but the command and block size are chosen
elsewhere with different tests.

ax88179a_bind() in ax88179a_devices.c uses an exact match:

	if (ax179_data->chip_version == AX_VERSION_AX88279) {
		ax179_data->eeprom_read_cmd = AX88179A_FLASH_READ;
		ax179_data->eeprom_block = 256;
	} else {
		ax179_data->eeprom_read_cmd = AX_ACCESS_EFUS;
		ax179_data->eeprom_block = 20;
	}

ax88179a_get_eeprom_len() uses a range test:

	if (ax179_data->chip_version >= AX_VERSION_AX88279)
		return AX88279_EEPROM_LEN;
	else
		return AX88179A_EEPROM_LEN;

and the new code in ax88179_bind() hardcodes AX_ACCESS_EEPROM with
eeprom_block = 2 while still storing whatever nibble the chip reported.

The nibble from AX_CHIP_STATUS is not validated anywhere, so for a value
above AX_VERSION_AX88279 the helper picks the split flash addressing while
bind configured AX_ACCESS_EFUS with 20-byte transfers, and
get_eeprom_len() advertises 16 KiB.  Symmetrically, a legacy device
reporting a nibble >= AX_VERSION_AX88179A gets AX_ACCESS_EEPROM issued with
efuse or flash addressing at a 2-byte length, and ethtool -E turns into
-EOPNOTSUPP.  Would it be better to derive the addressing from
eeprom_read_cmd/eeprom_block, or to reject unknown version nibbles at bind
time?

[Severity: Medium]
For the efuse branch, are wValue and wIndex right?  The comment in this same
function says the efuses are 20 bytes long, ax88179a_bind() sets
eeprom_block = 20, AX88179A_EEPROM_LEN is (32 * 20) = 640, and
ax88179_get_eeprom() below derives first/last, the per-row buffer offset and
the transfer length from eeprom_block.  But the address here advances by 16
per row:

	*value = i << 4;
	*idx = 0;

The existing AX_ACCESS_EFUS user in this driver family passes a byte offset
in wValue and the access length in wIndex:

drivers/net/usb/ax88179_178a.c:ax88179_check_efuse() {
	...
	if (ax88179_read_cmd(dev, AX_ACCESS_EFUS, 0, 64, 64, efuse) < 0)
		return -EINVAL;
	...
}

Under that convention consecutive 20-byte reads spaced 16 bytes apart
overlap by 4 bytes per row and can only reach bytes 0..515 of the 640-byte
range ethtool is told about, and wIndex = 0 asks for a zero-length access
while 20 bytes are requested on the wire.

Does the failure stay silent?  __usbnet_read_cmd() copies only the bytes the
device actually returned and reports a positive value, and
ax88179_get_eeprom() only tests ret < 0, so a short or rejected efuse access
is handed to userspace as a successful read of the kzalloc'ed zeros.

Has the AX88179A redefined AX_ACCESS_EFUS so that wValue is a shifted row
index and wIndex is unused?  If so a comment would help, otherwise the
stride and the length field look inconsistent with eeprom_block = 20.

> +
>  int ax88179_get_eeprom(struct net_device *net, struct ethtool_eeprom *eeprom, u8 *data)
>  {
>  	struct usbnet *dev = netdev_priv(net);
> -	u16 *eeprom_buff;
> -	int first_word, last_word;
> -	int i, ret;
> +	struct ax88179_data *ax179_data;
> +	int first, last, i, ret;
> +	u8 *eeprom_buff;
> +
> +	ax179_data = dev->driver_priv;
>  
>  	if (eeprom->len == 0)
>  		return -EINVAL;
>  
>  	eeprom->magic = AX88179_EEPROM_MAGIC;
>  
> -	first_word = eeprom->offset >> 1;
> -	last_word = (eeprom->offset + eeprom->len - 1) >> 1;
> -	eeprom_buff = kmalloc_array(last_word - first_word + 1, sizeof(u16),
> -				    GFP_KERNEL);
> +	first = eeprom->offset / ax179_data->eeprom_block;
> +	last = (eeprom->offset + eeprom->len - 1) / ax179_data->eeprom_block;
> +
> +	eeprom_buff = kzalloc((last - first + 1) * ax179_data->eeprom_block, GFP_KERNEL);
>  	if (!eeprom_buff)
>  		return -ENOMEM;
>  
> -	/* ax88179/178A returns 2 bytes from eeprom on read */
> -	for (i = first_word; i <= last_word; i++) {
> -		ret = __ax88179_read_cmd(dev, AX_ACCESS_EEPROM, i, 1, 2,
> -					 &eeprom_buff[i - first_word]);
> +	for (i = first; i <= last; i++) {
> +		u16 value, idx;
> +
> +		ax88179_eeprom_access_params(ax179_data, i, &value, &idx);
> +		ret = __ax88179_read_cmd(dev, ax179_data->eeprom_read_cmd,
> +					 value, idx, ax179_data->eeprom_block,
> +					 eeprom_buff + (i - first) * ax179_data->eeprom_block);
> +
>  		if (ret < 0) {
>  			kfree(eeprom_buff);
>  			return -EIO;
>  		}
>  	}
>  
> -	memcpy(data, (u8 *)eeprom_buff + (eeprom->offset & 1), eeprom->len);
> +	memcpy(data, eeprom_buff + eeprom->offset % ax179_data->eeprom_block, eeprom->len);
>  	kfree(eeprom_buff);
>  	return 0;
>  }
> @@ -231,12 +256,17 @@ int ax88179_get_eeprom(struct net_device *net, struct ethtool_eeprom *eeprom, u8
>  int ax88179_set_eeprom(struct net_device *net, struct ethtool_eeprom *eeprom, u8 *data)
>  {
>  	struct usbnet *dev = netdev_priv(net);
> +	struct ax88179_data *ax179_data;
>  	u16 *eeprom_buff;
>  	int first_word;
>  	int last_word;
>  	int ret;
>  	int i;
>  
> +	ax179_data = dev->driver_priv;
> +	if (ax179_data->chip_version >= AX_VERSION_AX88179A)
> +		return -EOPNOTSUPP;
> +

[ ... ]

-- 
Sashiko AI review · https://netdev-ai.bots.linux.dev/sashiko/#/patchset/20260914-ax88179a-v11-0-5ea7a925ba6d%40birger-koblitz.de