Messages in this thread | | | Date | Mon, 4 Apr 2022 19:50:02 +0300 | Subject | Re: [PATCH] net: ax88179: add proper error handling of usb read errors | From | Pavel Skripkin <> |
| |
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 [unhandled content-type:application/pgp-signature] | |