| Message ID | 51b854da-f031-4a25-a19f-dac442d7bee2@rowland.harvard.edu |
|---|---|
| State | New |
| Headers | show |
| Series | media/usb/siano: Fix endpoint type checking in smsusb | expand |
Greg and Mauro: Was this patch ever applied? It doesn't appear in the current -rc kernel. Was there some confusion about which tree it should be merged through? Here's a link to the original submission: https://lore.kernel.org/all/51b854da-f031-4a25-a19f-dac442d7bee2@rowland.harvard.edu/ Alan Stern On Wed, Jul 31, 2024 at 01:29:54PM -0400, Alan Stern wrote: > The syzbot fuzzer reports that the smsusb driver doesn't check whether > the endpoints it uses are actually Bulk: > > smsusb:smsusb_probe: board id=15, interface number 6 > smsusb:siano_media_device_register: media controller created > ------------[ cut here ]------------ > usb 1-1: BOGUS urb xfer, pipe 3 != type 1 > WARNING: CPU: 0 PID: 42 at drivers/usb/core/urb.c:503 usb_submit_urb+0xe4b/0x1730 drivers/usb/core/urb.c:503 > ... > Call Trace: > <TASK> > smsusb_submit_urb+0x288/0x410 drivers/media/usb/siano/smsusb.c:173 > smsusb_start_streaming drivers/media/usb/siano/smsusb.c:197 [inline] > smsusb_init_device+0x856/0xe10 drivers/media/usb/siano/smsusb.c:477 > smsusb_probe+0x5e2/0x10b0 drivers/media/usb/siano/smsusb.c:575 > > The problem can be fixed by checking the endpoints' types along with > their directions. > > Signed-off-by: Alan Stern <stern@rowland.harvard.edu> > Reported-by: syzbot+85e3ddbf0ddbfbc85f1e@syzkaller.appspotmail.com > Tested-by: syzbot+85e3ddbf0ddbfbc85f1e@syzkaller.appspotmail.com > Closes: https://lore.kernel.org/linux-usb/000000000000e45551061e558c37@google.com/ > Fixes: 31e0456de5be ("media: usb: siano: Fix general protection fault in smsusb") > Cc: <stable@vger.kernel.org> > > --- > > drivers/media/usb/siano/smsusb.c | 4 ++-- > 1 file changed, 2 insertions(+), 2 deletions(-) > > Index: usb-devel/drivers/media/usb/siano/smsusb.c > =================================================================== > --- usb-devel.orig/drivers/media/usb/siano/smsusb.c > +++ usb-devel/drivers/media/usb/siano/smsusb.c > @@ -410,10 +410,10 @@ static int smsusb_init_device(struct usb > 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; > } > }
On Sun, Aug 18, 2024 at 02:20:44PM -0400, Alan Stern wrote: > Greg and Mauro: > > Was this patch ever applied? It doesn't appear in the current -rc > kernel. Was there some confusion about which tree it should be merged > through? > > Here's a link to the original submission: > > https://lore.kernel.org/all/51b854da-f031-4a25-a19f-dac442d7bee2@rowland.harvard.edu/ I never took it as it was touching a file that I'm not the maintainer of. But I will be glad to do so if Mauro doesn't want to take it through his tree, just let me know. thanks, greg k-h
Em Mon, 19 Aug 2024 05:11:47 +0200 Greg KH <gregkh@linuxfoundation.org> escreveu: > On Sun, Aug 18, 2024 at 02:20:44PM -0400, Alan Stern wrote: > > Greg and Mauro: > > > > Was this patch ever applied? It doesn't appear in the current -rc > > kernel. Was there some confusion about which tree it should be merged > > through? > > > > Here's a link to the original submission: > > > > https://lore.kernel.org/all/51b854da-f031-4a25-a19f-dac442d7bee2@rowland.harvard.edu/ > > I never took it as it was touching a file that I'm not the maintainer > of. But I will be glad to do so if Mauro doesn't want to take it > through his tree, just let me know. This patch is duplicated of this one: https://patchwork.linuxtv.org/project/linux-media/patch/20240409143634.33230-1-n.zhandarovich@fintech.ru/ The part I didn't like with such approach is that it checks only for bulk endpoints. Most media devices have also isoc. Now, I'm not sure about Siano devices. There are 3 different major chipsets supported by this driver (sm1000, sm11xx, sm2xxx). Among them, sm1000 has one USB ID for cold boot, and, once firmware is loaded, it gains another USB ID for a a warm boot. Your patch and the previously submitted one are not only checking for the direction, but it is also discarding isoc endpoints. Applying a change like that without testing with real hardware of those three types just to make fuzz testing happy, sounded a little bit risky to my taste. I would be more willing to pick it if the check would either be tested on real hardware or if the logic would be changed to accept either bulk or isoc endpoints, just like the current code. Thanks, Mauro
On Mon, Aug 19, 2024 at 10:15:11AM +0200, Mauro Carvalho Chehab wrote: > This patch is duplicated of this one: > > https://patchwork.linuxtv.org/project/linux-media/patch/20240409143634.33230-1-n.zhandarovich@fintech.ru/ > > The part I didn't like with such approach is that it checks only for > bulk endpoints. Most media devices have also isoc. Now, I'm not sure > about Siano devices. There are 3 different major chipsets supported > by this driver (sm1000, sm11xx, sm2xxx). Among them, sm1000 has one > USB ID for cold boot, and, once firmware is loaded, it gains another > USB ID for a a warm boot. Are you sure about all this? The one source file in drivers/media/usb/siano refers only to bulk endpoints, and the files in drivers/media/common/siano don't refer to endpoints or URBs at all. Nothing in either directory refers to isochronous endpoints. Is there some other place I should be looking? Also, while there are many constants in those files whose names start with SMS1, there aren't any whose names start with SMS2 or SM2 (or their lowercase equivalents). And the Kconfig help text mentions only Siano SMS1xxx. > Your patch and the previously submitted one are not only checking > for the direction, but it is also discarding isoc endpoints. > Applying a change like that without testing with real hardware of > those three types just to make fuzz testing happy, sounded a little > bit risky to my taste. > > I would be more willing to pick it if the check would either be > tested on real hardware or if the logic would be changed to > accept either bulk or isoc endpoints, just like the current code. If the driver did apply to devices that used isochronous transfers, the ideal approach would be to check the endpoint type against the device type. However, as it stands this doesn't seem to be necessary. Alan Stern
Em Mon, 19 Aug 2024 10:32:05 -0400 Alan Stern <stern@rowland.harvard.edu> escreveu: > On Mon, Aug 19, 2024 at 10:15:11AM +0200, Mauro Carvalho Chehab wrote: > > This patch is duplicated of this one: > > > > https://patchwork.linuxtv.org/project/linux-media/patch/20240409143634.33230-1-n.zhandarovich@fintech.ru/ > > > > The part I didn't like with such approach is that it checks only for > > bulk endpoints. Most media devices have also isoc. Now, I'm not sure > > about Siano devices. There are 3 different major chipsets supported > > by this driver (sm1000, sm11xx, sm2xxx). Among them, sm1000 has one > > USB ID for cold boot, and, once firmware is loaded, it gains another > > USB ID for a a warm boot. > > Are you sure about all this? The one source file in > drivers/media/usb/siano refers only to bulk endpoints, and the files in > drivers/media/common/siano don't refer to endpoints or URBs at all. > Nothing in either directory refers to isochronous endpoints. Is there > some other place I should be looking? > Also, while there are many constants in those files whose names start > with SMS1, there aren't any whose names start with SMS2 or SM2 (or their > lowercase equivalents). And the Kconfig help text mentions only Siano > SMS1xxx. > > > > Your patch and the previously submitted one are not only checking > > for the direction, but it is also discarding isoc endpoints. > > Applying a change like that without testing with real hardware of > > those three types just to make fuzz testing happy, sounded a little > > bit risky to my taste. > > > > I would be more willing to pick it if the check would either be > > tested on real hardware or if the logic would be changed to > > accept either bulk or isoc endpoints, just like the current code. > > If the driver did apply to devices that used isochronous transfers, the > ideal approach would be to check the endpoint type against the device > type. However, as it stands this doesn't seem to be necessary. The initial driver had support only for SM1000 and SM11xx. There is a small note there about the sm1000 devices there (I guess this is the chipset number of Stellar, but my memories might be tricking me), but without a real association with the chipset number: /* This device is only present before firmware load */ { USB_DEVICE(0x187f, 0x0010), .driver_info = SMS1XXX_BOARD_SIANO_STELLAR_ROM }, /* This device pops up after firmware load */ { USB_DEVICE(0x187f, 0x0100), .driver_info = SMS1XXX_BOARD_SIANO_STELLAR }, Years later, support for sm2xxx was added. Those two boards, for instance (see drivers/media/common/siano/sms-cards.c) are variants of sm2xxx (one of them is sm2270, if I'm not mistaken) that supports Brazilian TV standard: [SMS1XXX_BOARD_SIANO_PELE] = { .name = "Siano Pele Digital Receiver", .type = SMS_PELE, .default_mode = DEVICE_MODE_ISDBT_BDA, }, [SMS1XXX_BOARD_SIANO_RIO] = { .name = "Siano Rio Digital Receiver", .type = SMS_RIO, .default_mode = DEVICE_MODE_ISDBT_BDA, }, There are some boards there with a different version of sm22xx that supports only DVB (can't remember anymore what boards). Basically, the actual SMS device type is given by this enum: enum sms_device_type_st { SMS_UNKNOWN_TYPE = -1, SMS_STELLAR = 0, SMS_NOVA_A0, SMS_NOVA_B0, SMS_VEGA, SMS_VENICE, SMS_MING, SMS_PELE, SMS_RIO, SMS_DENVER_1530, SMS_DENVER_2160, SMS_NUM_OF_DEVICE_TYPES /* This is just a count */ }; But I dunno if there are a 1:1 mapping between type and chipset number. The above type names probably match some vendor internal names, but we never had any tables associating them to a device number, as the vendor never provided us such information. Btw I vaguely remember I heard about a newer Siano chipsets (sm3xxx), but never saw such devices. - Now, I'm not sure about what endpoints this specific driver exports, as I'm lacking vendor's documentation. What I said is that almost all DVB devices have isoc endpoints, but I dunno if this is the case of Siano. Thanks, Mauro
On Mon, Aug 19, 2024 at 06:24:56PM +0200, Mauro Carvalho Chehab wrote: > Basically, the actual SMS device type is given by this enum: > > enum sms_device_type_st { > SMS_UNKNOWN_TYPE = -1, > > SMS_STELLAR = 0, > SMS_NOVA_A0, > SMS_NOVA_B0, > SMS_VEGA, > SMS_VENICE, > SMS_MING, > SMS_PELE, > SMS_RIO, > SMS_DENVER_1530, > SMS_DENVER_2160, > > SMS_NUM_OF_DEVICE_TYPES /* This is just a count */ > }; > > But I dunno if there are a 1:1 mapping between type and chipset > number. The above type names probably match some vendor internal > names, but we never had any tables associating them to a device number, > as the vendor never provided us such information. > > Btw I vaguely remember I heard about a newer Siano chipsets (sm3xxx), > but never saw such devices. > > - > > Now, I'm not sure about what endpoints this specific driver exports, as > I'm lacking vendor's documentation. What I said is that almost all DVB > devices have isoc endpoints, but I dunno if this is the case of Siano. Currently the driver exports only bulk endpoints, even though it doesn't check the endpoint type. You can tell because the only routine in it that calls usb_submit_urb() is smsusb_submit_urb(), and that routine calls usb_fill_bulk_urb() before doing the submission. Given this, I suggest merging the earlier patch submission from Nikita Zhandarovich as-is. If the driver ever evolves to include support for isochronous endpoints, the probe function can be modified then. Alan Stern
Em Mon, 19 Aug 2024 13:14:19 -0400 Alan Stern <stern@rowland.harvard.edu> escreveu: > On Mon, Aug 19, 2024 at 06:24:56PM +0200, Mauro Carvalho Chehab wrote: > > Basically, the actual SMS device type is given by this enum: > > > > enum sms_device_type_st { > > SMS_UNKNOWN_TYPE = -1, > > > > SMS_STELLAR = 0, > > SMS_NOVA_A0, > > SMS_NOVA_B0, > > SMS_VEGA, > > SMS_VENICE, > > SMS_MING, > > SMS_PELE, > > SMS_RIO, > > SMS_DENVER_1530, > > SMS_DENVER_2160, > > > > SMS_NUM_OF_DEVICE_TYPES /* This is just a count */ > > }; > > > > But I dunno if there are a 1:1 mapping between type and chipset > > number. The above type names probably match some vendor internal > > names, but we never had any tables associating them to a device number, > > as the vendor never provided us such information. > > > > Btw I vaguely remember I heard about a newer Siano chipsets (sm3xxx), > > but never saw such devices. > > > > - > > > > Now, I'm not sure about what endpoints this specific driver exports, as > > I'm lacking vendor's documentation. What I said is that almost all DVB > > devices have isoc endpoints, but I dunno if this is the case of Siano. > > Currently the driver exports only bulk endpoints, even though it doesn't > check the endpoint type. You can tell because the only routine in it > that calls usb_submit_urb() is smsusb_submit_urb(), and that routine > calls usb_fill_bulk_urb() before doing the submission. > > Given this, I suggest merging the earlier patch submission from Nikita > Zhandarovich as-is. If the driver ever evolves to include support for > isochronous endpoints, the probe function can be modified then. I'll see if I can try his patch and see if device keeps working. The logic indeed use endpoints in bulk mode, but I'm not sure if, for all the BIOS files at drivers/media/common/siano/smscoreapi.[ch], the endpoints are properly reported as bulk. What happens if an endpoint is reported as ISOC, but the URB submit is called without URB_ISO_ASAP? On a quick check, the code at usb_submit_urb() seems to not complain about that. I would be a lot more comfortable if the patch were using just if (usb_endpoint_dir_in(desc)) ... if (usb_endpoint_dir_out(desc)) ... or something like this (to accept both isoc and bulk): if (!usb_endpoint_xfer_int(epd)) { if (usb_endpoint_dir_in(desc)) ... if (usb_endpoint_dir_out(desc)) ... } instead of calling usb_endpoint_xfer_bulk(desc) to check if type is bulk. I'll try to do some tests, but not sure when, as I'm traveling abroad this week. Thanks, Mauro
On Tue, Aug 20, 2024 at 01:10:33AM +0200, Mauro Carvalho Chehab wrote: > Em Mon, 19 Aug 2024 13:14:19 -0400 > Alan Stern <stern@rowland.harvard.edu> escreveu: > > Currently the driver exports only bulk endpoints, even though it doesn't > > check the endpoint type. You can tell because the only routine in it > > that calls usb_submit_urb() is smsusb_submit_urb(), and that routine > > calls usb_fill_bulk_urb() before doing the submission. > > > > Given this, I suggest merging the earlier patch submission from Nikita > > Zhandarovich as-is. If the driver ever evolves to include support for > > isochronous endpoints, the probe function can be modified then. > > I'll see if I can try his patch and see if device keeps working. The > logic indeed use endpoints in bulk mode, but I'm not sure if, for all the > BIOS files at drivers/media/common/siano/smscoreapi.[ch], the endpoints > are properly reported as bulk. > > What happens if an endpoint is reported as ISOC, but the URB submit > is called without URB_ISO_ASAP? On a quick check, the code at usb_submit_urb() > seems to not complain about that. It _does_ complain if a driver submits a bulk URB to an isochronous endpoint. See the usb_pipe_type_check() function and the dev_WARN() on line 503 of drivers/usb/core/urb.c. (In any case, the URB_ISO_ASAP flag is optional, so of course there is no complaint if the flag is missing.) Furthermore, if an endpoint really is isochronous but the driver uses usb_fill_bulk_urb(), the transfer won't work at all. URBs for those two endpoint types use completely different ways of specifying their data buffers and transfer lengths. See the paragraph starting with "Isochronous URBs have a different data transfer model" in the kerneldoc for the definition of struct urb in include/linux/usb.h. > I would be a lot more comfortable if the patch were using just > > if (usb_endpoint_dir_in(desc)) > ... > if (usb_endpoint_dir_out(desc)) > ... > > or something like this (to accept both isoc and bulk): > > if (!usb_endpoint_xfer_int(epd)) { > if (usb_endpoint_dir_in(desc)) > ... > if (usb_endpoint_dir_out(desc)) > ... > } > > > instead of calling usb_endpoint_xfer_bulk(desc) to check if type > is bulk. > > I'll try to do some tests, but not sure when, as I'm traveling abroad > this week. Instead of going to the trouble of running a test, you could simply run "lsusb -v" and check whether or not all the endpoints are bulk. Alan Stern
Index: usb-devel/drivers/media/usb/siano/smsusb.c =================================================================== --- usb-devel.orig/drivers/media/usb/siano/smsusb.c +++ usb-devel/drivers/media/usb/siano/smsusb.c @@ -410,10 +410,10 @@ static int smsusb_init_device(struct usb 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; } }