* [PATCH RFT] can: mcba_usb: properly check endpoint type @ 2022-03-07 18:53 Pavel Skripkin 2022-03-08 0:23 ` Vincent MAILHOL 0 siblings, 1 reply; 9+ messages in thread From: Pavel Skripkin @ 2022-03-07 18:53 UTC (permalink / raw) To: yashi, wg, mkl, davem, kuba, mailhol.vincent Cc: linux-can, netdev, linux-kernel, Pavel Skripkin, syzbot+3bc1dce0cc0052d60fde 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(+) 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) { -- 2.35.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH RFT] can: mcba_usb: properly check endpoint type 2022-03-07 18:53 [PATCH RFT] can: mcba_usb: properly check endpoint type Pavel Skripkin @ 2022-03-08 0:23 ` Vincent MAILHOL 2022-03-08 8:06 ` Pavel Skripkin 2022-03-08 8:16 ` [PATCH v2] " Pavel Skripkin 0 siblings, 2 replies; 9+ messages in thread From: Vincent MAILHOL @ 2022-03-08 0:23 UTC (permalink / raw) To: Pavel Skripkin Cc: Yasushi SHOJI, Wolfgang Grandegger, Marc Kleine-Budde, David Miller, Jakub Kicinski, linux-can, netdev, open list, syzbot+3bc1dce0cc0052d60fde 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH RFT] can: mcba_usb: properly check endpoint type 2022-03-08 0:23 ` Vincent MAILHOL @ 2022-03-08 8:06 ` Pavel Skripkin 2022-03-09 2:58 ` Yasushi SHOJI 2022-03-08 8:16 ` [PATCH v2] " Pavel Skripkin 1 sibling, 1 reply; 9+ messages in thread From: Pavel Skripkin @ 2022-03-08 8:06 UTC (permalink / raw) To: Vincent MAILHOL Cc: Yasushi SHOJI, Wolfgang Grandegger, Marc Kleine-Budde, David Miller, Jakub Kicinski, linux-can, netdev, open list, syzbot+3bc1dce0cc0052d60fde 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH RFT] can: mcba_usb: properly check endpoint type 2022-03-08 8:06 ` Pavel Skripkin @ 2022-03-09 2:58 ` Yasushi SHOJI 2022-03-13 9:41 ` Pavel Skripkin 0 siblings, 1 reply; 9+ messages in thread From: Yasushi SHOJI @ 2022-03-09 2:58 UTC (permalink / raw) To: Pavel Skripkin Cc: Vincent MAILHOL, Wolfgang Grandegger, Marc Kleine-Budde, David Miller, Jakub Kicinski, linux-can, netdev, open list, syzbot+3bc1dce0cc0052d60fde 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, -- yashi ^ permalink raw reply [flat|nested] 9+ messages in thread
* Re: [PATCH RFT] can: mcba_usb: properly check endpoint type 2022-03-09 2:58 ` Yasushi SHOJI @ 2022-03-13 9:41 ` Pavel Skripkin 0 siblings, 0 replies; 9+ messages in thread From: Pavel Skripkin @ 2022-03-13 9:41 UTC (permalink / raw) To: Yasushi SHOJI Cc: Vincent MAILHOL, Wolfgang Grandegger, Marc Kleine-Budde, David Miller, Jakub Kicinski, linux-can, netdev, open list, syzbot+3bc1dce0cc0052d60fde 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 ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v2] can: mcba_usb: properly check endpoint type 2022-03-08 0:23 ` Vincent MAILHOL 2022-03-08 8:06 ` Pavel Skripkin @ 2022-03-08 8:16 ` Pavel Skripkin 2022-03-08 8:54 ` Vincent MAILHOL 1 sibling, 1 reply; 9+ messages in thread From: Pavel Skripkin @ 2022-03-08 8:16 UTC (permalink / raw) To: yashi, wg, mkl, davem, kuba, mailhol.vincent Cc: linux-can, netdev, linux-kernel, Pavel Skripkin, syzbot+3bc1dce0cc0052d60fde 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 Found pipes are now saved to struct mcba_priv and code uses them directly instead of making pipes in place. 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> --- Changes from RFT(RFC): - Add missing out pipe check - Use found pipes instead of making pipes in place - Do not hide usb_find_common_endpoints() error --- drivers/net/can/usb/mcba_usb.c | 22 +++++++++++++++------- 1 file changed, 15 insertions(+), 7 deletions(-) diff --git a/drivers/net/can/usb/mcba_usb.c b/drivers/net/can/usb/mcba_usb.c index 77bddff86252..91e79a2d5ae5 100644 --- a/drivers/net/can/usb/mcba_usb.c +++ b/drivers/net/can/usb/mcba_usb.c @@ -33,10 +33,6 @@ #define MCBA_USB_RX_BUFF_SIZE 64 #define MCBA_USB_TX_BUFF_SIZE (sizeof(struct mcba_usb_msg)) -/* MCBA endpoint numbers */ -#define MCBA_USB_EP_IN 1 -#define MCBA_USB_EP_OUT 1 - /* Microchip command id */ #define MBCA_CMD_RECEIVE_MESSAGE 0xE3 #define MBCA_CMD_I_AM_ALIVE_FROM_CAN 0xF5 @@ -83,6 +79,8 @@ struct mcba_priv { atomic_t free_ctx_cnt; void *rxbuf[MCBA_MAX_RX_URBS]; dma_addr_t rxbuf_dma[MCBA_MAX_RX_URBS]; + int rx_pipe; + int tx_pipe; }; /* CAN frame */ @@ -269,7 +267,7 @@ static netdev_tx_t mcba_usb_xmit(struct mcba_priv *priv, memcpy(buf, usb_msg, MCBA_USB_TX_BUFF_SIZE); usb_fill_bulk_urb(urb, priv->udev, - usb_sndbulkpipe(priv->udev, MCBA_USB_EP_OUT), buf, + priv->tx_pipe, buf, MCBA_USB_TX_BUFF_SIZE, mcba_usb_write_bulk_callback, ctx); @@ -608,7 +606,7 @@ static void mcba_usb_read_bulk_callback(struct urb *urb) resubmit_urb: usb_fill_bulk_urb(urb, priv->udev, - usb_rcvbulkpipe(priv->udev, MCBA_USB_EP_OUT), + priv->rx_pipe, urb->transfer_buffer, MCBA_USB_RX_BUFF_SIZE, mcba_usb_read_bulk_callback, priv); @@ -653,7 +651,7 @@ static int mcba_usb_start(struct mcba_priv *priv) urb->transfer_dma = buf_dma; usb_fill_bulk_urb(urb, priv->udev, - usb_rcvbulkpipe(priv->udev, MCBA_USB_EP_IN), + priv->rx_pipe, buf, MCBA_USB_RX_BUFF_SIZE, mcba_usb_read_bulk_callback, priv); urb->transfer_flags |= URB_NO_TRANSFER_DMA_MAP; @@ -807,6 +805,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, *out; + + err = usb_find_common_endpoints(intf->cur_altsetting, &in, &out, NULL, NULL); + if (err) { + dev_err(&intf->dev, "Can't find endpoints\n"); + return err; + } netdev = alloc_candev(sizeof(struct mcba_priv), MCBA_MAX_TX_URBS); if (!netdev) { @@ -852,6 +857,9 @@ static int mcba_usb_probe(struct usb_interface *intf, goto cleanup_free_candev; } + priv->rx_pipe = usb_rcvbulkpipe(priv->udev, in->bEndpointAddress); + priv->tx_pipe = usb_sndbulkpipe(priv->udev, out->bEndpointAddress); + devm_can_led_init(netdev); /* Start USB dev only if we have successfully registered CAN device */ -- 2.35.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v2] can: mcba_usb: properly check endpoint type 2022-03-08 8:16 ` [PATCH v2] " Pavel Skripkin @ 2022-03-08 8:54 ` Vincent MAILHOL 2022-03-13 10:09 ` [PATCH v3] " Pavel Skripkin 0 siblings, 1 reply; 9+ messages in thread From: Vincent MAILHOL @ 2022-03-08 8:54 UTC (permalink / raw) To: Pavel Skripkin Cc: yashi, wg, mkl, davem, kuba, linux-can, netdev, linux-kernel, syzbot+3bc1dce0cc0052d60fde On Tue. 8 Mar 2022 at 17:16, 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 > > Found pipes are now saved to struct mcba_priv and code uses them directly > instead of making pipes in place. > > 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> > --- > > Changes from RFT(RFC): > - Add missing out pipe check > - Use found pipes instead of making pipes in place > - Do not hide usb_find_common_endpoints() error > > --- > drivers/net/can/usb/mcba_usb.c | 22 +++++++++++++++------- > 1 file changed, 15 insertions(+), 7 deletions(-) > > diff --git a/drivers/net/can/usb/mcba_usb.c b/drivers/net/can/usb/mcba_usb.c > index 77bddff86252..91e79a2d5ae5 100644 > --- a/drivers/net/can/usb/mcba_usb.c > +++ b/drivers/net/can/usb/mcba_usb.c > @@ -33,10 +33,6 @@ > #define MCBA_USB_RX_BUFF_SIZE 64 > #define MCBA_USB_TX_BUFF_SIZE (sizeof(struct mcba_usb_msg)) > > -/* MCBA endpoint numbers */ > -#define MCBA_USB_EP_IN 1 > -#define MCBA_USB_EP_OUT 1 > - > /* Microchip command id */ > #define MBCA_CMD_RECEIVE_MESSAGE 0xE3 > #define MBCA_CMD_I_AM_ALIVE_FROM_CAN 0xF5 > @@ -83,6 +79,8 @@ struct mcba_priv { > atomic_t free_ctx_cnt; > void *rxbuf[MCBA_MAX_RX_URBS]; > dma_addr_t rxbuf_dma[MCBA_MAX_RX_URBS]; > + int rx_pipe; > + int tx_pipe; > }; > > /* CAN frame */ > @@ -269,7 +267,7 @@ static netdev_tx_t mcba_usb_xmit(struct mcba_priv *priv, > memcpy(buf, usb_msg, MCBA_USB_TX_BUFF_SIZE); > > usb_fill_bulk_urb(urb, priv->udev, > - usb_sndbulkpipe(priv->udev, MCBA_USB_EP_OUT), buf, > + priv->tx_pipe, buf, Nitpick: you might want to put more arguments per line (up to the 80 characters limit). > MCBA_USB_TX_BUFF_SIZE, mcba_usb_write_bulk_callback, > ctx); > > @@ -608,7 +606,7 @@ static void mcba_usb_read_bulk_callback(struct urb *urb) > resubmit_urb: > > usb_fill_bulk_urb(urb, priv->udev, > - usb_rcvbulkpipe(priv->udev, MCBA_USB_EP_OUT), > + priv->rx_pipe, > urb->transfer_buffer, MCBA_USB_RX_BUFF_SIZE, > mcba_usb_read_bulk_callback, priv); > > @@ -653,7 +651,7 @@ static int mcba_usb_start(struct mcba_priv *priv) > urb->transfer_dma = buf_dma; > > usb_fill_bulk_urb(urb, priv->udev, > - usb_rcvbulkpipe(priv->udev, MCBA_USB_EP_IN), > + priv->rx_pipe, > buf, MCBA_USB_RX_BUFF_SIZE, > mcba_usb_read_bulk_callback, priv); > urb->transfer_flags |= URB_NO_TRANSFER_DMA_MAP; > @@ -807,6 +805,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, *out; > + > + err = usb_find_common_endpoints(intf->cur_altsetting, &in, &out, NULL, NULL); > + if (err) { > + dev_err(&intf->dev, "Can't find endpoints\n"); > + return err; > + } > > netdev = alloc_candev(sizeof(struct mcba_priv), MCBA_MAX_TX_URBS); > if (!netdev) { > @@ -852,6 +857,9 @@ static int mcba_usb_probe(struct usb_interface *intf, > goto cleanup_free_candev; > } > > + priv->rx_pipe = usb_rcvbulkpipe(priv->udev, in->bEndpointAddress); > + priv->tx_pipe = usb_sndbulkpipe(priv->udev, out->bEndpointAddress); > + > devm_can_led_init(netdev); > > /* Start USB dev only if we have successfully registered CAN device */ Aside from the nitpick, it looks good to me. Reviewed-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr> Yours sincerely, Vincent Mailhol ^ permalink raw reply [flat|nested] 9+ messages in thread
* [PATCH v3] can: mcba_usb: properly check endpoint type 2022-03-08 8:54 ` Vincent MAILHOL @ 2022-03-13 10:09 ` Pavel Skripkin 2022-03-23 20:35 ` Pavel Skripkin 0 siblings, 1 reply; 9+ messages in thread From: Pavel Skripkin @ 2022-03-13 10:09 UTC (permalink / raw) To: yashi, wg, mkl, davem, kuba, mailhol.vincent Cc: linux-can, netdev, linux-kernel, Pavel Skripkin, syzbot+3bc1dce0cc0052d60fde 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 Found pipes are now saved to struct mcba_priv and code uses them directly instead of making pipes in place. 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> Reviewed-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr> --- Changes from v2: - Coding style error fixed Changes from RFT(RFC): - Add missing out pipe check - Use found pipes instead of making pipes in place - Do not hide usb_find_common_endpoints() error --- drivers/net/can/usb/mcba_usb.c | 26 ++++++++++++++++---------- 1 file changed, 16 insertions(+), 10 deletions(-) diff --git a/drivers/net/can/usb/mcba_usb.c b/drivers/net/can/usb/mcba_usb.c index 77bddff86252..56770a5a782a 100644 --- a/drivers/net/can/usb/mcba_usb.c +++ b/drivers/net/can/usb/mcba_usb.c @@ -33,10 +33,6 @@ #define MCBA_USB_RX_BUFF_SIZE 64 #define MCBA_USB_TX_BUFF_SIZE (sizeof(struct mcba_usb_msg)) -/* MCBA endpoint numbers */ -#define MCBA_USB_EP_IN 1 -#define MCBA_USB_EP_OUT 1 - /* Microchip command id */ #define MBCA_CMD_RECEIVE_MESSAGE 0xE3 #define MBCA_CMD_I_AM_ALIVE_FROM_CAN 0xF5 @@ -83,6 +79,8 @@ struct mcba_priv { atomic_t free_ctx_cnt; void *rxbuf[MCBA_MAX_RX_URBS]; dma_addr_t rxbuf_dma[MCBA_MAX_RX_URBS]; + int rx_pipe; + int tx_pipe; }; /* CAN frame */ @@ -268,10 +266,8 @@ static netdev_tx_t mcba_usb_xmit(struct mcba_priv *priv, memcpy(buf, usb_msg, MCBA_USB_TX_BUFF_SIZE); - usb_fill_bulk_urb(urb, priv->udev, - usb_sndbulkpipe(priv->udev, MCBA_USB_EP_OUT), buf, - MCBA_USB_TX_BUFF_SIZE, mcba_usb_write_bulk_callback, - ctx); + usb_fill_bulk_urb(urb, priv->udev, priv->tx_pipe, buf, MCBA_USB_TX_BUFF_SIZE, + mcba_usb_write_bulk_callback, ctx); urb->transfer_flags |= URB_NO_TRANSFER_DMA_MAP; usb_anchor_urb(urb, &priv->tx_submitted); @@ -608,7 +604,7 @@ static void mcba_usb_read_bulk_callback(struct urb *urb) resubmit_urb: usb_fill_bulk_urb(urb, priv->udev, - usb_rcvbulkpipe(priv->udev, MCBA_USB_EP_OUT), + priv->rx_pipe, urb->transfer_buffer, MCBA_USB_RX_BUFF_SIZE, mcba_usb_read_bulk_callback, priv); @@ -653,7 +649,7 @@ static int mcba_usb_start(struct mcba_priv *priv) urb->transfer_dma = buf_dma; usb_fill_bulk_urb(urb, priv->udev, - usb_rcvbulkpipe(priv->udev, MCBA_USB_EP_IN), + priv->rx_pipe, buf, MCBA_USB_RX_BUFF_SIZE, mcba_usb_read_bulk_callback, priv); urb->transfer_flags |= URB_NO_TRANSFER_DMA_MAP; @@ -807,6 +803,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, *out; + + err = usb_find_common_endpoints(intf->cur_altsetting, &in, &out, NULL, NULL); + if (err) { + dev_err(&intf->dev, "Can't find endpoints\n"); + return err; + } netdev = alloc_candev(sizeof(struct mcba_priv), MCBA_MAX_TX_URBS); if (!netdev) { @@ -852,6 +855,9 @@ static int mcba_usb_probe(struct usb_interface *intf, goto cleanup_free_candev; } + priv->rx_pipe = usb_rcvbulkpipe(priv->udev, in->bEndpointAddress); + priv->tx_pipe = usb_sndbulkpipe(priv->udev, out->bEndpointAddress); + devm_can_led_init(netdev); /* Start USB dev only if we have successfully registered CAN device */ -- 2.35.1 ^ permalink raw reply related [flat|nested] 9+ messages in thread
* Re: [PATCH v3] can: mcba_usb: properly check endpoint type 2022-03-13 10:09 ` [PATCH v3] " Pavel Skripkin @ 2022-03-23 20:35 ` Pavel Skripkin 0 siblings, 0 replies; 9+ messages in thread From: Pavel Skripkin @ 2022-03-23 20:35 UTC (permalink / raw) To: yashi, wg, mkl, davem, kuba, mailhol.vincent Cc: linux-can, netdev, linux-kernel, syzbot+3bc1dce0cc0052d60fde [-- Attachment #1.1: Type: text/plain, Size: 1471 bytes --] On 3/13/22 13:09, Pavel Skripkin 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 > > Found pipes are now saved to struct mcba_priv and code uses them directly > instead of making pipes in place. > > 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> > Reviewed-by: Vincent Mailhol <mailhol.vincent@wanadoo.fr> gentle ping. looks like this patch got somehow lost With regards, Pavel Skripkin [-- Attachment #2: OpenPGP digital signature --] [-- Type: application/pgp-signature, Size: 840 bytes --] ^ permalink raw reply [flat|nested] 9+ messages in thread
end of thread, other threads:[~2022-03-23 20:35 UTC | newest] Thread overview: 9+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-03-07 18:53 [PATCH RFT] can: mcba_usb: properly check endpoint type Pavel Skripkin 2022-03-08 0:23 ` Vincent MAILHOL 2022-03-08 8:06 ` Pavel Skripkin 2022-03-09 2:58 ` Yasushi SHOJI 2022-03-13 9:41 ` Pavel Skripkin 2022-03-08 8:16 ` [PATCH v2] " Pavel Skripkin 2022-03-08 8:54 ` Vincent MAILHOL 2022-03-13 10:09 ` [PATCH v3] " Pavel Skripkin 2022-03-23 20:35 ` Pavel Skripkin
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).