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

[Nikita Zhandarovich]

Hi

On 5/3/24 08:58, Mauro Carvalho Chehab wrote:
> 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)?

I did not have the hardware required to test it properly, I'm afraid.
I made sure to point that out in the patch description.

As for siano devices possibly having different endpoints type apart from
bulk, I was relying on the rest of the driver code to determine just
that (which is maybe somewhat flawed in this case). Since
smsusb_submit_urb() (and some other functions like usb_rcvbulkpipe())
works exclusively with bulk types, I did not feel the need to even
expect int types, for instance.

> 
> Regards,
> Mauro
> 
>>  			dev->out_ep = desc->bEndpointAddress;
>>  		}
>>  	}
>>

With regards,
Nikita