diff mbox series

[v2] can: mcba_usb: properly check endpoint type

Message ID 20220308081608.3243-1-paskripkin@gmail.com (mailing list archive)
State Superseded
Delegated to: Netdev Maintainers
Headers show
Series [v2] can: mcba_usb: properly check endpoint type | expand

Checks

Context Check Description
netdev/tree_selection success Series ignored based on subject

Commit Message

Pavel Skripkin March 8, 2022, 8:16 a.m. UTC 
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(-)

Comments

Vincent Mailhol March 8, 2022, 8:54 a.m. UTC | #1 
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
diff mbox series

Patch

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 */