* [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
* [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
* 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 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).