| Message ID | 20250520113240.2369438-1-n.zhandarovich@fintech.ru |
|---|---|
| State | New |
| Headers | show |
| Series | [net-next] net: usb: aqc111: fix error handling of usbnet read calls | expand |
On Tue, May 20, 2025 at 02:32:39PM +0300, Nikita Zhandarovich wrote: > Syzkaller, courtesy of syzbot, identified an error (see report [1]) in > aqc111 driver, caused by incomplete sanitation of usb read calls' > results. This problem is quite similar to the one fixed in commit > 920a9fa27e78 ("net: asix: add proper error handling of usb read errors"). > > For instance, usbnet_read_cmd() may read fewer than 'size' bytes, > even if the caller expected the full amount, and aqc111_read_cmd() > will not check its result properly. As [1] shows, this may lead > to MAC address in aqc111_bind() being only partly initialized, > triggering KMSAN warnings. It looks like __ax88179_read_cmd() has the same issue? Please could you have a look around and see if more of the same problem exists. Are there any use cases where usbnet_read_cmd() can actually return less than size and it not being an error? Maybe this check for ret != size can be moved inside usbnet_read_cmd()? Andrew
On 5/21/25 2:34 PM, Andrew Lunn wrote: > On Tue, May 20, 2025 at 02:32:39PM +0300, Nikita Zhandarovich wrote: >> Syzkaller, courtesy of syzbot, identified an error (see report [1]) in >> aqc111 driver, caused by incomplete sanitation of usb read calls' >> results. This problem is quite similar to the one fixed in commit >> 920a9fa27e78 ("net: asix: add proper error handling of usb read errors"). >> >> For instance, usbnet_read_cmd() may read fewer than 'size' bytes, >> even if the caller expected the full amount, and aqc111_read_cmd() >> will not check its result properly. As [1] shows, this may lead >> to MAC address in aqc111_bind() being only partly initialized, >> triggering KMSAN warnings. > > It looks like __ax88179_read_cmd() has the same issue? Please could > you have a look around and see if more of the same problem exists. > > Are there any use cases where usbnet_read_cmd() can actually return > less than size and it not being an error? Maybe this check for ret != > size can be moved inside usbnet_read_cmd()? Judging from __usbnet_read_cmd() implementation, it's actually expected that such helper could return a partial copy. The centralized check could possibly break some users, I think check the return value on a per-device basis is safer. Side note: the patch should have targeted the 'net' tree as the blamed commit is present there, given we are now in the merge window and net-next PR is somewhat upcoming, applying it to net-next will yield the same result. /P
Hello: This patch was applied to netdev/net-next.git (main) by Paolo Abeni <pabeni@redhat.com>: On Tue, 20 May 2025 14:32:39 +0300 you wrote: > Syzkaller, courtesy of syzbot, identified an error (see report [1]) in > aqc111 driver, caused by incomplete sanitation of usb read calls' > results. This problem is quite similar to the one fixed in commit > 920a9fa27e78 ("net: asix: add proper error handling of usb read errors"). > > For instance, usbnet_read_cmd() may read fewer than 'size' bytes, > even if the caller expected the full amount, and aqc111_read_cmd() > will not check its result properly. As [1] shows, this may lead > to MAC address in aqc111_bind() being only partly initialized, > triggering KMSAN warnings. > > [...] Here is the summary with links: - [net-next] net: usb: aqc111: fix error handling of usbnet read calls https://git.kernel.org/netdev/net-next/c/405b0d610745 You are awesome, thank you!
diff --git a/drivers/net/usb/aqc111.c b/drivers/net/usb/aqc111.c index ff5be2cbf17b..453a2cf82753 100644 --- a/drivers/net/usb/aqc111.c +++ b/drivers/net/usb/aqc111.c @@ -30,10 +30,13 @@ static int aqc111_read_cmd_nopm(struct usbnet *dev, u8 cmd, u16 value, ret = usbnet_read_cmd_nopm(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(0x%x) reg index 0x%04x: %d\n", cmd, index, ret); + } return ret; } @@ -46,10 +49,13 @@ static int aqc111_read_cmd(struct usbnet *dev, u8 cmd, u16 value, ret = usbnet_read_cmd(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(0x%x) reg index 0x%04x: %d\n", cmd, index, ret); + } return ret; }
Syzkaller, courtesy of syzbot, identified an error (see report [1]) in aqc111 driver, caused by incomplete sanitation of usb read calls' results. This problem is quite similar to the one fixed in commit 920a9fa27e78 ("net: asix: add proper error handling of usb read errors"). For instance, usbnet_read_cmd() may read fewer than 'size' bytes, even if the caller expected the full amount, and aqc111_read_cmd() will not check its result properly. As [1] shows, this may lead to MAC address in aqc111_bind() being only partly initialized, triggering KMSAN warnings. Fix the issue by verifying that the number of bytes read is as expected and not less. [1] Partial syzbot report: BUG: KMSAN: uninit-value in is_valid_ether_addr include/linux/etherdevice.h:208 [inline] BUG: KMSAN: uninit-value in usbnet_probe+0x2e57/0x4390 drivers/net/usb/usbnet.c:1830 is_valid_ether_addr include/linux/etherdevice.h:208 [inline] usbnet_probe+0x2e57/0x4390 drivers/net/usb/usbnet.c:1830 usb_probe_interface+0xd01/0x1310 drivers/usb/core/driver.c:396 call_driver_probe drivers/base/dd.c:-1 [inline] really_probe+0x4d1/0xd90 drivers/base/dd.c:658 __driver_probe_device+0x268/0x380 drivers/base/dd.c:800 ... Uninit was stored to memory at: dev_addr_mod+0xb0/0x550 net/core/dev_addr_lists.c:582 __dev_addr_set include/linux/netdevice.h:4874 [inline] eth_hw_addr_set include/linux/etherdevice.h:325 [inline] aqc111_bind+0x35f/0x1150 drivers/net/usb/aqc111.c:717 usbnet_probe+0xbe6/0x4390 drivers/net/usb/usbnet.c:1772 usb_probe_interface+0xd01/0x1310 drivers/usb/core/driver.c:396 ... Uninit was stored to memory at: ether_addr_copy include/linux/etherdevice.h:305 [inline] aqc111_read_perm_mac drivers/net/usb/aqc111.c:663 [inline] aqc111_bind+0x794/0x1150 drivers/net/usb/aqc111.c:713 usbnet_probe+0xbe6/0x4390 drivers/net/usb/usbnet.c:1772 usb_probe_interface+0xd01/0x1310 drivers/usb/core/driver.c:396 call_driver_probe drivers/base/dd.c:-1 [inline] ... Local variable buf.i created at: aqc111_read_perm_mac drivers/net/usb/aqc111.c:656 [inline] aqc111_bind+0x221/0x1150 drivers/net/usb/aqc111.c:713 usbnet_probe+0xbe6/0x4390 drivers/net/usb/usbnet.c:1772 Reported-by: syzbot+3b6b9ff7b80430020c7b@syzkaller.appspotmail.com Closes: https://syzkaller.appspot.com/bug?extid=3b6b9ff7b80430020c7b Tested-by: syzbot+3b6b9ff7b80430020c7b@syzkaller.appspotmail.com Fixes: df2d59a2ab6c ("net: usb: aqc111: Add support for getting and setting of MAC address") Signed-off-by: Nikita Zhandarovich <n.zhandarovich@fintech.ru> --- P.S. In aqc111 there are many calls to aqc111_read_cmd[_nopm] functions in other parts of the driver and most of them are not checked at all. I've chosen to forego error handling of them, as it seems it's missing deliberately. Correct me if I am wrong. drivers/net/usb/aqc111.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-)