linux-kernel - Re: [PATCH] media/usb/siano: Fix endpoint type checking in smsusb

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <9ff214a6-10be-414b-bf86-3757dd819243@rowland.harvard.edu>
Date: Mon, 19 Aug 2024 22:31:03 -0400
From: Alan Stern <stern@...land.harvard.edu>
To: Mauro Carvalho Chehab <mchehab+huawei@...nel.org>
Cc: Greg KH <gregkh@...uxfoundation.org>,
	Mauro Carvalho Chehab <mchehab@...nel.org>,
	syzbot <syzbot+85e3ddbf0ddbfbc85f1e@...kaller.appspotmail.com>,
	linux-kernel@...r.kernel.org, linux-media@...r.kernel.org,
	linux-usb@...r.kernel.org, syzkaller-bugs@...glegroups.com
Subject: Re: [PATCH] media/usb/siano: Fix endpoint type checking in smsusb

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@...land.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