Re: [PATCH 11/11] Input: ims-pcu - fix potential infinite loop 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 driver frees `urb_in_buf` using `max_out_size` instead of the originally allocated size `max_in_size`, which can lead to memory corruption.
- [High] The driver fails to check if `max_ctrl_size` is zero before allocating DMA memory and submitting URBs.
--

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

Input: ims-pcu - fix potential infinite loop in CDC union descriptor parsing

This commit fixes a potential infinite loop in the IMS PCU driver when parsing
CDC union descriptors. It adds a check to ensure the descriptor length is at
least 2, preventing issues caused by a malicious USB device returning a
zero-length descriptor.

> diff --git a/drivers/input/misc/ims-pcu.c b/drivers/input/misc/ims-pcu.c
> index 7cdf40ac42ce1..4d0c7d3e8676c 100644
> --- a/drivers/input/misc/ims-pcu.c
> +++ b/drivers/input/misc/ims-pcu.c
> @@ -1678,8 +1678,9 @@ ims_pcu_get_cdc_union_desc(struct usb_interface *intf)
>  	while (buflen >= sizeof(*union_desc)) {
>  		union_desc = (struct usb_cdc_union_desc *)buf;
>  
> -		if (union_desc->bLength > buflen) {
> -			dev_err(&intf->dev, "Too large descriptor\n");
> +		if (union_desc->bLength < 2 || union_desc->bLength > buflen) {
> +			dev_err(&intf->dev, "Invalid descriptor length: %d\n",
> +				union_desc->bLength);
>  			return NULL;
>  		}

While reviewing this, I noticed a couple of pre-existing issues nearby that
might also be triggered by a malicious USB device.

In ims_pcu_parse_cdc_data():

>	pcu->ep_ctrl = &alt->endpoint[0].desc;
>	if (!usb_endpoint_is_int_in(pcu->ep_ctrl)) {
>		dev_err(pcu->dev,
>			"Control endpoint is not INTERRUPT IN\n");
>		return -EINVAL;
>	}
>
>	pcu->max_ctrl_size = usb_endpoint_maxp(pcu->ep_ctrl);

[Severity: High]
This is a pre-existing issue, but does this code need to verify that
pcu->max_ctrl_size is greater than zero?

A malicious USB device could intentionally set the control endpoint's
wMaxPacketSize to 0. Passing a size of 0 to usb_alloc_coherent() could invoke
undefined behavior in the DMA API. Additionally, submitting a zero-length
interrupt URB could cause undefined behavior in the host controller driver or
lead to an interrupt storm, since ims_pcu_irq() will repeatedly resubmit it.

In ims_pcu_buffers_free():

>static void ims_pcu_buffers_free(struct ims_pcu *pcu)
>{
>	usb_kill_urb(pcu->urb_in);
>	usb_free_urb(pcu->urb_in);
>
>	usb_free_coherent(pcu->udev, pcu->max_out_size,
>			  pcu->urb_in_buf, pcu->read_dma);

[Severity: High]
This isn't a bug introduced by this patch, but does this code free urb_in_buf
using the wrong size?

In ims_pcu_buffers_alloc(), urb_in_buf is allocated using pcu->max_in_size,
but here it is freed using pcu->max_out_size. If a malicious device provides
descriptors where the OUT endpoint's wMaxPacketSize is significantly larger
than the IN endpoint's size, could calling usb_free_coherent() with this larger
size erroneously free adjacent memory pages?

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