Re: [PATCH RFT] can: mcba_usb: properly check endpoint type

[Vincent MAILHOL]

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@xxxxxxxxx> 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@xxxxxxxxxxxxxxxxxxxxxxxxx
> Fixes: 51f3baad7de9 ("can: mcba_usb: Add support for Microchip CAN BUS Analyzer")
> Signed-off-by: Pavel Skripkin <paskripkin@xxxxxxxxx>
> ---
>
> 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