On 6/4/24 06:35, Greg Kroah-Hartman wrote:
> On Tue, May 28, 2024 at 11:38:07AM -0700, Nikita Zhandarovich wrote:
>> Syzbot is still reporting quite an old issue [1] that occurs due to
>> incomplete checking of present usb endpoints. As such, wrong
>> endpoints types may be used at urb sumbitting stage which in turn
>> triggers a warning in usb_submit_urb().
>>
>> Fix the issue by verifying that required endpoint types are present
>> for both in and out endpoints, taking into account cmd endpoint type.
>>
>> Unfortunately, this patch has not been tested on real hardware.
>>
>> [1] Syzbot report:
>> usb 1-1: BOGUS urb xfer, pipe 1 != type 3
>> WARNING: CPU: 0 PID: 8667 at drivers/usb/core/urb.c:502 usb_submit_urb+0xed2/0x18a0 drivers/usb/core/urb.c:502
>> Modules linked in:
>> CPU: 0 PID: 8667 Comm: kworker/0:4 Not tainted 5.14.0-rc4-syzkaller #0
>> Hardware name: Google Google Compute Engine/Google Compute Engine, BIOS Google 01/01/2011
>> Workqueue: usb_hub_wq hub_event
>> RIP: 0010:usb_submit_urb+0xed2/0x18a0 drivers/usb/core/urb.c:502
>> ...
>> Call Trace:
>> cxacru_cm+0x3c0/0x8e0 drivers/usb/atm/cxacru.c:649
>> cxacru_card_status+0x22/0xd0 drivers/usb/atm/cxacru.c:760
>> cxacru_bind+0x7ac/0x11a0 drivers/usb/atm/cxacru.c:1209
>> usbatm_usb_probe+0x321/0x1ae0 drivers/usb/atm/usbatm.c:1055
>> cxacru_usb_probe+0xdf/0x1e0 drivers/usb/atm/cxacru.c:1363
>> usb_probe_interface+0x315/0x7f0 drivers/usb/core/driver.c:396
>> call_driver_probe drivers/base/dd.c:517 [inline]
>> really_probe+0x23c/0xcd0 drivers/base/dd.c:595
>> __driver_probe_device+0x338/0x4d0 drivers/base/dd.c:747
>> driver_probe_device+0x4c/0x1a0 drivers/base/dd.c:777
>> __device_attach_driver+0x20b/0x2f0 drivers/base/dd.c:894
>> bus_for_each_drv+0x15f/0x1e0 drivers/base/bus.c:427
>> __device_attach+0x228/0x4a0 drivers/base/dd.c:965
>> bus_probe_device+0x1e4/0x290 drivers/base/bus.c:487
>> device_add+0xc2f/0x2180 drivers/base/core.c:3354
>> usb_set_configuration+0x113a/0x1910 drivers/usb/core/message.c:2170
>> usb_generic_driver_probe+0xba/0x100 drivers/usb/core/generic.c:238
>> usb_probe_device+0xd9/0x2c0 drivers/usb/core/driver.c:293
>>
>> Reported-and-tested-by: syzbot+00c18ee8497dd3be6ade@xxxxxxxxxxxxxxxxxxxxxxxxx
>> Fixes: 902ffc3c707c ("USB: cxacru: Use a bulk/int URB to access the command endpoint")
>> Signed-off-by: Nikita Zhandarovich <n.zhandarovich@xxxxxxxxxx>
>> ---
>> P.S. While the driver is orphaned, it might still make sense to
>> suppress the syzbot report, seeing how ancient it is.
>> P.P.S. checkpatch complains about outdated format of debug printing
>> but I decided to keep it in tune with the rest of the driver.
>>
>> drivers/usb/atm/cxacru.c | 16 +++++++++++++++-
>> 1 file changed, 15 insertions(+), 1 deletion(-)
>>
>> diff --git a/drivers/usb/atm/cxacru.c b/drivers/usb/atm/cxacru.c
>> index 4ce7cba2b48a..8a8e94a601c6 100644
>> --- a/drivers/usb/atm/cxacru.c
>> +++ b/drivers/usb/atm/cxacru.c
>> @@ -1131,7 +1131,8 @@ static int cxacru_bind(struct usbatm_data *usbatm_instance,
>> struct cxacru_data *instance;
>> struct usb_device *usb_dev = interface_to_usbdev(intf);
>> struct usb_host_endpoint *cmd_ep = usb_dev->ep_in[CXACRU_EP_CMD];
>> - int ret;
>> + struct usb_endpoint_descriptor *in, *out;
>> + int ret = -1;
>
> Why initialize this and then write over it?
>
> Also, -1 is not a valid return value, so even if this was needed, it's
> not correct :(
>
>
I agree, that was a mistake on my part. An artifact from WIP-version of
the patch. I should have removed that initialization. Thank you for
bringing that up.
>>
>> /* instance init */
>> instance = kzalloc(sizeof(*instance), GFP_KERNEL);
>> @@ -1177,6 +1178,19 @@ static int cxacru_bind(struct usbatm_data *usbatm_instance,
>> goto fail;
>> }
>>
>> + if (usb_endpoint_xfer_int(&cmd_ep->desc))
>> + ret = usb_find_common_endpoints(intf->cur_altsetting,
>> + NULL, NULL, &in, &out);
>> + else
>> + ret = usb_find_common_endpoints(intf->cur_altsetting,
>> + &in, &out, NULL, NULL);
>> +
>> + if (ret) {
>> + usb_dbg(usbatm_instance, "cxacru_bind: interface has incorrect endpoints\n");
>
> Shouldn't this be an error instead?
I was torn between following the code style established in cxacru_bind()
(and some other functions) where in case of an error usb_dbg() is used
AND doing exactly what you suggested. I agree that using usb_err()
probably makes more sense here.
>
> thanks,
>
> greg k-h
I'll send revised patch soon.
Regards,
Nikita
