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 <[email protected]>
> Reported-by: [email protected]
> ---
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
On Tue, Apr 5, 2022 at 3:05 AM Pavel Skripkin <[email protected]> wrote:
> 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.
> 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
I insist though. It will reduce the resource consumption (i.e. storage
and network load on cloning / pulling) a lot (taking into account
multiplier of how many Linux kernel source copies are around the
globe) and hence become more environmentally friendly.
--
With Best Regards,
Andy Shevchenko