Re: [PATCH 03/11] Input: ims-pcu - fix type confusion in CDC union descriptor parsing

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The size parameter passed to `usb_free_coherent()` when freeing `urb_in_buf` is incorrect.
- [High] The driver does not properly clean up or stop I/O when the secondary (data) interface is unbound.
--

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

Input: ims-pcu - fix type confusion in CDC union descriptor parsing

The driver currently trusts the bMasterInterface0 from the CDC union
descriptor without verifying that it matches the interface being probed.
Validate that the control interface found in the descriptor is indeed the
one we are probing to avoid overwriting private data.

> diff --git a/drivers/input/misc/ims-pcu.c b/drivers/input/misc/ims-pcu.c
> index 2ad72fe4a51d9..5d1df07ac2a2a 100644
> --- a/drivers/input/misc/ims-pcu.c
> +++ b/drivers/input/misc/ims-pcu.c
> @@ -1693,7 +1693,7 @@ static int ims_pcu_parse_cdc_data(struct usb_interface *intf, struct ims_pcu *pc
>  
>  	pcu->ctrl_intf = usb_ifnum_to_if(pcu->udev,
>  					 union_desc->bMasterInterface0);
> -	if (!pcu->ctrl_intf)
> +	if (pcu->ctrl_intf != intf)
>  		return -EINVAL;
>  
>  	alt = pcu->ctrl_intf->cur_altsetting;

[Severity: High]
This isn't a bug introduced by this patch, but pre-exists in the driver:
the size parameter passed to usb_free_coherent() when freeing urb_in_buf
appears incorrect.

Does this mismatch result in partial memory unmapping or page leaks if the
max packet sizes for IN and OUT endpoints differ?

In ims_pcu_buffers_alloc(), pcu->urb_in_buf is allocated using
pcu->max_in_size:

drivers/input/misc/ims-pcu.c:ims_pcu_buffers_alloc() {
	...
	pcu->urb_in_buf = usb_alloc_coherent(pcu->udev, pcu->max_in_size,
					     GFP_KERNEL, &pcu->read_dma);
	...
}

However, in ims_pcu_buffers_free(), it is freed using pcu->max_out_size:

drivers/input/misc/ims-pcu.c:ims_pcu_buffers_free() {
	...
	usb_free_coherent(pcu->udev, pcu->max_out_size,
			  pcu->urb_in_buf, pcu->read_dma);
	...
}

Because USB devices can have different max packet sizes for IN and OUT
endpoints, max_in_size and max_out_size are not guaranteed to be equal.
The DMA API requires that the exact size passed to the allocator is
provided to the free function.

[Severity: High]
This is a pre-existing issue, but the driver does not appear to properly
clean up or stop I/O when the secondary (data) interface is unbound.

If a user manually unbinds the data interface via sysfs, will the driver
continue submitting URBs to the unbound interface's endpoints?

Looking at ims_pcu_disconnect(), if the unbound interface is not the
control interface, it returns early without stopping URBs:

drivers/input/misc/ims-pcu.c:ims_pcu_disconnect() {
	...
	/*
	 * See if we are dealing with control or data interface. The cleanup
	 * happens when we unbind primary (control) interface.
	 */
	if (intf != pcu->ctrl_intf)
		return;

	ims_pcu_stop_io(pcu);
	...
}

This means the control interface remains bound and the driver will continue
to submit URBs (such as urb_out in ims_pcu_send_cmd_chunk()) to the data
interface's endpoints.

This unstopped I/O can lead to state corruption if the data interface is
subsequently claimed.

Note that a later patch 'Input: ims-pcu - fix use-after-free and double-free
in disconnect' alters the early return condition to compare pointers instead
of class types, but it maintains the fundamentally flawed behavior of
skipping cleanup when the data interface is unbound.

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