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

[Nikita Zhandarovich]

Hi Mauro,

On 5/3/24 14:20, Mauro Carvalho Chehab wrote:
> Em Fri, 3 May 2024 09:14:37 -0700
> Nikita Zhandarovich <n.zhandarovich@xxxxxxxxxx> escreveu:
> 
>> 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). 
> 
> Most digital TV devices also have ISOC endpoints. I'm not sure about
> Siano.
> 
>> 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.
> 
> The problem is not with int endpoints. Most media devices have isoc
> endpoints. There are even cases that different device models supported
> by the same driver have some bulk and some isoc endpoints. See em28xx 
> driver for instance: most devices are isoc only; some have both isoc
> and bulk; a few have just bulk.
> 
> In the specific case of siano, it supports 3 generations of devices
> (sms1000, sms11xx, sms22xx), each with several submodels. 
> Those 3 generations have different specs, and sms1000 even have one
> "boot" USB ID, and one "warm" USB ID, each one with different endpoints.
> 
> That's basically why I'm afraid of a patch like this could cause
> regressions if not properly tested, as it is changing the logic
> from just detecting the direction to also validate if the endpoint
> is not isoc.
> 
> Regards,
> Mauro

After doing a tiny bit of research I've had a couple more questions as
regards to dealing with ISOC endpoints in siano devices. I am still not
exactly versed in the subject, so forgive my ignorance.

Firstly, judging from the current state of siano driver I think that
sms1xxx devices do not actually deal with ISOC or some other "special"
endpoints apart from bulk ones. em28xx and at least a couple of other
drivers that may expect ISOC eps, check for their presence and prepare
URBs differently, checking for pipe types and filling URBs with
usb_rcvisocpipe() and usb_fill_int_urb() accordingly.

Since siano essentially uses only bulk functions, it either treats isoc
and bulk (and maybe other types) eps as bulk only - which seems wrong to
me and implies problems with devices as a result of this; or siano
driver doesn't actually require isoc-specific actions (or for other
types). I want to point out that I myself tend to agree with latter
option, once again judging by the way driver currently works with eps.

Which brings me to another question - if you could share an example of a
spec or datasheet for one of the smsxxxx devices that explicitly
confirms presence of other ep types, that would be much appreciated. For
some reason, I failed to find something meaningful on my own.

Also, I could use some direction on how to properly test my change on
real hardware. I am looking towards something along the lines of
https://linuxtv.org/wiki/index.php/Smart_Plus as a test subject. If
there are some examples/approaches you think are appropriate for this,
I'll gladly adopt them.

Thank you for your patience :)

Regards,
Nikita