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: [email protected]
Fixes: 902ffc3c707c ("USB: cxacru: Use a bulk/int URB to access the command endpoint")
Signed-off-by: Nikita Zhandarovich <[email protected]>
---
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;
/* 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");
+ ret = -ENODEV;
+ goto fail;
+ }
+
if ((cmd_ep->desc.bmAttributes & USB_ENDPOINT_XFERTYPE_MASK)
== USB_ENDPOINT_XFER_INT) {
usb_fill_int_urb(instance->rcv_urb,
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: [email protected]
> Fixes: 902ffc3c707c ("USB: cxacru: Use a bulk/int URB to access the command endpoint")
> Signed-off-by: Nikita Zhandarovich <[email protected]>
> ---
> 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 :(
>
> /* 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?
thanks,
greg k-h
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: [email protected]
>> Fixes: 902ffc3c707c ("USB: cxacru: Use a bulk/int URB to access the command endpoint")
>> Signed-off-by: Nikita Zhandarovich <[email protected]>
>> ---
>> 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