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: [email protected]
Fixes: 31e0456de5be ("media: usb: siano: Fix general protection fault in smsusb")
Signed-off-by: Nikita Zhandarovich <[email protected]>
---
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)) {
dev->out_ep = desc->bEndpointAddress;
}
}
Em Tue, 9 Apr 2024 07:36:34 -0700
Nikita Zhandarovich <[email protected]> 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: [email protected]
> Fixes: 31e0456de5be ("media: usb: siano: Fix general protection fault in smsusb")
> Signed-off-by: Nikita Zhandarovich <[email protected]>
> ---
> 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 <[email protected]> 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: [email protected]
>> Fixes: 31e0456de5be ("media: usb: siano: Fix general protection fault in smsusb")
>> Signed-off-by: Nikita Zhandarovich <[email protected]>
>> ---
>> 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 <[email protected]> escreveu:
> Hi
>
> On 5/3/24 08:58, Mauro Carvalho Chehab wrote:
> > Em Tue, 9 Apr 2024 07:36:34 -0700
> > Nikita Zhandarovich <[email protected]> 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: [email protected]
> >> Fixes: 31e0456de5be ("media: usb: siano: Fix general protection fault in smsusb")
> >> Signed-off-by: Nikita Zhandarovich <[email protected]>
> >> ---
> >> 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 <[email protected]> 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 <[email protected]> escreveu:
>
>> Hi
>>
>> On 5/3/24 08:58, Mauro Carvalho Chehab wrote:
>>> Em Tue, 9 Apr 2024 07:36:34 -0700
>>> Nikita Zhandarovich <[email protected]> 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: [email protected]
>>>> Fixes: 31e0456de5be ("media: usb: siano: Fix general protection fault in smsusb")
>>>> Signed-off-by: Nikita Zhandarovich <[email protected]>
>>>> ---
>>>> 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