[PATCH] Input: ims-pcu - fix usb_free_coherent() size in ims_pcu_buffers_alloc()

[PATCH] Input: ims-pcu - fix usb_free_coherent() size in ims_pcu_buffers_alloc()

[Thomas Fourier]

The input buffer size is pcu->max_in_size, but pcu->max_out_size is
passed to usb_free_coherent().

Change size to match the allocation size.

Fixes: 628329d52474 ("Input: add IMS Passenger Control Unit driver")
Cc: <stable@vger.kernel.org>
Signed-off-by: Thomas Fourier <fourier.thomas@gmail.com>
---
 drivers/input/misc/ims-pcu.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/input/misc/ims-pcu.c b/drivers/input/misc/ims-pcu.c
index 4c022a36dbe8..7a1cb9333f53 100644
--- a/drivers/input/misc/ims-pcu.c
+++ b/drivers/input/misc/ims-pcu.c
@@ -1624,7 +1624,7 @@ 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,
+	usb_free_coherent(pcu->udev, pcu->max_in_size,
 			  pcu->urb_in_buf, pcu->read_dma);
 
 	kfree(pcu->urb_out_buf);
-- 
2.43.0

Re: [PATCH] Input: ims-pcu - fix usb_free_coherent() size in ims_pcu_buffers_alloc()

[sashiko-bot]

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

New issues:
- [Low] Commit message inaccuracy regarding the fixed function.

Pre-existing issues:
- [High] Failure to release explicitly claimed secondary data interface on disconnect leaves the interface permanently claimed.
- [High] Memory leak of firmware object in `ims_pcu_process_async_firmware()`.
- [High] Missing interface verification in `ims_pcu_disconnect()` leads to Use-After-Free and Double-Free vulnerabilities.
- [High] Race condition in sysfs `reset_device` store callback due to missing mutex.
- [High] Type confusion vulnerability in CDC union descriptor parsing.
- [Medium] Missing endpoint type validation for the control interface.
- [Medium] Logic error in `ims_pcu_reset_packet()` breaks the parser state machine.
- [Medium] Out-of-bounds read and information leak due to missing response length checks.
- [Medium] Violation of DMA mapping rules by performing DMA on an embedded structure.
- [Low] Out-of-bounds read in `ims_pcu_irq()` debug logging.
--

commit b02b6acbca140f2c98e04a2bf7bf5d8b5b24ae41
Author: Thomas Fourier <fourier.thomas@gmail.com>

Input: ims-pcu - fix usb_free_coherent() size in ims_pcu_buffers_alloc()

This commit changes the size passed to usb_free_coherent() in
ims_pcu_buffers_free() from max_out_size to max_in_size, properly matching
the input buffer's allocation size.

> Input: ims-pcu - fix usb_free_coherent() size in ims_pcu_buffers_alloc()
> 
> The input buffer size is pcu->max_in_size, but pcu->max_out_size is
> passed to usb_free_coherent().
> 
> Change size to match the allocation size.

[Severity: Low]
Is the function name in the subject line accurate? This modifies
ims_pcu_buffers_free(), but the subject mentions ims_pcu_buffers_alloc().
The error handling path in ims_pcu_buffers_alloc() appears to already
correctly use pcu->max_in_size.

> 
> Fixes: 628329d52474 ("Input: add IMS Passenger Control Unit driver")
> Cc: <stable@vger.kernel.org>
> Signed-off-by: Thomas Fourier <fourier.thomas@gmail.com>

> diff --git a/drivers/input/misc/ims-pcu.c b/drivers/input/misc/ims-pcu.c
> index 4c022a36dbe84..7a1cb9333f53c 100644
> --- a/drivers/input/misc/ims-pcu.c
> +++ b/drivers/input/misc/ims-pcu.c
> @@ -1624,7 +1624,7 @@ 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,
> +	usb_free_coherent(pcu->udev, pcu->max_in_size,
>  			  pcu->urb_in_buf, pcu->read_dma);
>  
>  	kfree(pcu->urb_out_buf);


[Severity: High]
This is a pre-existing issue, but does ims_pcu_disconnect() leak the claimed
secondary data interface?

drivers/input/misc/ims-pcu.c:ims_pcu_disconnect() {
[ ... ]
	if (alt->desc.bInterfaceClass != USB_CLASS_COMM)
		return;

	ims_pcu_stop_io(pcu);
[ ... ]
	ims_pcu_buffers_free(pcu);
	kfree(pcu);
}

During ims_pcu_probe(), the driver claims the secondary data interface using
usb_driver_claim_interface(), but ims_pcu_disconnect() never calls
usb_driver_release_interface() for pcu->data_intf when the primary control
interface is unbound. Does this leave the data interface permanently claimed,
causing subsequent binds to fail with -EBUSY?


[Severity: High]
This is a pre-existing issue, but can this logic in ims_pcu_disconnect() lead
to a use-after-free or double-free?

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 (alt->desc.bInterfaceClass != USB_CLASS_COMM)
		return;
[ ... ]
	kfree(pcu);
}

If a malicious USB device creates a secondary data interface with the
USB_CLASS_COMM class, and the user unbinds this malicious data interface via
sysfs, does ims_pcu_disconnect() bypass the return statement and prematurely
free the pcu structure while the primary interface is still active?
Should it verify if (intf != pcu->ctrl_intf) instead?


[Severity: High]
This is a pre-existing issue, but does this code cause a type confusion
regression during probe?

In ims_pcu_parse_cdc_data(), the driver looks up the control interface using
union_desc->bMasterInterface0 from the untrusted CDC union descriptor, and
assigns it to pcu->ctrl_intf without validating that pcu->ctrl_intf matches
the interface currently being probed.

drivers/input/misc/ims-pcu.c:ims_pcu_probe() {
[ ... ]
	error = ims_pcu_parse_cdc_data(intf, pcu);
	if (error)
		goto err_free_mem;
[ ... ]
	usb_set_intfdata(pcu->ctrl_intf, pcu);
[ ... ]

If a malicious device points bMasterInterface0 to an interface owned by a
completely different driver, does usb_set_intfdata() overwrite the other
driver's private data pointer, potentially leading to memory corruption?


[Severity: High]
This is a pre-existing issue, but does this error path leak the firmware
object?

If ihex_validate_fw(fw) returns an error in ims_pcu_process_async_firmware(),
the function jumps to the out label, which bypasses the release_firmware(fw)
call. Since fw is a valid pointer when ihex_validate_fw() is called, does this
directly leak the firmware object memory?


[Severity: High]
This is a pre-existing issue, but is there a race condition in the sysfs
reset_device store callback?

The ims_pcu_reset_device() sysfs callback directly calls
ims_pcu_execute_command() without acquiring pcu->cmd_mutex. This function
manipulates shared variables like pcu->expected_response, pcu->cmd_done, and
pcu->cmd_buf. If triggered concurrently with other commands (such as LED
brightness updates which properly lock the mutex), does this cause data races
and corruption of the shared command buffer?


[Severity: Medium]
This is a pre-existing issue, but does the driver validate the endpoint type
for the control interface?

In ims_pcu_parse_cdc_data(), the driver extracts the first endpoint:
pcu->ep_ctrl = &alt->endpoint[0].desc

Does it fail to verify that it is an IN interrupt endpoint? If a malicious
device provides a different endpoint type (e.g., bulk OUT), which is then
passed to usb_fill_int_urb() and usb_submit_urb(), does this trigger a
BOGUS urb xfer kernel warning?


[Severity: Medium]
This is a pre-existing issue, but is there a logic error in
ims_pcu_reset_packet() that breaks the parser state machine?

ims_pcu_reset_packet() is intended to reset the protocol state machine to wait
for the next packet, but it sets pcu->have_stx = true.

Does this cause the state machine to bypass the wait for the next STX byte
after the first packet is processed, and immediately accept garbage bytes? When
the actual STX byte later arrives, does the driver spam the kernel log with
Unexpected STX at byte X, discarding old data warnings?


[Severity: Medium]
This is a pre-existing issue, but does this code lead to out-of-bounds reads
and information leaks?

Functions like ims_pcu_report_events() and ims_pcu_get_info() process data
directly from the response buffers (e.g., pcu->read_buf[3] or pcu->cmd_buf)
without checking pcu->read_pos or pcu->cmd_buf_len to ensure the device
actually sent enough data.

If a device sends a shortened response, does the driver parse stale,
uninitialized data remaining in the buffer from previous commands?


[Severity: Medium]
This is a pre-existing issue, but does this code violate DMA mapping rules?

In ims_pcu_line_setup(), pcu->cmd_buf is passed to usb_control_msg() for a DMA
transfer:

drivers/input/misc/ims-pcu.c:ims_pcu_line_setup() {
[ ... ]
	struct usb_cdc_line_coding *line = (void *)pcu->cmd_buf;
[ ... ]
	error = usb_control_msg(pcu->udev, usb_sndctrlpipe(pcu->udev, 0),
				USB_CDC_REQ_SET_LINE_CODING,
				USB_TYPE_CLASS | USB_RECIP_INTERFACE,
				0, interface->desc.bInterfaceNumber,
				line, sizeof(struct usb_cdc_line_coding),
				5000);
[ ... ]

Since pcu->cmd_buf is an array embedded directly inside the struct ims_pcu
allocation, and embedded fields are not guaranteed to be strictly cacheline
aligned, can cache maintenance operations on this buffer corrupt adjacent
fields in the struct ims_pcu on architectures without hardware cache coherency?


[Severity: Low]
This is a pre-existing issue, but is there an out-of-bounds read in the
ims_pcu_irq() debug logging?

In ims_pcu_irq(), the debug logging unconditionally prints data from
pcu->urb_in_buf up to urb->actual_length bytes. If the interrupt fired for
pcu->urb_ctrl, the actual transferred data resides in pcu->urb_ctrl_buf.

Does this result in printing unrelated stale data, and if urb->actual_length
from the control URB exceeds pcu->max_in_size, does it read out of bounds up
to the format limit?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260522085412.45430-2-fourier.thomas@gmail.com?part=1