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

[Mauro Carvalho Chehab]

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