netdev - Re: [PATCH v5] Bluetooth: Fix possible race with userspace of sysfs isoc

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: <2025021807-ultimate-ascent-f5e0@gregkh>
Date: Tue, 18 Feb 2025 10:21:02 +0100
From: Greg KH <gregkh@...uxfoundation.org>
To: Hsin-chen Chuang <chharry@...gle.com>
Cc: linux-bluetooth@...r.kernel.org, luiz.dentz@...il.com,
	chromeos-bluetooth-upstreaming@...omium.org,
	Hsin-chen Chuang <chharry@...omium.org>,
	"David S. Miller" <davem@...emloft.net>,
	Eric Dumazet <edumazet@...gle.com>,
	Jakub Kicinski <kuba@...nel.org>,
	Johan Hedberg <johan.hedberg@...il.com>,
	Marcel Holtmann <marcel@...tmann.org>,
	Paolo Abeni <pabeni@...hat.com>, Simon Horman <horms@...nel.org>,
	Ying Hsu <yinghsu@...omium.org>, linux-kernel@...r.kernel.org,
	netdev@...r.kernel.org
Subject: Re: [PATCH v5] Bluetooth: Fix possible race with userspace of sysfs
 isoc_alt

On Tue, Feb 18, 2025 at 04:57:38PM +0800, Hsin-chen Chuang wrote:
> Hi Greg,
> 
> On Tue, Feb 18, 2025 at 4:23 PM Greg KH <gregkh@...uxfoundation.org> wrote:
> >
> > On Tue, Feb 18, 2025 at 12:24:07PM +0800, Hsin-chen Chuang wrote:
> > > Hi Greg,
> > >
> > > On Mon, Feb 17, 2025 at 4:53 PM Greg KH <gregkh@...uxfoundation.org> wrote:
> > > >
> > > > On Mon, Feb 17, 2025 at 04:44:35PM +0800, Hsin-chen Chuang wrote:
> > > > > On Fri, Feb 14, 2025 at 7:37 PM Greg KH <gregkh@...uxfoundation.org> wrote:
> > > > > >
> > > > > > On Fri, Feb 14, 2025 at 07:16:17PM +0800, Hsin-chen Chuang wrote:
> > > > > > > From: Hsin-chen Chuang <chharry@...omium.org>
> > > > > > >
> > > > > > > Expose the isoc_alt attr with device group to avoid the racing.
> > > > > > >
> > > > > > > Now we create a dev node for btusb. The isoc_alt attr belongs to it and
> > > > > > > it also becomes the parent device of hci dev.
> > > > > > >
> > > > > > > Fixes: b16b327edb4d ("Bluetooth: btusb: add sysfs attribute to control USB alt setting")
> > > > > >
> > > > > > Wait, step back, why is this commit needed if you can change the alt
> > > > > > setting already today through usbfs/libusb without needing to mess with
> > > > > > the bluetooth stack at all?
> > > > >
> > > > > In short: We want to configure the alternate settings without
> > > > > detaching the btusb driver, while detaching seems necessary for
> > > > > libusb_set_interface_alt_setting to work (Please correct me if I'm
> > > > > wrong!)
> > > >
> > > > I think changing the alternate setting should work using usbfs as you
> > > > would send that command to the device, not the interface, so the driver
> > > > bound to the existing interface would not need to be removed.
> > >
> > > I thought USBDEVFS_SETINTERFACE was the right command to begin with,
> > > but it seems not working in this case.
> > > The command itself attempts to claim the interface, but the interface
> > > is already claimed by btusb so it failed with Device or resource busy
> > >
> > > drivers/usb/core/devio.c:
> > >   USBDEVFS_SETINTERFACE -> proc_setintf -> checkintf -> claimintf
> >
> > Ah, ok, thanks for checking.  So as you control this device, why not
> > just disconnect it, change the setting, and then reconnect it?
> 
> After dis/reconnecting, a Bluetooth chipset would lose all its state:
> Existing connections/scanners/advertisers are all dropped.

If you are changing the alternate USB configuration, all state should be
dropped, right?  If not, huh how does the device know to keep that
state?

> This is as bad as (just an analogy) "Whenever you access a http web
> page, you need to bring your ethernet interface down and up, and after
> the page is downloaded, do that again".

Your ethernet interface does not contain state like this, we handle
chainging IP addresses and devices all the time, so perhaps wrong
analogy :)

> > Also, see my other review comment, how does BlueZ do this today?
> 
> BlueZ handles that in their MGMT command, that is, through Control
> channel -> BlueZ kernel space code -> driver callbacks.
> Once a Bluetooth chipset is opened with the User channel, it can't be
> used with the Control channel simultaneously, and vice versa.

So why not use that same control channel in your code?  Why are you
reinventing a new control channel for something that is obviously there
already?

So in short, what's preventing you from using the same exact driver
callbacks, OR the same exact kernel api.  Surely you all are not
replacing all of the in-kernel BlueZ code with an external kernel driver
just for this, right?  If so, that's not ok at all.

thanks,

greg k-h