Re: [PATCH 02/11] Input: ims-pcu - fix use-after-free and double-free in disconnect

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Concurrent unbinding of both the control and data interfaces via sysfs triggers a Use-After-Free on the `pcu` structure.

Pre-existing issues:
- [Critical] The driver blindly trusts the CDC Union Descriptor to identify interfaces, leading to memory corruption during probe and an unconditional NULL pointer dereference in disconnect.
- [High] In `ims_pcu_buffers_free`, the input DMA buffer is freed using the maximum output buffer size instead of its actual allocated size.
--

commit 73572626cc6ab15fc9107238c7a713e379862dd8
Author: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Input: ims-pcu - fix use-after-free and double-free in disconnect

This commit changes the interface verification in ims_pcu_disconnect() to
directly compare the disconnecting interface against the saved control
interface pointer. This prevents premature cleanup caused by malicious
devices presenting multiple interfaces with the same class.

> diff --git a/drivers/input/misc/ims-pcu.c b/drivers/input/misc/ims-pcu.c
> index 89737ef381c1c..2ad72fe4a51d9 100644
> --- a/drivers/input/misc/ims-pcu.c
> +++ b/drivers/input/misc/ims-pcu.c
> @@ -2071,7 +2071,6 @@ static int ims_pcu_probe(struct usb_interface *intf,
>  static void ims_pcu_disconnect(struct usb_interface *intf)
>  {
>  	struct ims_pcu *pcu = usb_get_intfdata(intf);
> -	struct usb_host_interface *alt = intf->cur_altsetting;
>  
>  	usb_set_intfdata(intf, NULL);
>  
> @@ -2079,7 +2078,7 @@ static void ims_pcu_disconnect(struct usb_interface *intf)
>  	 * See if we are dealing with control or data interface. The cleanup
>  	 * happens when we unbind primary (control) interface.
>  	 */
> -	if (alt->desc.bInterfaceClass != USB_CLASS_COMM)
> +	if (intf != pcu->ctrl_intf)

[Severity: High]
Can this introduce a use-after-free if both control and data interfaces are
unbound concurrently via sysfs?

If Process A unbinds the data interface, it gets the pcu pointer from
usb_get_intfdata() and might be preempted. The USB core sets the data
interface condition to USB_INTERFACE_UNBINDING.

Process B then unbinds the control interface, enters ims_pcu_disconnect(),
and calls usb_driver_release_interface() on the data interface.

Since the data interface is unbinding, usb_driver_release_interface() returns
immediately without waiting, and Process B proceeds to kfree(pcu).

When Process A resumes, evaluating intf != pcu->ctrl_intf would dereference
the freed pcu pointer.

[Severity: Critical]
This is a pre-existing issue, but does this new check introduce an
unconditional NULL pointer dereference if the CDC Union Descriptor is spoofed?

During ims_pcu_probe(), the driver uses bMasterInterface0 from the CDC Union
Descriptor as pcu->ctrl_intf without verifying it matches the probed
interface.

If a malicious device supplies a descriptor pointing to an interface owned by
another driver, the actual probed interface's intfdata remains NULL.

In that scenario, usb_get_intfdata() returns NULL for pcu, and evaluating
intf != pcu->ctrl_intf here will panic. Note that this is fixed later in the
patch series by commit af29ec151c71a.

>  		return;
>  
>  	ims_pcu_stop_io(pcu);

[Severity: High]
This is a pre-existing issue, but could ims_pcu_buffers_free() be freeing
the input buffer with the wrong size?

Looking at ims_pcu_buffers_alloc(), pcu->urb_in_buf is allocated using
pcu->max_in_size. However, ims_pcu_buffers_free() appears to free it using
pcu->max_out_size:

usb_free_coherent(pcu->udev, pcu->max_out_size, pcu->urb_in_buf,
                  pcu->read_dma);

If a device specifies different maximum packet sizes for IN and OUT endpoints,
calling usb_free_coherent() with the wrong size could corrupt the DMA
allocator's buddy system.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260523050634.501509-1-dmitry.torokhov@gmail.com?part=2