Re: issue with devm_ methods in disconnect and freeing memory in btusb

[Raphael Pinsonneault-Thibeault <rpthibeault@gmail.com>]

On Fri, Nov 14, 2025 at 09:03:45AM +0100, Oliver Neukum wrote:
> Hi,
> 
> looking at the change to btusb_disconnect() in 23d22f2f7176
> and the discussion leading to it I have doubts. Let me quote the change log:
> 
> -- quote --
> 
> Syzbot opens a usb device with out of order interface descriptors:
> Interface 3 (ISOC) in position 0, Interface 2 (DIAG) in position 1,
> Interface 1 (INTF) in position 2.
> So, ISOC is the first interface to get disconnected by usb_disconnect()
> -> usb_disable_device() -> ... -> btusb_disconnect().
> 
> I don't think this is a problem on hardware, where the bInterfaceNumber
> matches the position in the dev->actconfig->interface list; and in
> btusb_disconnect() it would only ever go into the first if
> statement: "if (intf == data->intf)" and not into any of the others.
> 
> -- quote --
> 
> Now, we cannot do this. The order disconnect() is called is arbitrary
> 
> 1. The order syzbot used is valid according to spec, albeit unusual
> 2. disconnect() can be triggered from user space via sysfs
> 
> We must always be ready to handle any arbitrary order.
> The code in the second branch of the if statement used to be perfectly correct.
> The actual breaking commit was 98921dbd00c4e by introducing devm_kzalloc()
> for memory allocation. That ties the lifetime of memory to the binding
> of a driver to an interface. In hindsight in a driver that binds
> to multiple interfaces, this is problematic. Hence I would propose
> to just revert 98921dbd00c4e. It seems to me that we have discovered
> a design limitation in the devm_ methods. What do you think?
> 
> 	Regards
> 		Oliver
> 

Hi Oliver,

I was under the impression that tying the memory lifetime to the INTF
interface was intentional since 98921dbd00c4e was from 2012, hence my
commit message in 23d22f2f7176:

-- quote --

There is a KASAN: slab-use-after-free read in btusb_disconnect().
Calling "usb_driver_release_interface(&btusb_driver, data->intf)" will
free the btusb data associated with the interface. The same data is
then used later in the function, hence the UAF.

Fix by moving the accesses to btusb data to before the data is free'd.

-- quote --

However, it seems that support for other interfaces was added
later, e.g. by 9d08f50401ac7, and out-of-band wakeup support even later
by fd913ef7ce619.

So, maybe it just wasn't considered?

The actual code changes in the patch itself shouldn't cause issues
unless calling device_init_wakeup() and gpiod_put() must actually be
done after releasing the interfaces. Which isn't immediately obvious
especially if tying the memory lifetime to INTF is intentional, where
calling them after leads to the UAF.

I've tested reverting 98921dbd00c4e locally, and it seems fine. I'll
send a new patch if the maintainers agree.

Raphael