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] [thread-next>] [day] [month] [year] [list]

Message-ID: <54c7e42c-465b-42fc-9707-d848ae53a00c@rowland.harvard.edu>
Date: Mon, 19 Aug 2024 10:32:05 -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 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