| Message ID | 20240409143634.33230-1-n.zhandarovich@fintech.ru |
|---|---|
| State | New |
| Headers | show |
| Series | media: usb: siano: fix endpoint checks in smsusb_init_device() | expand |
Em Tue, 9 Apr 2024 07:36:34 -0700 Nikita Zhandarovich <n.zhandarovich@fintech.ru> 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@syzkaller.appspotmail.com > Fixes: 31e0456de5be ("media: usb: siano: Fix general protection fault in smsusb") > Signed-off-by: Nikita Zhandarovich <n.zhandarovich@fintech.ru> > --- > 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; > } > } >
Hi On 5/3/24 08:58, Mauro Carvalho Chehab wrote: > Em Tue, 9 Apr 2024 07:36:34 -0700 > Nikita Zhandarovich <n.zhandarovich@fintech.ru> 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@syzkaller.appspotmail.com >> Fixes: 31e0456de5be ("media: usb: siano: Fix general protection fault in smsusb") >> Signed-off-by: Nikita Zhandarovich <n.zhandarovich@fintech.ru> >> --- >> 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
Em Fri, 3 May 2024 09:14:37 -0700 Nikita Zhandarovich <n.zhandarovich@fintech.ru> 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@fintech.ru> 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@syzkaller.appspotmail.com > >> Fixes: 31e0456de5be ("media: usb: siano: Fix general protection fault in smsusb") > >> Signed-off-by: Nikita Zhandarovich <n.zhandarovich@fintech.ru> > >> --- > >> 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
Hi On 5/3/24 14:20, Mauro Carvalho Chehab wrote: > Em Fri, 3 May 2024 09:14:37 -0700 > Nikita Zhandarovich <n.zhandarovich@fintech.ru> escreveu: > >>> 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. After a quick glance at Siano driver I am not sure about that being true specifically for Siano devices just yet, but I understand your point nonetheless. > >> 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. Thank you for being so thorough with your answer. I think I overestimated how much I should trust current driver implementation. I should be (at least, in this case) looking at what hardware expects of the driver. Let me try to do a little more research into smsxxxx specs and maybe see about getting my hands on some actual devices for testing. > > Regards, > Mauro With regards, Nikita
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@fintech.ru> 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@fintech.ru> 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@syzkaller.appspotmail.com >>>> Fixes: 31e0456de5be ("media: usb: siano: Fix general protection fault in smsusb") >>>> Signed-off-by: Nikita Zhandarovich <n.zhandarovich@fintech.ru> >>>> --- >>>> 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
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)) { dev->out_ep = desc->bEndpointAddress; } }
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@syzkaller.appspotmail.com Fixes: 31e0456de5be ("media: usb: siano: Fix general protection fault in smsusb") Signed-off-by: Nikita Zhandarovich <n.zhandarovich@fintech.ru> --- drivers/media/usb/siano/smsusb.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-)