diff mbox series

media/usb/siano: Fix endpoint type checking in smsusb

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

Commit Message

Alan Stern July 31, 2024, 5:29 p.m. UTC
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(-)

Comments

Alan Stern Aug. 18, 2024, 6:20 p.m. UTC | #1
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;
>  		}
>  	}
Greg Kroah-Hartman Aug. 19, 2024, 3:11 a.m. UTC | #2
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
Mauro Carvalho Chehab Aug. 19, 2024, 8:15 a.m. UTC | #3
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
Alan Stern Aug. 19, 2024, 2:32 p.m. UTC | #4
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
Mauro Carvalho Chehab Aug. 19, 2024, 4:24 p.m. UTC | #5
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
Alan Stern Aug. 19, 2024, 5:14 p.m. UTC | #6
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
Mauro Carvalho Chehab Aug. 19, 2024, 11:10 p.m. UTC | #7
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
Alan Stern Aug. 20, 2024, 2:31 a.m. UTC | #8
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
diff mbox series

Patch

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;
 		}
 	}