| Message ID | 20220823222436.514204-1-mazinalhaddad05@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Johannes Berg |
| Headers | show |
| Series | ar5523: check endpoints type and direction in probe() | expand |
On Wed, Aug 24, 2022 at 01:24:38AM +0300, Mazin Al Haddad wrote: > Fixes a bug reported by syzbot, where a warning occurs in usb_submit_urb() > due to the wrong endpoint type. There is no check for both the number > of endpoints and the type which causes an error as the code tries to > send a URB to the wrong endpoint. > > Fix it by adding a check for the number of endpoints and the > direction/type of the endpoints. If the endpoints do not match the > expected configuration -ENODEV is returned. > > Syzkaller report: > > usb 1-1: BOGUS urb xfer, pipe 3 != type 1 > WARNING: CPU: 1 PID: 71 at drivers/usb/core/urb.c:502 usb_submit_urb+0xed2/0x18a0 drivers/usb/core/urb.c:502 > Modules linked in: > CPU: 1 PID: 71 Comm: kworker/1:2 Not tainted 5.19.0-rc7-syzkaller-00150-g32f02a211b0a #0 > Hardware name: Google Compute Engine/Google Compute Engine, BIOS Google 06/29/2022 > Workqueue: usb_hub_wq hub_event > Call Trace: > <TASK> > ar5523_cmd+0x420/0x790 drivers/net/wireless/ath/ar5523/ar5523.c:275 > ar5523_cmd_read drivers/net/wireless/ath/ar5523/ar5523.c:302 [inline] > ar5523_host_available drivers/net/wireless/ath/ar5523/ar5523.c:1376 [inline] > ar5523_probe+0xc66/0x1da0 drivers/net/wireless/ath/ar5523/ar5523.c:1655 > > > Link: https://syzkaller.appspot.com/bug?extid=1bc2c2afd44f820a669f > Reported-and-tested-by: syzbot+1bc2c2afd44f820a669f@syzkaller.appspotmail.com > Signed-off-by: Mazin Al Haddad <mazinalhaddad05@gmail.com> > --- > drivers/net/wireless/ath/ar5523/ar5523.c | 31 ++++++++++++++++++++++++ > 1 file changed, 31 insertions(+) > > diff --git a/drivers/net/wireless/ath/ar5523/ar5523.c b/drivers/net/wireless/ath/ar5523/ar5523.c > index 6f937d2cc126..5451bf9ab9fb 100644 > --- a/drivers/net/wireless/ath/ar5523/ar5523.c > +++ b/drivers/net/wireless/ath/ar5523/ar5523.c > @@ -1581,8 +1581,39 @@ static int ar5523_probe(struct usb_interface *intf, > struct usb_device *dev = interface_to_usbdev(intf); > struct ieee80211_hw *hw; > struct ar5523 *ar; > + struct usb_host_interface *host = intf->cur_altsetting; > int error = -ENOMEM; > > + if (host->desc.bNumEndpoints != 4) { > + dev_err(&dev->dev, "Wrong number of endpoints\n"); > + return -ENODEV; > + } > + > + for (int i = 0; i < host->desc.bNumEndpoints; ++i) { > + struct usb_endpoint_descriptor *ep = &host->endpoint[i].desc; > + // Check for type of endpoint and direction. > + switch (i) { > + case 0: > + case 1: > + if ((ep->bEndpointAddress & USB_DIR_OUT) && > + ((ep->bmAttributes & USB_ENDPOINT_XFERTYPE_MASK) > + == USB_ENDPOINT_XFER_BULK)){ Did you run your change through checkpatch? > + dev_err(&dev->dev, "Wrong type of endpoints\n"); > + return -ENODEV; > + } > + break; > + case 2: > + case 3: > + if ((ep->bEndpointAddress & USB_DIR_IN) && > + ((ep->bmAttributes & USB_ENDPOINT_XFERTYPE_MASK) > + == USB_ENDPOINT_XFER_BULK)){ > + dev_err(&dev->dev, "Wrong type of endpoints\n"); > + return -ENODEV; > + } > + break; > + } We have usb helper functions for all of this, why not use them instead of attempting to roll your own? thanks, greg k-h
On Wed Aug 24, 2022 at 9:04 AM +03, Greg KH wrote: > On Wed, Aug 24, 2022 at 01:24:38AM +0300, Mazin Al Haddad wrote: > > Fixes a bug reported by syzbot, where a warning occurs in usb_submit_urb() > > due to the wrong endpoint type. There is no check for both the number > > of endpoints and the type which causes an error as the code tries to > > send a URB to the wrong endpoint. > > > > Fix it by adding a check for the number of endpoints and the > > direction/type of the endpoints. If the endpoints do not match the > > expected configuration -ENODEV is returned. > > > > Syzkaller report: > > > > usb 1-1: BOGUS urb xfer, pipe 3 != type 1 > > WARNING: CPU: 1 PID: 71 at drivers/usb/core/urb.c:502 usb_submit_urb+0xed2/0x18a0 drivers/usb/core/urb.c:502 > > Modules linked in: > > CPU: 1 PID: 71 Comm: kworker/1:2 Not tainted 5.19.0-rc7-syzkaller-00150-g32f02a211b0a #0 > > Hardware name: Google Compute Engine/Google Compute Engine, BIOS Google 06/29/2022 > > Workqueue: usb_hub_wq hub_event > > Call Trace: > > <TASK> > > ar5523_cmd+0x420/0x790 drivers/net/wireless/ath/ar5523/ar5523.c:275 > > ar5523_cmd_read drivers/net/wireless/ath/ar5523/ar5523.c:302 [inline] > > ar5523_host_available drivers/net/wireless/ath/ar5523/ar5523.c:1376 [inline] > > ar5523_probe+0xc66/0x1da0 drivers/net/wireless/ath/ar5523/ar5523.c:1655 > > > > > > Link: https://syzkaller.appspot.com/bug?extid=1bc2c2afd44f820a669f > > Reported-and-tested-by: syzbot+1bc2c2afd44f820a669f@syzkaller.appspotmail.com > > Signed-off-by: Mazin Al Haddad <mazinalhaddad05@gmail.com> > > --- > > drivers/net/wireless/ath/ar5523/ar5523.c | 31 ++++++++++++++++++++++++ > > 1 file changed, 31 insertions(+) > > > > diff --git a/drivers/net/wireless/ath/ar5523/ar5523.c b/drivers/net/wireless/ath/ar5523/ar5523.c > > index 6f937d2cc126..5451bf9ab9fb 100644 > > --- a/drivers/net/wireless/ath/ar5523/ar5523.c > > +++ b/drivers/net/wireless/ath/ar5523/ar5523.c > > @@ -1581,8 +1581,39 @@ static int ar5523_probe(struct usb_interface *intf, > > struct usb_device *dev = interface_to_usbdev(intf); > > struct ieee80211_hw *hw; > > struct ar5523 *ar; > > + struct usb_host_interface *host = intf->cur_altsetting; > > int error = -ENOMEM; > > > > + if (host->desc.bNumEndpoints != 4) { > > + dev_err(&dev->dev, "Wrong number of endpoints\n"); > > + return -ENODEV; > > + } > > + > > + for (int i = 0; i < host->desc.bNumEndpoints; ++i) { > > + struct usb_endpoint_descriptor *ep = &host->endpoint[i].desc; > > + // Check for type of endpoint and direction. > > + switch (i) { > > + case 0: > > + case 1: > > + if ((ep->bEndpointAddress & USB_DIR_OUT) && > > + ((ep->bmAttributes & USB_ENDPOINT_XFERTYPE_MASK) > > + == USB_ENDPOINT_XFER_BULK)){ > > Did you run your change through checkpatch? Yes. > We have usb helper functions for all of this, why not use them instead > of attempting to roll your own? Using the helpers is indeed a lot better. I wasn't aware of all of them. Since find_common_endpoints() won't work here, I used the helpers for checking direction/type. I'll send v3 now. If there are any more changes that you feel are necessary please let me know, I'll be happy to incorporate them. Thanks!
On Sat, Aug 27, 2022 at 01:36:29PM +0300, Mazin Al Haddad wrote: > On Wed Aug 24, 2022 at 9:04 AM +03, Greg KH wrote: > > On Wed, Aug 24, 2022 at 01:24:38AM +0300, Mazin Al Haddad wrote: > > > Fixes a bug reported by syzbot, where a warning occurs in usb_submit_urb() > > > due to the wrong endpoint type. There is no check for both the number > > > of endpoints and the type which causes an error as the code tries to > > > send a URB to the wrong endpoint. > > > > > > Fix it by adding a check for the number of endpoints and the > > > direction/type of the endpoints. If the endpoints do not match the > > > expected configuration -ENODEV is returned. > > > > > > Syzkaller report: > > > > > > usb 1-1: BOGUS urb xfer, pipe 3 != type 1 > > > WARNING: CPU: 1 PID: 71 at drivers/usb/core/urb.c:502 usb_submit_urb+0xed2/0x18a0 drivers/usb/core/urb.c:502 > > > Modules linked in: > > > CPU: 1 PID: 71 Comm: kworker/1:2 Not tainted 5.19.0-rc7-syzkaller-00150-g32f02a211b0a #0 > > > Hardware name: Google Compute Engine/Google Compute Engine, BIOS Google 06/29/2022 > > > Workqueue: usb_hub_wq hub_event > > > Call Trace: > > > <TASK> > > > ar5523_cmd+0x420/0x790 drivers/net/wireless/ath/ar5523/ar5523.c:275 > > > ar5523_cmd_read drivers/net/wireless/ath/ar5523/ar5523.c:302 [inline] > > > ar5523_host_available drivers/net/wireless/ath/ar5523/ar5523.c:1376 [inline] > > > ar5523_probe+0xc66/0x1da0 drivers/net/wireless/ath/ar5523/ar5523.c:1655 > > > > > > > > > Link: https://syzkaller.appspot.com/bug?extid=1bc2c2afd44f820a669f > > > Reported-and-tested-by: syzbot+1bc2c2afd44f820a669f@syzkaller.appspotmail.com > > > Signed-off-by: Mazin Al Haddad <mazinalhaddad05@gmail.com> > > > --- > > > drivers/net/wireless/ath/ar5523/ar5523.c | 31 ++++++++++++++++++++++++ > > > 1 file changed, 31 insertions(+) > > > > > > diff --git a/drivers/net/wireless/ath/ar5523/ar5523.c b/drivers/net/wireless/ath/ar5523/ar5523.c > > > index 6f937d2cc126..5451bf9ab9fb 100644 > > > --- a/drivers/net/wireless/ath/ar5523/ar5523.c > > > +++ b/drivers/net/wireless/ath/ar5523/ar5523.c > > > @@ -1581,8 +1581,39 @@ static int ar5523_probe(struct usb_interface *intf, > > > struct usb_device *dev = interface_to_usbdev(intf); > > > struct ieee80211_hw *hw; > > > struct ar5523 *ar; > > > + struct usb_host_interface *host = intf->cur_altsetting; > > > int error = -ENOMEM; > > > > > > + if (host->desc.bNumEndpoints != 4) { > > > + dev_err(&dev->dev, "Wrong number of endpoints\n"); > > > + return -ENODEV; > > > + } > > > + > > > + for (int i = 0; i < host->desc.bNumEndpoints; ++i) { > > > + struct usb_endpoint_descriptor *ep = &host->endpoint[i].desc; > > > + // Check for type of endpoint and direction. > > > + switch (i) { > > > + case 0: > > > + case 1: > > > + if ((ep->bEndpointAddress & USB_DIR_OUT) && > > > + ((ep->bmAttributes & USB_ENDPOINT_XFERTYPE_MASK) > > > + == USB_ENDPOINT_XFER_BULK)){ > > > > Did you run your change through checkpatch? > > Yes. > > > We have usb helper functions for all of this, why not use them instead > > of attempting to roll your own? > > Using the helpers is indeed a lot better. I wasn't aware of all of them. > Since find_common_endpoints() won't work here, I used the helpers for > checking direction/type. I don't understand why usb_find_common_endpoints() will not work here. It seems to be very generic and should work just fine. thanks, greg k-h
diff --git a/drivers/net/wireless/ath/ar5523/ar5523.c b/drivers/net/wireless/ath/ar5523/ar5523.c index 6f937d2cc126..5451bf9ab9fb 100644 --- a/drivers/net/wireless/ath/ar5523/ar5523.c +++ b/drivers/net/wireless/ath/ar5523/ar5523.c @@ -1581,8 +1581,39 @@ static int ar5523_probe(struct usb_interface *intf, struct usb_device *dev = interface_to_usbdev(intf); struct ieee80211_hw *hw; struct ar5523 *ar; + struct usb_host_interface *host = intf->cur_altsetting; int error = -ENOMEM; + if (host->desc.bNumEndpoints != 4) { + dev_err(&dev->dev, "Wrong number of endpoints\n"); + return -ENODEV; + } + + for (int i = 0; i < host->desc.bNumEndpoints; ++i) { + struct usb_endpoint_descriptor *ep = &host->endpoint[i].desc; + // Check for type of endpoint and direction. + switch (i) { + case 0: + case 1: + if ((ep->bEndpointAddress & USB_DIR_OUT) && + ((ep->bmAttributes & USB_ENDPOINT_XFERTYPE_MASK) + == USB_ENDPOINT_XFER_BULK)){ + dev_err(&dev->dev, "Wrong type of endpoints\n"); + return -ENODEV; + } + break; + case 2: + case 3: + if ((ep->bEndpointAddress & USB_DIR_IN) && + ((ep->bmAttributes & USB_ENDPOINT_XFERTYPE_MASK) + == USB_ENDPOINT_XFER_BULK)){ + dev_err(&dev->dev, "Wrong type of endpoints\n"); + return -ENODEV; + } + break; + } + } + /* * Load firmware if the device requires it. This will return * -ENXIO on success and we'll get called back afer the usb
Fixes a bug reported by syzbot, where a warning occurs in usb_submit_urb() due to the wrong endpoint type. There is no check for both the number of endpoints and the type which causes an error as the code tries to send a URB to the wrong endpoint. Fix it by adding a check for the number of endpoints and the direction/type of the endpoints. If the endpoints do not match the expected configuration -ENODEV is returned. Syzkaller report: usb 1-1: BOGUS urb xfer, pipe 3 != type 1 WARNING: CPU: 1 PID: 71 at drivers/usb/core/urb.c:502 usb_submit_urb+0xed2/0x18a0 drivers/usb/core/urb.c:502 Modules linked in: CPU: 1 PID: 71 Comm: kworker/1:2 Not tainted 5.19.0-rc7-syzkaller-00150-g32f02a211b0a #0 Hardware name: Google Compute Engine/Google Compute Engine, BIOS Google 06/29/2022 Workqueue: usb_hub_wq hub_event Call Trace: <TASK> ar5523_cmd+0x420/0x790 drivers/net/wireless/ath/ar5523/ar5523.c:275 ar5523_cmd_read drivers/net/wireless/ath/ar5523/ar5523.c:302 [inline] ar5523_host_available drivers/net/wireless/ath/ar5523/ar5523.c:1376 [inline] ar5523_probe+0xc66/0x1da0 drivers/net/wireless/ath/ar5523/ar5523.c:1655 Link: https://syzkaller.appspot.com/bug?extid=1bc2c2afd44f820a669f Reported-and-tested-by: syzbot+1bc2c2afd44f820a669f@syzkaller.appspotmail.com Signed-off-by: Mazin Al Haddad <mazinalhaddad05@gmail.com> --- drivers/net/wireless/ath/ar5523/ar5523.c | 31 ++++++++++++++++++++++++ 1 file changed, 31 insertions(+)