Syzkaller reports [1] hitting a warning which is caused by presence
of a wrong endpoint type at the URB sumbitting stage. While there
was a check for a specific 4th endpoint, since it can switch types
between bulk and interrupt, other endpoints are trusted implicitly.
Similar warning is triggered in a couple of other syzbot issues [2].
Fix the issue by doing a comprehensive check of all endpoints
taking into account difference between high- and full-speed
configuration.
This patch has not been tested on real hardware.
[1] Syzkaller report:
...
WARNING: CPU: 0 PID: 4721 at drivers/usb/core/urb.c:504 usb_submit_urb+0xed6/0x1880 drivers/usb/core/urb.c:504
...
Call Trace:
<TASK>
carl9170_usb_send_rx_irq_urb+0x273/0x340 drivers/net/wireless/ath/carl9170/usb.c:504
carl9170_usb_init_device drivers/net/wireless/ath/carl9170/usb.c:939 [inline]
carl9170_usb_firmware_finish drivers/net/wireless/ath/carl9170/usb.c:999 [inline]
carl9170_usb_firmware_step2+0x175/0x240 drivers/net/wireless/ath/carl9170/usb.c:1028
request_firmware_work_func+0x130/0x240 drivers/base/firmware_loader/main.c:1107
process_one_work+0x9bf/0x1710 kernel/workqueue.c:2289
worker_thread+0x669/0x1090 kernel/workqueue.c:2436
kthread+0x2e8/0x3a0 kernel/kthread.c:376
ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:308
</TASK>
[2] Related syzkaller crashes:
Link: https://syzkaller.appspot.com/bug?extid=e394db78ae0b0032cb4d
Link: https://syzkaller.appspot.com/bug?extid=9468df99cb63a4a4c4e1
Reported-and-tested-by: [email protected]
Fixes: a84fab3cbfdc ("carl9170: 802.11 rx/tx processing and usb backend")
Signed-off-by: Nikita Zhandarovich <[email protected]>
---
v2: as Christian Lamparter <[email protected]> was kind to point out,
before returning with error, make sure to free previously allocated
'ar' with carl9170_free(ar).
drivers/net/wireless/ath/carl9170/usb.c | 32 ++++++++++++++++++++++++++++++++
1 file changed, 32 insertions(+)
diff --git a/drivers/net/wireless/ath/carl9170/usb.c b/drivers/net/wireless/ath/carl9170/usb.c
index c4edf8355941..a3e03580cd9f 100644
--- a/drivers/net/wireless/ath/carl9170/usb.c
+++ b/drivers/net/wireless/ath/carl9170/usb.c
@@ -1069,6 +1069,38 @@ static int carl9170_usb_probe(struct usb_interface *intf,
ar->usb_ep_cmd_is_bulk = true;
}
+ /* Verify that all expected endpoints are present */
+ if (ar->usb_ep_cmd_is_bulk) {
+ u8 bulk_ep_addr[] = {
+ AR9170_USB_EP_RX | USB_DIR_IN,
+ AR9170_USB_EP_TX | USB_DIR_OUT,
+ AR9170_USB_EP_CMD | USB_DIR_OUT,
+ 0};
+ u8 int_ep_addr[] = {
+ AR9170_USB_EP_IRQ | USB_DIR_IN,
+ 0};
+ if (!usb_check_bulk_endpoints(intf, bulk_ep_addr) ||
+ !usb_check_int_endpoints(intf, int_ep_addr))
+ err = -ENODEV;
+ } else {
+ u8 bulk_ep_addr[] = {
+ AR9170_USB_EP_RX | USB_DIR_IN,
+ AR9170_USB_EP_TX | USB_DIR_OUT,
+ 0};
+ u8 int_ep_addr[] = {
+ AR9170_USB_EP_IRQ | USB_DIR_IN,
+ AR9170_USB_EP_CMD | USB_DIR_OUT,
+ 0};
+ if (!usb_check_bulk_endpoints(intf, bulk_ep_addr) ||
+ !usb_check_int_endpoints(intf, int_ep_addr))
+ err = -ENODEV;
+ }
+
+ if (err) {
+ carl9170_free(ar);
+ return err;
+ }
+
usb_set_intfdata(intf, ar);
SET_IEEE80211_DEV(ar->hw, &intf->dev);
On 4/22/24 8:33 PM, Nikita Zhandarovich wrote:
> Syzkaller reports [1] hitting a warning which is caused by presence
> of a wrong endpoint type at the URB sumbitting stage. While there
> was a check for a specific 4th endpoint, since it can switch types
> between bulk and interrupt, other endpoints are trusted implicitly.
> Similar warning is triggered in a couple of other syzbot issues [2].
>
> Fix the issue by doing a comprehensive check of all endpoints
> taking into account difference between high- and full-speed
> configuration.
>
> This patch has not been tested on real hardware.
Oh, I've tested the original patch on real hardware ;). You can remove that line.
USB: 0846:9010 NetGear, Inc. WNDA3100v1 802.11abgn [Atheros AR9170+AR9104]
USB: 0CF3:1002 Atheros Communications, Inc. TP-Link TL-WN821N v2 / TL-WN822N v1 802.11n [Atheros AR9170]
With both high- and full-speed configuration on two different hcds.
In both cases the driver works the same as before and the interface comes up.
I can retest this patch tomorrow/saturday in case you want to wait around.
But I don't "see" how this can go wrong.
Acked-By: Christian Lamparter <[email protected]>
I assume the "Reported-and-tested" means that syzbot has verified that with
this patch, it can no longer get the USB-core to throw a warning, right?
<https://syzkaller.appspot.com/bug?extid=e394db78ae0b0032cb4d> says under
"Last patch testing requests) that it tested on the 2024/04/17 and the result
was "error OK"? )
Cheers,
Christian
>
> [1] Syzkaller report:
> ...
> WARNING: CPU: 0 PID: 4721 at drivers/usb/core/urb.c:504 usb_submit_urb+0xed6/0x1880 drivers/usb/core/urb.c:504
> ...
> Call Trace:
> <TASK>
> carl9170_usb_send_rx_irq_urb+0x273/0x340 drivers/net/wireless/ath/carl9170/usb.c:504
> carl9170_usb_init_device drivers/net/wireless/ath/carl9170/usb.c:939 [inline]
> carl9170_usb_firmware_finish drivers/net/wireless/ath/carl9170/usb.c:999 [inline]
> carl9170_usb_firmware_step2+0x175/0x240 drivers/net/wireless/ath/carl9170/usb.c:1028
> request_firmware_work_func+0x130/0x240 drivers/base/firmware_loader/main.c:1107
> process_one_work+0x9bf/0x1710 kernel/workqueue.c:2289
> worker_thread+0x669/0x1090 kernel/workqueue.c:2436
> kthread+0x2e8/0x3a0 kernel/kthread.c:376
> ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:308
> </TASK>
>
> [2] Related syzkaller crashes:
> Link: https://syzkaller.appspot.com/bug?extid=e394db78ae0b0032cb4d
> Link: https://syzkaller.appspot.com/bug?extid=9468df99cb63a4a4c4e1
>
> Reported-and-tested-by: [email protected]
> Fixes: a84fab3cbfdc ("carl9170: 802.11 rx/tx processing and usb backend")
> Signed-off-by: Nikita Zhandarovich <[email protected]>
> ---
> v2: as Christian Lamparter <[email protected]> was kind to point out,
> before returning with error, make sure to free previously allocated
> 'ar' with carl9170_free(ar).
>
> drivers/net/wireless/ath/carl9170/usb.c | 32 ++++++++++++++++++++++++++++++++
> 1 file changed, 32 insertions(+)
>
> diff --git a/drivers/net/wireless/ath/carl9170/usb.c b/drivers/net/wireless/ath/carl9170/usb.c
> index c4edf8355941..a3e03580cd9f 100644
> --- a/drivers/net/wireless/ath/carl9170/usb.c
> +++ b/drivers/net/wireless/ath/carl9170/usb.c
> @@ -1069,6 +1069,38 @@ static int carl9170_usb_probe(struct usb_interface *intf,
> ar->usb_ep_cmd_is_bulk = true;
> }
>
> + /* Verify that all expected endpoints are present */
> + if (ar->usb_ep_cmd_is_bulk) {
> + u8 bulk_ep_addr[] = {
> + AR9170_USB_EP_RX | USB_DIR_IN,
> + AR9170_USB_EP_TX | USB_DIR_OUT,
> + AR9170_USB_EP_CMD | USB_DIR_OUT,
> + 0};
> + u8 int_ep_addr[] = {
> + AR9170_USB_EP_IRQ | USB_DIR_IN,
> + 0};
> + if (!usb_check_bulk_endpoints(intf, bulk_ep_addr) ||
> + !usb_check_int_endpoints(intf, int_ep_addr))
> + err = -ENODEV;
> + } else {
> + u8 bulk_ep_addr[] = {
> + AR9170_USB_EP_RX | USB_DIR_IN,
> + AR9170_USB_EP_TX | USB_DIR_OUT,
> + 0};
> + u8 int_ep_addr[] = {
> + AR9170_USB_EP_IRQ | USB_DIR_IN,
> + AR9170_USB_EP_CMD | USB_DIR_OUT,
> + 0};
> + if (!usb_check_bulk_endpoints(intf, bulk_ep_addr) ||
> + !usb_check_int_endpoints(intf, int_ep_addr))
> + err = -ENODEV;
> + }
> +
> + if (err) {
> + carl9170_free(ar);
> + return err;
> + }
> +
> usb_set_intfdata(intf, ar);
> SET_IEEE80211_DEV(ar->hw, &intf->dev);
>
Christian Lamparter <[email protected]> writes:
> On 4/22/24 8:33 PM, Nikita Zhandarovich wrote:
>> Syzkaller reports [1] hitting a warning which is caused by presence
>> of a wrong endpoint type at the URB sumbitting stage. While there
>> was a check for a specific 4th endpoint, since it can switch types
>> between bulk and interrupt, other endpoints are trusted implicitly.
>> Similar warning is triggered in a couple of other syzbot issues [2].
>> Fix the issue by doing a comprehensive check of all endpoints
>> taking into account difference between high- and full-speed
>> configuration.
>> This patch has not been tested on real hardware.
>
> Oh, I've tested the original patch on real hardware ;). You can remove that line.
BTW I can remove that line, no need to resend because of this.
--
https://patchwork.kernel.org/project/linux-wireless/list/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches
On 4/26/24 6:58 AM, Kalle Valo wrote:
> Christian Lamparter <[email protected]> writes:
>
>> On 4/22/24 8:33 PM, Nikita Zhandarovich wrote:
>>> Syzkaller reports [1] hitting a warning which is caused by presence
>>> of a wrong endpoint type at the URB sumbitting stage. While there
>>> was a check for a specific 4th endpoint, since it can switch types
>>> between bulk and interrupt, other endpoints are trusted implicitly.
>>> Similar warning is triggered in a couple of other syzbot issues [2].
>>> Fix the issue by doing a comprehensive check of all endpoints
>>> taking into account difference between high- and full-speed
>>> configuration.
>>> This patch has not been tested on real hardware.
>>
>> Oh, I've tested the original patch on real hardware ;). You can remove that line.
>
> BTW I can remove that line, no need to resend because of this.
>
finished testing v2 - it works, no surprise.
high-speed:
| [ 188.470363] usb 3-14: new high-speed USB device number 4 using xhci_hcd
| [ 188.661053] usb 3-14: New USB device found, idVendor=0846, idProduct=9010, bcdDevice= 1.06
| [ 188.661056] usb 3-14: New USB device strings: Mfr=16, Product=32, SerialNumber=48
| [ 188.661058] usb 3-14: Product: USB2.0 WLAN
| [ 188.661059] usb 3-14: Manufacturer: ATHER
| [ 188.661060] usb 3-14: SerialNumber: 12345
| [ 188.783843] usb 3-14: reset high-speed USB device number 4 using xhci_hcd
| [ 188.963408] usb 3-14: driver API: 1.9.9 2016-02-15 [1-1]
| [ 188.963412] usb 3-14: firmware API: 1.9.6 2012-07-07
| [ 189.298218] ath: EEPROM regdomain: 0x0
| [ 189.298221] ath: EEPROM indicates default country code should be used
| [ 189.298222] ath: doing EEPROM country->regdmn map search
| [ 189.298223] ath: country maps to regdmn code: 0x3a
| [ 189.298224] ath: Country alpha2 being used: US
| [ 189.298225] ath: Regpair used: 0x3a
| [ 189.298290] ieee80211 phy2: Selected rate control algorithm 'minstrel_ht'
| [ 189.301463] usb 3-14: Atheros AR9170 is registered as 'phy2'
full-speed:
| [ 205.743314] usb 4-2: new full-speed USB device number 3 using ohci-pci
| [ 205.990614] usb 4-2: not running at top speed; connect to a high speed hub
| [ 206.029618] usb 4-2: New USB device found, idVendor=0cf3, idProduct=1002, bcdDevice= 1.06
| [ 206.029621] usb 4-2: New USB device strings: Mfr=16, Product=32, SerialNumber=48
| [ 206.029622] usb 4-2: Product: USB2.0 WLAN
| [ 206.029623] usb 4-2: Manufacturer: ATHER
| [ 206.029624] usb 4-2: SerialNumber: 12345
| [ 206.209969] usb 4-2: reset full-speed USB device number 3 using ohci-pci
| [ 206.471776] usb 4-2: driver API: 1.9.9 2016-02-15 [1-1]
| [ 206.471779] usb 4-2: firmware API: 1.9.6 2012-07-07
| [ 206.885680] ath: EEPROM regdomain: 0x809c
| [ 206.885684] ath: EEPROM indicates we should expect a country code
| [ 206.885686] ath: doing EEPROM country->regdmn map search
| [ 206.885687] ath: country maps to regdmn code: 0x52
| [ 206.885689] ath: Country alpha2 being used: CN
| [ 206.885691] ath: Regpair used: 0x52
| [ 206.885794] ieee80211 phy3: Selected rate control algorithm 'minstrel_ht'
| [ 206.888834] input: phy3 WPS Button as /devices/pci0000:00/0000:00:1c.3/0000:05:00.0/0000:06:01.0/usb4/4-2/4-2:1.0/ieee80211/phy3/input17
| [ 206.892694] usb 4-2: Atheros AR9170 is registered as 'phy3'
Tested-by: Christian Lamparter <[email protected]>
Nikita Zhandarovich <[email protected]> wrote:
> Syzkaller reports [1] hitting a warning which is caused by presence
> of a wrong endpoint type at the URB sumbitting stage. While there
> was a check for a specific 4th endpoint, since it can switch types
> between bulk and interrupt, other endpoints are trusted implicitly.
> Similar warning is triggered in a couple of other syzbot issues [2].
>
> Fix the issue by doing a comprehensive check of all endpoints
> taking into account difference between high- and full-speed
> configuration.
>
> [1] Syzkaller report:
> ...
> WARNING: CPU: 0 PID: 4721 at drivers/usb/core/urb.c:504 usb_submit_urb+0xed6/0x1880 drivers/usb/core/urb.c:504
> ...
> Call Trace:
> <TASK>
> carl9170_usb_send_rx_irq_urb+0x273/0x340 drivers/net/wireless/ath/carl9170/usb.c:504
> carl9170_usb_init_device drivers/net/wireless/ath/carl9170/usb.c:939 [inline]
> carl9170_usb_firmware_finish drivers/net/wireless/ath/carl9170/usb.c:999 [inline]
> carl9170_usb_firmware_step2+0x175/0x240 drivers/net/wireless/ath/carl9170/usb.c:1028
> request_firmware_work_func+0x130/0x240 drivers/base/firmware_loader/main.c:1107
> process_one_work+0x9bf/0x1710 kernel/workqueue.c:2289
> worker_thread+0x669/0x1090 kernel/workqueue.c:2436
> kthread+0x2e8/0x3a0 kernel/kthread.c:376
> ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:308
> </TASK>
>
> [2] Related syzkaller crashes:
> Link: https://syzkaller.appspot.com/bug?extid=e394db78ae0b0032cb4d
> Link: https://syzkaller.appspot.com/bug?extid=9468df99cb63a4a4c4e1
>
> Reported-and-tested-by: [email protected]
> Fixes: a84fab3cbfdc ("carl9170: 802.11 rx/tx processing and usb backend")
> Signed-off-by: Nikita Zhandarovich <[email protected]>
> Acked-By: Christian Lamparter <[email protected]>
> Signed-off-by: Kalle Valo <[email protected]>
Patch applied to ath-next branch of ath.git, thanks.
b6dd09b3dac8 wifi: carl9170: add a proper sanity check for endpoints
--
https://patchwork.kernel.org/project/linux-wireless/patch/[email protected]/
https://wireless.wiki.kernel.org/en/developers/documentation/submittingpatches