diff mbox series

[v2] wifi: carl9170: add a proper sanity check for endpoints

Message ID 20240422183355.3785-1-n.zhandarovich@fintech.ru (mailing list archive)
State Accepted
Commit b6dd09b3dac89b45d1ea3e3bd035a3859c0369a0
Delegated to: Kalle Valo
Headers show
Series [v2] wifi: carl9170: add a proper sanity check for endpoints | expand

Commit Message

Nikita Zhandarovich April 22, 2024, 6:33 p.m. UTC 
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: syzbot+0ae4804973be759fa420@syzkaller.appspotmail.com
Fixes: a84fab3cbfdc ("carl9170: 802.11 rx/tx processing and usb backend")
Signed-off-by: Nikita Zhandarovich <n.zhandarovich@fintech.ru>
---
v2: as Christian Lamparter <chunkeey@gmail.com> 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(+)

Comments

Christian Lamparter April 25, 2024, 8:37 p.m. UTC | #1 
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 <chunkeey@gmail.com>

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: syzbot+0ae4804973be759fa420@syzkaller.appspotmail.com
> Fixes: a84fab3cbfdc ("carl9170: 802.11 rx/tx processing and usb backend")
> Signed-off-by: Nikita Zhandarovich <n.zhandarovich@fintech.ru>
> ---
> v2: as Christian Lamparter <chunkeey@gmail.com> 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);
>
Kalle Valo April 26, 2024, 4:58 a.m. UTC | #2 
Christian Lamparter <chunkeey@gmail.com> 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.
Christian Lamparter April 26, 2024, 5:02 p.m. UTC | #3 
On 4/26/24 6:58 AM, Kalle Valo wrote:
> Christian Lamparter <chunkeey@gmail.com> 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 <chunkeey@gmail.com>
Kalle Valo April 29, 2024, 5:07 p.m. UTC | #4 
Nikita Zhandarovich <n.zhandarovich@fintech.ru> 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: syzbot+0ae4804973be759fa420@syzkaller.appspotmail.com
> Fixes: a84fab3cbfdc ("carl9170: 802.11 rx/tx processing and usb backend")
> Signed-off-by: Nikita Zhandarovich <n.zhandarovich@fintech.ru>
> Acked-By: Christian Lamparter <chunkeey@gmail.com>
> Signed-off-by: Kalle Valo <quic_kvalo@quicinc.com>

Patch applied to ath-next branch of ath.git, thanks.

b6dd09b3dac8 wifi: carl9170: add a proper sanity check for endpoints
diff mbox series

Patch

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