| Message ID | 20240528183807.3832-1-n.zhandarovich@fintech.ru |
|---|---|
| State | New |
| Headers | show |
| Series | usb: atm: cxacru: fix endpoint checking in cxacru_bind() | expand |
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@syzkaller.appspotmail.com > Fixes: 902ffc3c707c ("USB: cxacru: Use a bulk/int URB to access the command endpoint") > Signed-off-by: Nikita Zhandarovich <n.zhandarovich@fintech.ru> > --- > 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: syzbot+00c18ee8497dd3be6ade@syzkaller.appspotmail.com >> Fixes: 902ffc3c707c ("USB: cxacru: Use a bulk/int URB to access the command endpoint") >> Signed-off-by: Nikita Zhandarovich <n.zhandarovich@fintech.ru> >> --- >> 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
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,
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@syzkaller.appspotmail.com Fixes: 902ffc3c707c ("USB: cxacru: Use a bulk/int URB to access the command endpoint") Signed-off-by: Nikita Zhandarovich <n.zhandarovich@fintech.ru> --- 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(-)