Re: [PATCH] Input: ims-pcu - fix use-after-free in probe error path

[sashiko-bot@kernel.org]

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

New issues:
- [Low] The patch introduces a useless `goto` statement immediately preceding its target label.

Pre-existing issues:
- [High] A malicious USB device can trigger a Double Free and Use-After-Free in the probe error path by spoofing the data interface class.
- [High] The driver blindly trusts the CDC Union descriptor's `bMasterInterface0` field, allowing a malicious device to corrupt other drivers' drvdata.
- [High] An infinite loop can occur in `ims_pcu_get_cdc_union_desc` if a USB descriptor has a `bLength` of 0.
- [High] The driver frees the `urb_in_buf` DMA coherent buffer using an incorrect size parameter.
- [High] The driver assumes the first endpoint of the control interface is an Interrupt IN endpoint without verifying its type or direction.
--

commit a298fe95c1b14555659ce23349aaf55e3bb035b5
Author: Zhian Liang <liangzhan5dev@gmail.com>

Input: ims-pcu - fix use-after-free in probe error path

This commit addresses a use-after-free in the probe error path by clearing
the control interface data to NULL before freeing the pcu structure. This
prevents the disconnect handler from accessing the freed pointer if the
device is removed.

> diff --git a/drivers/input/misc/ims-pcu.c b/drivers/input/misc/ims-pcu.c
> index 4c022a36dbe84..fce3232ebf072 100644
> --- a/drivers/input/misc/ims-pcu.c
> +++ b/drivers/input/misc/ims-pcu.c
> @@ -2063,6 +2063,10 @@ static int ims_pcu_probe(struct usb_interface *intf,
>  	ims_pcu_buffers_free(pcu);

[Severity: High]
This is a pre-existing issue, but does this function properly free the
urb_in_buf?

Looking at ims_pcu_buffers_free(), it seems to use max_out_size instead of
max_in_size:

ims_pcu_buffers_free() {
        ...
        usb_free_coherent(pcu->udev, pcu->max_out_size,
                          pcu->urb_in_buf, pcu->read_dma);
        ...
}

Could a device with mismatched IN and OUT endpoint descriptors cause an
improper free size and corrupt the DMA allocator?

>  err_unclaim_intf:
>  	usb_driver_release_interface(&ims_pcu_driver, pcu->data_intf);

[Severity: High]
This is a pre-existing issue, but can a device spoofing the data interface
class trigger a double free here?

If a device presents a data interface with bInterfaceClass == USB_CLASS_COMM,
ims_pcu_disconnect() might fail to skip it. When
usb_driver_release_interface() is called on the data interface during a
probe failure, it synchronously unbinds the interface, triggering
ims_pcu_disconnect(). 

Could ims_pcu_disconnect() then incorrectly assume it is handling the
control interface and free the pcu structure, leading to a use-after-free
read on pcu->ctrl_intf and a double free on pcu in this error path?

> +	goto err_clear_intfdata;

[Severity: Low]
Is this goto statement necessary? Execution would naturally fall through to
the err_clear_intfdata label on the very next line, making this instruction
redundant.

> +err_clear_intfdata:
> +	if (pcu->ctrl_intf)
> +		usb_set_intfdata(pcu->ctrl_intf, NULL);

[Severity: High]
This is a pre-existing issue, but does the driver safely assign
pcu->ctrl_intf?

In ims_pcu_parse_cdc_data(), pcu->ctrl_intf is assigned using the
bMasterInterface0 field from the CDC Union descriptor. If a device sets
bMasterInterface0 to point to a different, already-bound interface, could
the probe function unconditionally overwrite that victim interface's drvdata?

If the probe subsequently fails, this error path would clear the victim's
drvdata to NULL, which might cause the victim driver to crash when
disconnected.

>  err_free_mem:
>  	kfree(pcu);
>  	return error;

[Severity: High]
This is a pre-existing issue, but could there be an infinite loop during
descriptor parsing? 

If a device provides a CDC union descriptor with a bLength of 0, the size
check in ims_pcu_get_cdc_union_desc() would pass since 0 is not greater
than the remaining length. The loop would then subtract 0 from buflen and
add 0 to buf, causing it to never advance or terminate. Would this hang
the USB enumeration thread?

[Severity: High]
This is a pre-existing issue, but does ims_pcu_parse_cdc_data() verify the
endpoint direction and type before using it?

The code assigns pcu->ep_ctrl to &alt->endpoint[0].desc and passes its
address to usb_fill_int_urb(). If a device exposes a Bulk or OUT endpoint
at this index instead, could submitting it as an Interrupt IN URB lead to
unhandled behavior or warnings in host controller drivers?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260525151410.42750-1-liangzhan5dev@gmail.com?part=1