| Message ID | 20220307185314.11228-1-paskripkin@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Netdev Maintainers |
| Headers | show |
| Series | [RFT] can: mcba_usb: properly check endpoint type | expand |
| Context | Check | Description |
|---|---|---|
| netdev/tree_selection | success | Series ignored based on subject |
Hi Pavel, > [PATCH RFT] can: mcba_usb: properly check endpoint type It is RFC, not RFT :) I guess you went on some manual editing. Next time, you can just let git add the tag for you by doing: | git format-patch --rfc ... On Tue. 8 Mar 2022, 03:53, Pavel Skripkin <paskripkin@gmail.com> wrote: > Syzbot reported warning in usb_submit_urb() which is caused by wrong > endpoint type. We should check that in endpoint is actually present to > prevent this warning > > Fail log: > > usb 5-1: BOGUS urb xfer, pipe 3 != type 1 > WARNING: CPU: 1 PID: 49 at drivers/usb/core/urb.c:502 usb_submit_urb+0xed2/0x18a0 drivers/usb/core/urb.c:502 > Modules linked in: > CPU: 1 PID: 49 Comm: kworker/1:2 Not tainted 5.17.0-rc6-syzkaller-00184-g38f80f42147f #0 > Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.14.0-2 04/01/2014 > Workqueue: usb_hub_wq hub_event > RIP: 0010:usb_submit_urb+0xed2/0x18a0 drivers/usb/core/urb.c:502 > ... > Call Trace: > <TASK> > mcba_usb_start drivers/net/can/usb/mcba_usb.c:662 [inline] > mcba_usb_probe+0x8a3/0xc50 drivers/net/can/usb/mcba_usb.c:858 > usb_probe_interface+0x315/0x7f0 drivers/usb/core/driver.c:396 > call_driver_probe drivers/base/dd.c:517 [inline] > > Reported-and-tested-by: syzbot+3bc1dce0cc0052d60fde@syzkaller.appspotmail.com > Fixes: 51f3baad7de9 ("can: mcba_usb: Add support for Microchip CAN BUS Analyzer") > Signed-off-by: Pavel Skripkin <paskripkin@gmail.com> > --- > > Meta comments: > > I am not an usb expert, but looks like this driver uses one > endpoint for in and out transactions: > > /* MCBA endpoint numbers */ > #define MCBA_USB_EP_IN 1 > #define MCBA_USB_EP_OUT 1 > > That's why check only for in endpoint is added MCBA_USB_EP_{IN,OUT} are respectively used in usb_rcvbulkpipe() and usb_sndbulkpipe(). I invite you to have a look at what those macros do and you will understand that these returns two different pipes: https://elixir.bootlin.com/linux/latest/source/include/linux/usb.h#L1964 In other words, ep_in and ep_out are some indexes of a different entity and do not conflict with each other. > --- > drivers/net/can/usb/mcba_usb.c | 7 +++++++ > 1 file changed, 7 insertions(+) > > diff --git a/drivers/net/can/usb/mcba_usb.c b/drivers/net/can/usb/mcba_usb.c > index 77bddff86252..646aac1a8684 100644 > --- a/drivers/net/can/usb/mcba_usb.c > +++ b/drivers/net/can/usb/mcba_usb.c > @@ -807,6 +807,13 @@ static int mcba_usb_probe(struct usb_interface *intf, > struct mcba_priv *priv; > int err; > struct usb_device *usbdev = interface_to_usbdev(intf); > + struct usb_endpoint_descriptor *in; > + > + err = usb_find_common_endpoints(intf->cur_altsetting, &in, NULL, NULL, NULL); If you go this direction, then please use usb_find_common_endpoint() to retrieve the value of both ep_in and ep_out and use them instead of MCBA_USB_EP_{IN,OUT} > + if (err) { > + dev_err(&intf->dev, "Can't find endpoints\n"); > + return -ENODEV; return ret; Please keep the error code of usb_find_common_endpoint(). > + } > > > > netdev = alloc_candev(sizeof(struct mcba_priv), MCBA_MAX_TX_URBS); > if (!netdev) { Yours sincerely, Vincent Mailhol
Hi Vincent, On 3/8/22 03:23, Vincent MAILHOL wrote: > Hi Pavel, > >> [PATCH RFT] can: mcba_usb: properly check endpoint type > It is RFC, not RFT :) > I guess you went on some manual editing. Next time, you can just let > git add the tag for you by doing: > | git format-patch --rfc ... > I marked it as RFT, because I wanted someone to test it. But indeed with my lack of usb knowledge it should have been RFC :) > > On Tue. 8 Mar 2022, 03:53, Pavel Skripkin <paskripkin@gmail.com> wrote: [snip] >> /* MCBA endpoint numbers */ >> #define MCBA_USB_EP_IN 1 >> #define MCBA_USB_EP_OUT 1 >> >> That's why check only for in endpoint is added > > MCBA_USB_EP_{IN,OUT} are respectively used in usb_rcvbulkpipe() > and usb_sndbulkpipe(). I invite you to have a look at what those > macros do and you will understand that these returns two different > pipes: > > https://elixir.bootlin.com/linux/latest/source/include/linux/usb.h#L1964 > > In other words, ep_in and ep_out are some indexes of a different > entity and do not conflict with each other. > Got it! Thank you for pointing out! >> --- >> drivers/net/can/usb/mcba_usb.c | 7 +++++++ >> 1 file changed, 7 insertions(+) >> >> diff --git a/drivers/net/can/usb/mcba_usb.c b/drivers/net/can/usb/mcba_usb.c >> index 77bddff86252..646aac1a8684 100644 >> --- a/drivers/net/can/usb/mcba_usb.c >> +++ b/drivers/net/can/usb/mcba_usb.c >> @@ -807,6 +807,13 @@ static int mcba_usb_probe(struct usb_interface *intf, >> struct mcba_priv *priv; >> int err; >> struct usb_device *usbdev = interface_to_usbdev(intf); >> + struct usb_endpoint_descriptor *in; >> + >> + err = usb_find_common_endpoints(intf->cur_altsetting, &in, NULL, NULL, NULL); > > If you go this direction, then please use > usb_find_common_endpoint() to retrieve the value of both ep_in > and ep_out and use them instead of MCBA_USB_EP_{IN,OUT} > >> + if (err) { >> + dev_err(&intf->dev, "Can't find endpoints\n"); >> + return -ENODEV; > > return ret; > > Please keep the error code of usb_find_common_endpoint(). > Will redo in v2. Thank you for reviewing With regards, Pavel Skripkin
Hi Pavel, On Tue, Mar 8, 2022 at 5:06 PM Pavel Skripkin <paskripkin@gmail.com> wrote: > On 3/8/22 03:23, Vincent MAILHOL wrote: > >> [PATCH RFT] can: mcba_usb: properly check endpoint type > > It is RFC, not RFT :) > > I guess you went on some manual editing. Next time, you can just let > > git add the tag for you by doing: > > | git format-patch --rfc ... > > > > I marked it as RFT, because I wanted someone to test it. But indeed with > my lack of usb knowledge it should have been RFC :) l didn't know RFT to mean "Request for Testing" :D I have the device and do testing. Do you have test code I can run? Best Regards,
Hi Yasushi, On 3/9/22 05:58, Yasushi SHOJI wrote: > Hi Pavel, > > On Tue, Mar 8, 2022 at 5:06 PM Pavel Skripkin <paskripkin@gmail.com> wrote: >> On 3/8/22 03:23, Vincent MAILHOL wrote: >> >> [PATCH RFT] can: mcba_usb: properly check endpoint type >> > It is RFC, not RFT :) >> > I guess you went on some manual editing. Next time, you can just let >> > git add the tag for you by doing: >> > | git format-patch --rfc ... >> > >> >> I marked it as RFT, because I wanted someone to test it. But indeed with >> my lack of usb knowledge it should have been RFC :) > > l didn't know RFT to mean "Request for Testing" :D > > I have the device and do testing. Do you have test code I can run? > Sorry for late reply, this mail got somehow lost in my inbox. I don't have any code and even this device. I think, if with this patch applied driver probes and in-out stream works nice then this patch is correct :) With regards, Pavel Skripkin
diff --git a/drivers/net/can/usb/mcba_usb.c b/drivers/net/can/usb/mcba_usb.c index 77bddff86252..646aac1a8684 100644 --- a/drivers/net/can/usb/mcba_usb.c +++ b/drivers/net/can/usb/mcba_usb.c @@ -807,6 +807,13 @@ static int mcba_usb_probe(struct usb_interface *intf, struct mcba_priv *priv; int err; struct usb_device *usbdev = interface_to_usbdev(intf); + struct usb_endpoint_descriptor *in; + + err = usb_find_common_endpoints(intf->cur_altsetting, &in, NULL, NULL, NULL); + if (err) { + dev_err(&intf->dev, "Can't find endpoints\n"); + return -ENODEV; + } netdev = alloc_candev(sizeof(struct mcba_priv), MCBA_MAX_TX_URBS); if (!netdev) {
Syzbot reported warning in usb_submit_urb() which is caused by wrong endpoint type. We should check that in endpoint is actually present to prevent this warning Fail log: usb 5-1: BOGUS urb xfer, pipe 3 != type 1 WARNING: CPU: 1 PID: 49 at drivers/usb/core/urb.c:502 usb_submit_urb+0xed2/0x18a0 drivers/usb/core/urb.c:502 Modules linked in: CPU: 1 PID: 49 Comm: kworker/1:2 Not tainted 5.17.0-rc6-syzkaller-00184-g38f80f42147f #0 Hardware name: QEMU Standard PC (Q35 + ICH9, 2009), BIOS 1.14.0-2 04/01/2014 Workqueue: usb_hub_wq hub_event RIP: 0010:usb_submit_urb+0xed2/0x18a0 drivers/usb/core/urb.c:502 ... Call Trace: <TASK> mcba_usb_start drivers/net/can/usb/mcba_usb.c:662 [inline] mcba_usb_probe+0x8a3/0xc50 drivers/net/can/usb/mcba_usb.c:858 usb_probe_interface+0x315/0x7f0 drivers/usb/core/driver.c:396 call_driver_probe drivers/base/dd.c:517 [inline] Reported-and-tested-by: syzbot+3bc1dce0cc0052d60fde@syzkaller.appspotmail.com Fixes: 51f3baad7de9 ("can: mcba_usb: Add support for Microchip CAN BUS Analyzer") Signed-off-by: Pavel Skripkin <paskripkin@gmail.com> --- Meta comments: I am not an usb expert, but looks like this driver uses one endpoint for in and out transactions: /* MCBA endpoint numbers */ #define MCBA_USB_EP_IN 1 #define MCBA_USB_EP_OUT 1 That's why check only for in endpoint is added --- drivers/net/can/usb/mcba_usb.c | 7 +++++++ 1 file changed, 7 insertions(+)