Re: [PATCH] media: usb: siano: fix endpoint checks in smsusb_init_device()

[Mauro Carvalho Chehab]

Em Tue, 9 Apr 2024 07:36:34 -0700
Nikita Zhandarovich <n.zhandarovich@xxxxxxxxxx> escreveu:

> Syzkaller reported a warning [1] in smsusb_submit_urb() which occurs
> if an attempt is made to send a bulk URB using the wrong endpoint
> type. The current approach to perform endpoint checking does not
> explicitly check if an endpoint in question has its type set to bulk.
> 
> Fix this issue by using functions usb_endpoint_is_bulk_XXX() to
> enable testing for correct ep types.
> 
> This patch has not been tested on real hardware.
> 
> [1] Syzkaller report:
> usb 1-1: string descriptor 0 read error: -71
> smsusb:smsusb_probe: board id=2, interface number 0
> smsusb:siano_media_device_register: media controller created
> ------------[ cut here ]------------
> usb 1-1: BOGUS urb xfer, pipe 3 != type 1
> WARNING: CPU: 0 PID: 3147 at drivers/usb/core/urb.c:494 usb_submit_urb+0xacd/0x1550 drivers/usb/core/urb.c:493
> ...
> Call Trace:
>  smsusb_start_streaming+0x16/0x1d0 drivers/media/usb/siano/smsusb.c:195
>  smsusb_init_device+0xd85/0x12d0 drivers/media/usb/siano/smsusb.c:475
>  smsusb_probe+0x496/0xa90 drivers/media/usb/siano/smsusb.c:566
>  usb_probe_interface+0x633/0xb40 drivers/usb/core/driver.c:396
>  really_probe+0x3cb/0x1020 drivers/base/dd.c:580
>  driver_probe_device+0x178/0x350 drivers/base/dd.c:763
> ...
>  hub_event+0x48d/0xd90 drivers/usb/core/hub.c:5644
>  process_one_work+0x833/0x10c0 kernel/workqueue.c:2276
>  worker_thread+0xac1/0x1300 kernel/workqueue.c:2422
>  kthread+0x39a/0x3c0 kernel/kthread.c:313
>  ret_from_fork+0x1f/0x30 arch/x86/entry/entry_64.S:294
> 
> Reported-and-tested-by: syzbot+12002a39b8c60510f8fb@xxxxxxxxxxxxxxxxxxxxxxxxx
> Fixes: 31e0456de5be ("media: usb: siano: Fix general protection fault in smsusb")
> Signed-off-by: Nikita Zhandarovich <n.zhandarovich@xxxxxxxxxx>
> ---
>  drivers/media/usb/siano/smsusb.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/usb/siano/smsusb.c b/drivers/media/usb/siano/smsusb.c
> index 723510520d09..daaac121c670 100644
> --- a/drivers/media/usb/siano/smsusb.c
> +++ b/drivers/media/usb/siano/smsusb.c
> @@ -405,10 +405,10 @@ static int smsusb_init_device(struct usb_interface *intf, int board_id)
>  		struct usb_endpoint_descriptor *desc =
>  				&intf->cur_altsetting->endpoint[i].desc;
>  
> -		if (desc->bEndpointAddress & USB_DIR_IN) {
> +		if (usb_endpoint_is_bulk_in(desc)) {
>  			dev->in_ep = desc->bEndpointAddress;
>  			align = usb_endpoint_maxp(desc) - sizeof(struct sms_msg_hdr);
> -		} else {
> +		} else if (usb_endpoint_is_bulk_out(desc)) {

Did you test it on what devices? I'm not sure if all siano devices
are bulk. Why did you decide to use usb_endpoint_is_bulk_(in|out)
instead of usb_endpoint_dir_(in|out)?

Regards,
Mauro

>  			dev->out_ep = desc->bEndpointAddress;
>  		}
>  	}
>