[PATCH net-next] net: usb: aqc111: fix error handling of usbnet read calls

[PATCH net-next] net: usb: aqc111: fix error handling of usbnet read calls

[Nikita Zhandarovich]

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(-)

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;
 }

Re: [PATCH net-next] net: usb: aqc111: fix error handling of usbnet read calls

[Andrew Lunn]

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

Re: [PATCH net-next] net: usb: aqc111: fix error handling of usbnet read calls

[Nikita Zhandarovich]

Hello,

On 5/21/25 15:34, 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.
> 

Yes, you are correct, similar issue with ax88179. There was once an
attempt to enable similar sanity checks there, see
https://lore.kernel.org/all/20220514133234.33796-1-k.kahurani@gmail.com/,
but for some reason it did not pan out.

As for other places with the same issue, after a quick glance I see these:

__ax88179_read_cmd - drivers/net/usb/ax88179_178a.c
cdc_ncm_init - drivers/net/usb/cdc_ncm.c
nc_vendor_read - drivers/net/usb/net1080.c
pla_read_word - drivers/net/usb/r8153_ecm.c
pla_write_word - drivers/net/usb/r8153_ecm.c
sierra_net_get_fw_attr - drivers/net/usb/sierra_net.c

This covers all instances usbnet_read_cmd[_nopm] that do not check for
full 'size' reads, only for straightforward errors. Roughly half of all
usbnet_read_cmd() calls kernel-wide.

> 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()?

I can't reliably state how normal it is when usbnet_read_cmd() ends up
reading less than size. Both aqc111 and syzkaller with its repros (and
ax88179/asix as well) tell us it does happen when it shouldn't.

Personally, while I see logic of moving this fix into
usbnet_read_cmd(), I am wary for the very reason you stated in your
question - sometimes it may be expected. Also, more often than not
functions that envelop usbnet_read_cmd() (like ax88179_read_cmd()) seem
to be deliberately ignoring return values, but even an early warning
may be helpful in such cases.

In other words, I'd rather leave the decision up to the maintainers. I
don't mind doing either option.

Regards,
Nikita

> 
> 	Andrew

Re: [PATCH net-next] net: usb: aqc111: fix error handling of usbnet read calls

[Paolo Abeni]

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

Re: [PATCH net-next] net: usb: aqc111: fix error handling of usbnet read calls

[patchwork-bot+netdevbpf]

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!
-- 
Deet-doot-dot, I am a bot.
https://korg.docs.kernel.org/patchwork/pwbot.html