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: [email protected]
Fixes: 51f3baad7de9 ("can: mcba_usb: Add support for Microchip CAN BUS Analyzer")
Signed-off-by: Pavel Skripkin <[email protected]>
---
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
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 <[email protected]> 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 <[email protected]> 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
Hi Yasushi,
On 3/9/22 05:58, Yasushi SHOJI wrote:
> Hi Pavel,
>
> On Tue, Mar 8, 2022 at 5:06 PM Pavel Skripkin <[email protected]> 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