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 :(
>
> /* 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
