Re: [PATCH 06/11] Input: ims-pcu - validate control endpoint type

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The control endpoint's `wMaxPacketSize` is not validated for being non-zero, allowing a malicious USB device to trigger a `WARN_ON` in the DMA allocator.
- [High] In `ims_pcu_buffers_free()`, `usb_free_coherent()` is called for `urb_in_buf` using `max_out_size` instead of the size it was allocated with (`max_in_size`).
- [High] The driver re-initializes an already initialized completion structure using `init_completion()` instead of `reinit_completion()`.
- [High] The driver modifies the shared buffer `pcu->cmd_buf` in softirq context without acquiring any locks, racing with process-context readers and writers.
--

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

Input: ims-pcu - validate control endpoint type

The driver currently assumes that the first endpoint of the control
interface is an interrupt IN endpoint without verifying it. A malicious
device could provide a different endpoint type, which would then be
passed to usb_fill_int_urb(), potentially leading to kernel warnings
or undefined behavior.

> diff --git a/drivers/input/misc/ims-pcu.c b/drivers/input/misc/ims-pcu.c
> index 22be49d3b879d..51b1cdc226e65 100644
> --- a/drivers/input/misc/ims-pcu.c
> +++ b/drivers/input/misc/ims-pcu.c
> @@ -1703,6 +1703,12 @@ static int ims_pcu_parse_cdc_data(struct usb_interface *intf, struct ims_pcu *pc
>  		return -ENODEV;
>  
>  	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 is the control endpoint's wMaxPacketSize
validated to ensure it is non-zero?

A malicious device can provide an interrupt IN endpoint with a
wMaxPacketSize of 0, which the driver does not validate. Later,
ims_pcu_buffers_alloc() attempts to allocate DMA memory for the control
buffer by calling usb_alloc_coherent() with a size of 0.

This invokes the underlying DMA allocation APIs, which trigger a WARN_ON_ONCE
for zero-sized allocations. Furthermore, it causes usb_submit_urb() to be
called with a 0-length transfer buffer for an interrupt IN endpoint, which
may lead to undefined behavior in the host controller driver.

[Severity: High]
This isn't a bug introduced by this patch, but does ims_pcu_buffers_free()
pass the correct size when freeing urb_in_buf?

Looking at the allocation in ims_pcu_buffers_alloc():

        pcu->urb_in_buf = usb_alloc_coherent(pcu->udev, pcu->max_in_size,
                                             GFP_KERNEL, &pcu->read_dma);

And the freeing in ims_pcu_buffers_free():

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

It appears max_out_size is used to free the buffer instead of max_in_size.
If a device has different max packet sizes for its in and out endpoints,
this size mismatch violates DMA API requirements, as the size passed to
dma_free_coherent() must exactly match the size passed to
dma_alloc_coherent().

Can this lead to memory corruption in the DMA allocator or memory leaks?

[Severity: High]
This is a pre-existing issue, but does the driver safely re-use the cmd_done
completion in __ims_pcu_execute_command()?

__ims_pcu_execute_command() calls init_completion() before sending a command:

        pcu->expected_response = expected_response;
        init_completion(&pcu->cmd_done);

If a late or duplicate response from a previous command arrives just as
init_completion() is executing, the softirq handler
(ims_pcu_handle_response) might concurrently call complete(&pcu->cmd_done).

Because init_completion() unconditionally re-initializes the wait queue's
spinlock, this concurrent access can cause lock corruption and a potential
kernel panic.

Should the driver use reinit_completion() here instead to safely reuse the
completion structure?

[Severity: High]
This isn't a bug introduced by this patch, but is the shared buffer
pcu->cmd_buf adequately protected against concurrent access?

When a response is received, the ims_pcu_handle_response() softirq executes
the buffer copy without holding any spinlock:

        memcpy(pcu->cmd_buf, pcu->read_buf, pcu->read_pos);

Process contexts read and write pcu->cmd_buf while only holding the sleepable
cmd_mutex. Since softirqs don't acquire cmd_mutex, there is no
synchronization.

If a malicious device sends multiple identical or spoofed response packets
for the same command, the softirq will repeatedly overwrite cmd_buf
concurrently while the process context is reading it after waking up from
wait_for_completion().

Could this data race cause the process context to read corrupted data?

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