issue with devm_ methods in disconnect and freeing memory in btusb

issue with devm_ methods in disconnect and freeing memory in btusb

[Oliver Neukum]

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

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

[Raphael Pinsonneault-Thibeault]

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

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

[Oliver Neukum]

On 14.11.25 19:11, Raphael Pinsonneault-Thibeault wrote:
> On Fri, Nov 14, 2025 at 09:03:45AM +0100, Oliver Neukum wrote:


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

No, it was not. And it would be kind of defeating the purpose
of a devm-method. You use them to not have to free memory manually.
In the disconnect() method of btusb we do free memory manually,
only that we do so implicitly. And that is dangerous.

> 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

Was that obvious to you?

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

ISO support is very old. It is necessary because you cannot
change the altsetting of an interface that is in use. It is
a design flaw of USB.
> So, maybe it just wasn't considered?
Indeed and hence I would just revert it.
This is an accident waiting to happen.

	Regards
		Oliver