Re: [PATCH] net: ax88179: add proper error handling of usb read errors

[Pavel Skripkin <paskripkin@gmail.com>]

[-- Attachment #1.1: Type: text/plain, Size: 5116 bytes --]

Hi David,

On 4/4/22 18:10, David Kahurani wrote:
> Reads that are lesser than the requested size lead to uninit-value bugs. Qualify
> such reads as errors and handle them correctly.
> 
> ax88179_178a 2-1:0.35 (unnamed net_device) (uninitialized): Failed to read reg index 0x0001: -71
> ax88179_178a 2-1:0.35 (unnamed net_device) (uninitialized): Failed to read reg index 0x0002: -71
> =====================================================
> BUG: KMSAN: uninit-value in ax88179_check_eeprom drivers/net/usb/ax88179_178a.c:1074 [inline]
> BUG: KMSAN: uninit-value in ax88179_led_setting+0x884/0x30b0 drivers/net/usb/ax88179_178a.c:1168
>   ax88179_check_eeprom drivers/net/usb/ax88179_178a.c:1074 [inline]
>   ax88179_led_setting+0x884/0x30b0 drivers/net/usb/ax88179_178a.c:1168
>   ax88179_bind+0xe75/0x1990 drivers/net/usb/ax88179_178a.c:1411
>   usbnet_probe+0x1284/0x4140 drivers/net/usb/usbnet.c:1747
>   usb_probe_interface+0xf19/0x1600 drivers/usb/core/driver.c:396
>   really_probe+0x67d/0x1510 drivers/base/dd.c:596
>   __driver_probe_device+0x3e9/0x530 drivers/base/dd.c:751
>   driver_probe_device drivers/base/dd.c:781 [inline]
>   __device_attach_driver+0x79f/0x1120 drivers/base/dd.c:898
>   bus_for_each_drv+0x2d6/0x3f0 drivers/base/bus.c:427
>   __device_attach+0x593/0x8e0 drivers/base/dd.c:969
>   device_initial_probe+0x4a/0x60 drivers/base/dd.c:1016
>   bus_probe_device+0x17b/0x3e0 drivers/base/bus.c:487
>   device_add+0x1d3e/0x2400 drivers/base/core.c:3394
>   usb_set_configuration+0x37e9/0x3ed0 drivers/usb/core/message.c:2170
>   usb_generic_driver_probe+0x13c/0x300 drivers/usb/core/generic.c:238
>   usb_probe_device+0x309/0x570 drivers/usb/core/driver.c:293
>   really_probe+0x67d/0x1510 drivers/base/dd.c:596
>   __driver_probe_device+0x3e9/0x530 drivers/base/dd.c:751
>   driver_probe_device drivers/base/dd.c:781 [inline]
>   __device_attach_driver+0x79f/0x1120 drivers/base/dd.c:898
>   bus_for_each_drv+0x2d6/0x3f0 drivers/base/bus.c:427
>   __device_attach+0x593/0x8e0 drivers/base/dd.c:969
>   device_initial_probe+0x4a/0x60 drivers/base/dd.c:1016
> 

I'd personally cut this log a bit and would add this part of the initial 
report

Local variable eeprom.i created at:
  ax88179_check_eeprom drivers/net/usb/ax88179_178a.c:1045 [inline]
  ax88179_led_setting+0x2e2/0x30b0 drivers/net/usb/ax88179_178a.c:1168
  ax88179_bind+0xe75/0x1990 drivers/net/usb/ax88179_178a.c:1411

Since it shows exactly where problem comes from.

I do not insist, just IMO

> Signed-off-by: David Kahurani <k.kahurani@gmail.com>
> Reported-by: syzbot+d3dbdf31fbe9d8f5f311@syzkaller.appspotmail.com
> ---

It's indeed a bug fix, so fixes tag is needed

Fixes: e2ca90c276e1 ("ax88179_178a: ASIX AX88179_178A USB 3.0/2.0 to 
gigabit ethernet adapter driver")


>   drivers/net/usb/ax88179_178a.c | 255 +++++++++++++++++++++++++++------
>   1 file changed, 213 insertions(+), 42 deletions(-)
> 
> diff --git a/drivers/net/usb/ax88179_178a.c b/drivers/net/usb/ax88179_178a.c
> index e2fa56b92..b5e114bed 100644
> --- a/drivers/net/usb/ax88179_178a.c
> +++ b/drivers/net/usb/ax88179_178a.c
> @@ -185,8 +185,9 @@ static const struct {
>   	{7, 0xcc, 0x4c, 0x18, 8},
>   };
>   
> -static int __ax88179_read_cmd(struct usbnet *dev, u8 cmd, u16 value, u16 index,
> -			      u16 size, void *data, int in_pm)
> +static int __must_check __ax88179_read_cmd(struct usbnet *dev, u8 cmd,
> +		                           u16 value, u16 index, u16 size,
> +					   void *data, int in_pm)
>   {
>   	int ret;
>   	int (*fn)(struct usbnet *, u8, u8, u16, u16, void *, u16);
> @@ -201,9 +202,12 @@ static int __ax88179_read_cmd(struct usbnet *dev, u8 cmd, u16 value, u16 index,
>   	ret = fn(dev, cmd, USB_DIR_IN | USB_TYPE_VENDOR | USB_RECIP_DEVICE,
>   		 value, index, data, size);
>   
> -	if (unlikely(ret < 0))
> +	if (unlikely(ret < size)) {
> +		ret = ret < 0 ? ret : -ENODATA;
> +
>   		netdev_warn(dev->net, "Failed to read reg index 0x%04x: %d\n",
>   			    index, ret);
> +	}
>   
>   	return ret;
>   }
> @@ -249,19 +253,26 @@ static void ax88179_write_cmd_async(struct usbnet *dev, u8 cmd, u16 value,
>   	}
>   }
>   
> -static int ax88179_read_cmd_nopm(struct usbnet *dev, u8 cmd, u16 value,
> -				 u16 index, u16 size, void *data)
> +static int __must_check ax88179_read_cmd_nopm(struct usbnet *dev, u8 cmd,
> +		                              u16 value, u16 index, u16 size,
> +					      void *data)
>   {
>   	int ret;
>   
>   	if (2 == size) {
>   		u16 buf;
>   		ret = __ax88179_read_cmd(dev, cmd, value, index, size, &buf, 1);
> +
> +		if (ret < 0)
> +			return ret;

Empty line after assignment and before check looks redundant.

>   		le16_to_cpus(&buf);
>   		*((u16 *)data) = buf;
>   	} else if (4 == size) {
>   		u32 buf;
>   		ret = __ax88179_read_cmd(dev, cmd, value, index, size, &buf, 1);
> +
> +		if (ret < 0)
> +			return ret;

Same here


In general, looks good, but, please, fix up indenting and other small 
issues.




With regards,
Pavel Skripkin

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]