[PATCH 01/11] Input: ims-pcu - release data interface on disconnect

[PATCH 01/11] Input: ims-pcu - release data interface on disconnect

[Dmitry Torokhov]

During probe the driver claims the data interface, but it never releases
it. Release it in disconnect to avoid leaving it permanently claimed.

Fixes: 628329d52474 ("Input: add IMS Passenger Control Unit driver")
Cc: stable@vger.kernel.org
Reported-by: Sashiko bot <sashiko-bot@kernel.org>
Assisted-by: Gemini:gemini-3.1-pro
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/misc/ims-pcu.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/drivers/input/misc/ims-pcu.c b/drivers/input/misc/ims-pcu.c
index 7a1cb9333f53..57d917387544 100644
--- a/drivers/input/misc/ims-pcu.c
+++ b/drivers/input/misc/ims-pcu.c
@@ -2090,6 +2090,7 @@ static void ims_pcu_disconnect(struct usb_interface *intf)
 		ims_pcu_destroy_application_mode(pcu);
 
 	ims_pcu_buffers_free(pcu);
+	usb_driver_release_interface(&ims_pcu_driver, pcu->data_intf);
 	kfree(pcu);
 }
 
-- 
2.54.0.746.g67dd491aae-goog

[PATCH 02/11] Input: ims-pcu - fix use-after-free and double-free in disconnect

[Dmitry Torokhov]

ims_pcu_disconnect() only intended to perform cleanup when the primary
(control) interface is unbound. However, it currently relies on the
interface class to distinguish between control and data interfaces.
A malicious device could present a data interface with the same class
as the control interface, leading to premature cleanup and potential
use-after-free or double-free.

Switch to verifying that the interface being disconnected is indeed
the control interface.

Fixes: 628329d52474 ("Input: add IMS Passenger Control Unit driver")
Cc: stable@vger.kernel.org
Reported-by: Sashiko bot <sashiko-bot@kernel.org>
Assisted-by: Gemini:gemini-3.1-pro
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/misc/ims-pcu.c | 3 +--
 1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/drivers/input/misc/ims-pcu.c b/drivers/input/misc/ims-pcu.c
index 57d917387544..d0934d577b5e 100644
--- a/drivers/input/misc/ims-pcu.c
+++ b/drivers/input/misc/ims-pcu.c
@@ -2071,7 +2071,6 @@ static int ims_pcu_probe(struct usb_interface *intf,
 static void ims_pcu_disconnect(struct usb_interface *intf)
 {
 	struct ims_pcu *pcu = usb_get_intfdata(intf);
-	struct usb_host_interface *alt = intf->cur_altsetting;
 
 	usb_set_intfdata(intf, NULL);
 
@@ -2079,7 +2078,7 @@ static void ims_pcu_disconnect(struct usb_interface *intf)
 	 * 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)
+	if (intf != pcu->ctrl_intf)
 		return;
 
 	ims_pcu_stop_io(pcu);
-- 
2.54.0.746.g67dd491aae-goog

Re: [PATCH 02/11] Input: ims-pcu - fix use-after-free and double-free in disconnect

[sashiko-bot]

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

New issues:
- [High] Concurrent unbinding of both the control and data interfaces via sysfs triggers a Use-After-Free on the `pcu` structure.

Pre-existing issues:
- [Critical] The driver blindly trusts the CDC Union Descriptor to identify interfaces, leading to memory corruption during probe and an unconditional NULL pointer dereference in disconnect.
- [High] In `ims_pcu_buffers_free`, the input DMA buffer is freed using the maximum output buffer size instead of its actual allocated size.
--

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

Input: ims-pcu - fix use-after-free and double-free in disconnect

This commit changes the interface verification in ims_pcu_disconnect() to
directly compare the disconnecting interface against the saved control
interface pointer. This prevents premature cleanup caused by malicious
devices presenting multiple interfaces with the same class.

> diff --git a/drivers/input/misc/ims-pcu.c b/drivers/input/misc/ims-pcu.c
> index 89737ef381c1c..2ad72fe4a51d9 100644
> --- a/drivers/input/misc/ims-pcu.c
> +++ b/drivers/input/misc/ims-pcu.c
> @@ -2071,7 +2071,6 @@ static int ims_pcu_probe(struct usb_interface *intf,
>  static void ims_pcu_disconnect(struct usb_interface *intf)
>  {
>  	struct ims_pcu *pcu = usb_get_intfdata(intf);
> -	struct usb_host_interface *alt = intf->cur_altsetting;
>  
>  	usb_set_intfdata(intf, NULL);
>  
> @@ -2079,7 +2078,7 @@ static void ims_pcu_disconnect(struct usb_interface *intf)
>  	 * 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)
> +	if (intf != pcu->ctrl_intf)

[Severity: High]
Can this introduce a use-after-free if both control and data interfaces are
unbound concurrently via sysfs?

If Process A unbinds the data interface, it gets the pcu pointer from
usb_get_intfdata() and might be preempted. The USB core sets the data
interface condition to USB_INTERFACE_UNBINDING.

Process B then unbinds the control interface, enters ims_pcu_disconnect(),
and calls usb_driver_release_interface() on the data interface.

Since the data interface is unbinding, usb_driver_release_interface() returns
immediately without waiting, and Process B proceeds to kfree(pcu).

When Process A resumes, evaluating intf != pcu->ctrl_intf would dereference
the freed pcu pointer.

[Severity: Critical]
This is a pre-existing issue, but does this new check introduce an
unconditional NULL pointer dereference if the CDC Union Descriptor is spoofed?

During ims_pcu_probe(), the driver uses bMasterInterface0 from the CDC Union
Descriptor as pcu->ctrl_intf without verifying it matches the probed
interface.

If a malicious device supplies a descriptor pointing to an interface owned by
another driver, the actual probed interface's intfdata remains NULL.

In that scenario, usb_get_intfdata() returns NULL for pcu, and evaluating
intf != pcu->ctrl_intf here will panic. Note that this is fixed later in the
patch series by commit af29ec151c71a.

>  		return;
>  
>  	ims_pcu_stop_io(pcu);

[Severity: High]
This is a pre-existing issue, but could ims_pcu_buffers_free() be freeing
the input buffer with the wrong size?

Looking at ims_pcu_buffers_alloc(), pcu->urb_in_buf is allocated using
pcu->max_in_size. However, ims_pcu_buffers_free() appears to free it using
pcu->max_out_size:

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

If a device specifies different maximum packet sizes for IN and OUT endpoints,
calling usb_free_coherent() with the wrong size could corrupt the DMA
allocator's buddy system.

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

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

[Dmitry Torokhov]

The driver currently trusts the bMasterInterface0 from the CDC union
descriptor without verifying that it matches the interface being
probed. This could lead to the driver overwriting the private data of
another interface.

Validate that the control interface found in the descriptor is indeed
the one we are probing.

Fixes: 628329d52474 ("Input: add IMS Passenger Control Unit driver")
Cc: stable@vger.kernel.org
Reported-by: Sashiko bot <sashiko-bot@kernel.org>
Assisted-by: Gemini:gemini-3.1-pro
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@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 d0934d577b5e..a134483e543b 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;
-- 
2.54.0.746.g67dd491aae-goog

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

[sashiko-bot]

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

[PATCH 04/11] Input: ims-pcu - fix firmware leak in async update

[Dmitry Torokhov]

The firmware object was not being released if validation failed.
Use __free(firmware) to ensure the firmware is always released.

Fixes: 628329d52474 ("Input: add IMS Passenger Control Unit driver")
Cc: stable@vger.kernel.org
Reported-by: Sashiko bot <sashiko-bot@kernel.org>
Assisted-by: Gemini:gemini-3.1-pro
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/misc/ims-pcu.c | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/drivers/input/misc/ims-pcu.c b/drivers/input/misc/ims-pcu.c
index a134483e543b..f86f9a5a7564 100644
--- a/drivers/input/misc/ims-pcu.c
+++ b/drivers/input/misc/ims-pcu.c
@@ -934,9 +934,10 @@ static int ims_pcu_handle_firmware_update(struct ims_pcu *pcu,
 	return retval;
 }
 
-static void ims_pcu_process_async_firmware(const struct firmware *fw,
+static void ims_pcu_process_async_firmware(const struct firmware *_fw,
 					   void *context)
 {
+	const struct firmware *fw __free(firmware) = _fw;
 	struct ims_pcu *pcu = context;
 	int error;
 
@@ -956,8 +957,6 @@ static void ims_pcu_process_async_firmware(const struct firmware *fw,
 	scoped_guard(mutex, &pcu->cmd_mutex)
 		ims_pcu_handle_firmware_update(pcu, fw);
 
-	release_firmware(fw);
-
 out:
 	complete(&pcu->async_firmware_done);
 }
-- 
2.54.0.746.g67dd491aae-goog

Re: [PATCH 04/11] Input: ims-pcu - fix firmware leak in async update

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Mixing `goto`-based error handling with scope-based cleanup (`__free`) violates subsystem guidelines and alters cleanup ordering.
--

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

Input: ims-pcu - fix firmware leak in async update

This commit changes the async firmware update routine to use scope-based
cleanup for the firmware object. This ensures the firmware is released
if validation fails.

> diff --git a/drivers/input/misc/ims-pcu.c b/drivers/input/misc/ims-pcu.c
> index 5d1df07ac2a2a..e6603aaf7b303 100644
> --- a/drivers/input/misc/ims-pcu.c
> +++ b/drivers/input/misc/ims-pcu.c
[ ... ]
> @@ -934,9 +934,10 @@ static int ims_pcu_handle_firmware_update(struct ims_pcu *pcu,
> -static void ims_pcu_process_async_firmware(const struct firmware *fw,
> +static void ims_pcu_process_async_firmware(const struct firmware *_fw,
>  					   void *context)
>  {
> +	const struct firmware *fw __free(firmware) = _fw;
>  	struct ims_pcu *pcu = context;
>  	int error;
[ ... ]
> @@ -956,8 +957,6 @@ static void ims_pcu_process_async_firmware(const struct firmware *fw,
>  	scoped_guard(mutex, &pcu->cmd_mutex)
>  		ims_pcu_handle_firmware_update(pcu, fw);
>  
> -	release_firmware(fw);
> -
>  out:
>  	complete(&pcu->async_firmware_done);
>  }

[Severity: Medium]
Does mixing goto with __free() in this function alter the intended cleanup
order and cause a regression?

Because scope-based cleanup handlers execute at the end of the variable's
scope (which is after the out label), release_firmware() will now run after
complete() rather than before it.

The cleanup guidelines also note that for a given routine, we should avoid
mixing goto and __free(), and instead convert all resources to scope-based
cleanup or convert none of them.

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

[PATCH 05/11] Input: ims-pcu - fix race condition in reset_device sysfs callback

[Dmitry Torokhov]

The ims_pcu_reset_device() sysfs callback calls ims_pcu_execute_command()
without acquiring pcu->cmd_mutex. This can lead to data races and
corruption of the shared command buffer if triggered concurrently with
other commands.

Acquire pcu->cmd_mutex before calling ims_pcu_execute_command().

Fixes: 628329d52474 ("Input: add IMS Passenger Control Unit driver")
Cc: stable@vger.kernel.org
Reported-by: Sashiko bot <sashiko-bot@kernel.org>
Assisted-by: Gemini:gemini-3.1-pro
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/misc/ims-pcu.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/drivers/input/misc/ims-pcu.c b/drivers/input/misc/ims-pcu.c
index f86f9a5a7564..7fdff9dd1b5f 100644
--- a/drivers/input/misc/ims-pcu.c
+++ b/drivers/input/misc/ims-pcu.c
@@ -1152,6 +1152,8 @@ static ssize_t ims_pcu_reset_device(struct device *dev,
 
 	dev_info(pcu->dev, "Attempting to reset device\n");
 
+	guard(mutex)(&pcu->cmd_mutex);
+
 	error = ims_pcu_execute_command(pcu, PCU_RESET, &reset_byte, 1);
 	if (error) {
 		dev_info(pcu->dev,
-- 
2.54.0.746.g67dd491aae-goog

Re: [PATCH 05/11] Input: ims-pcu - fix race condition in reset_device sysfs callback

[sashiko-bot]

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

Pre-existing issues:
- [High] Incorrect size passed to usb_free_coherent() when freeing the read buffer.
- [High] Sysfs callbacks on `data_intf` race with buffer allocation/freeing, leading to Use-After-Free or NULL pointer dereference.
- [High] Sequence number (ack_id) race condition allows stale delayed responses to complete a new command prematurely.
--

commit 1a84560748d9dd10fa9a7709e4c015dd9ef492e6
Author: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Subject: Input: ims-pcu - fix race condition in reset_device sysfs callback

This commit fixes a race condition in the ims_pcu_reset_device sysfs callback
by acquiring the command mutex before executing commands. This prevents data
races and corruption of the shared command buffer when commands run
concurrently.

> diff --git a/drivers/input/misc/ims-pcu.c b/drivers/input/misc/ims-pcu.c
> index e6603aaf7b303..22be49d3b879d 100644
> --- a/drivers/input/misc/ims-pcu.c
> +++ b/drivers/input/misc/ims-pcu.c
> @@ -1152,6 +1152,8 @@ static ssize_t ims_pcu_reset_device(struct device *dev,
>  
>  	dev_info(pcu->dev, "Attempting to reset device\n");
>  
> +	guard(mutex)(&pcu->cmd_mutex);
> +

[Severity: High]
This is a pre-existing issue, but can this sysfs callback race with buffer
allocation during probe?

During ims_pcu_probe(), usb_driver_claim_interface() binds data_intf to the
driver before ims_pcu_buffers_alloc() is called:

drivers/input/misc/ims-pcu.c:ims_pcu_probe() {
    ...
	error = usb_driver_claim_interface(&ims_pcu_driver,
					   pcu->data_intf, pcu);
    ...
	error = ims_pcu_buffers_alloc(pcu);
    ...
}

This implicitly registers the sysfs attributes before the command buffers are
allocated. Can a concurrent sysfs access like writing to reset_device execute
ims_pcu_execute_command() and write to pcu->urb_out_buf while it is still
null?

Similarly, during ims_pcu_disconnect(), ims_pcu_buffers_free() is called
before usb_driver_release_interface() removes the sysfs attributes. Could a
concurrent sysfs operation access the freed buffers, causing a use-after-free?

>  	error = ims_pcu_execute_command(pcu, PCU_RESET, &reset_byte, 1);
>  	if (error) {
>  		dev_info(pcu->dev,

There are also a couple of other pre-existing issues in this driver:

[Severity: High]
This isn't a bug introduced by this patch, but is the correct size passed to
usb_free_coherent() when freeing the read buffer?

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);
    ...
}

Is it possible this mismatch could cause memory leaks or crashes on
architectures that rely on the size parameter for unmapping pages?

[Severity: High]
This is a pre-existing issue, but does the sequence number initialization in
__ims_pcu_execute_command() allow stale responses to complete a new command
prematurely?

In __ims_pcu_execute_command(), pcu->expected_response is set and
init_completion() is called before ims_pcu_send_command():

drivers/input/misc/ims-pcu.c:__ims_pcu_execute_command() {
    ...
	pcu->expected_response = expected_response;
	init_completion(&pcu->cmd_done);

	error = ims_pcu_send_command(pcu, command, data, len);
    ...
}

The sequence number pcu->ack_id is only incremented later inside
ims_pcu_send_command().

If a previous command timed out, could a delayed response arrive from the
hardware during this exact window? Its sequence number would match the still
unincremented pcu->ack_id - 1 checked in ims_pcu_handle_response():

drivers/input/misc/ims-pcu.c:ims_pcu_handle_response() {
    ...
		if (pcu->read_buf[0] == pcu->expected_response &&
		    pcu->read_buf[1] == pcu->ack_id - 1) {
			memcpy(pcu->cmd_buf, pcu->read_buf, pcu->read_pos);
			pcu->cmd_buf_len = pcu->read_pos;
			complete(&pcu->cmd_done);
			return;
		}
    ...
}

Would this cause the waiting thread to wake up immediately and process the
stale data instead of the actual response?

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

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

[Dmitry Torokhov]

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.

Verify that the control endpoint is an interrupt IN endpoint.

Fixes: 628329d52474 ("Input: add IMS Passenger Control Unit driver")
Cc: stable@vger.kernel.org
Reported-by: Sashiko bot <sashiko-bot@kernel.org>
Assisted-by: Gemini:gemini-3.1-pro
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/misc/ims-pcu.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/input/misc/ims-pcu.c b/drivers/input/misc/ims-pcu.c
index 7fdff9dd1b5f..0e7a783526e6 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);
 
 	pcu->data_intf = usb_ifnum_to_if(pcu->udev,
-- 
2.54.0.746.g67dd491aae-goog

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

[sashiko-bot]

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

[PATCH 07/11] Input: ims-pcu - fix logic error in packet reset

[Dmitry Torokhov]

ims_pcu_reset_packet() incorrectly sets have_stx to true, which implies
that the start-of-packet delimiter has already been received. This
causes the protocol parser to skip waiting for the next STX byte and
potentially process garbage data.

Correctly set have_stx to false when resetting the packet state.

Fixes: 875115b82c29 ("Input: ims-pcu - fix heap-buffer-overflow in ims_pcu_process_data()")
Cc: stable@vger.kernel.org
Reported-by: Sashiko bot <sashiko-bot@kernel.org>
Assisted-by: Gemini:gemini-3.1-pro
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@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 0e7a783526e6..cdb46b2297a2 100644
--- a/drivers/input/misc/ims-pcu.c
+++ b/drivers/input/misc/ims-pcu.c
@@ -440,7 +440,7 @@ static void ims_pcu_handle_response(struct ims_pcu *pcu)
 
 static void ims_pcu_reset_packet(struct ims_pcu *pcu)
 {
-	pcu->have_stx = true;
+	pcu->have_stx = false;
 	pcu->have_dle = false;
 	pcu->read_pos = 0;
 	pcu->check_sum = 0;
-- 
2.54.0.746.g67dd491aae-goog

Re: [PATCH 07/11] Input: ims-pcu - fix logic error in packet reset

[sashiko-bot]

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

Pre-existing issues:
- [High] Incorrect DMA Buffer Free Size in ims_pcu_buffers_free
--

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

    Input: ims-pcu - fix logic error in packet reset

    ims_pcu_reset_packet() incorrectly sets have_stx to true, which implies
    that the start-of-packet delimiter has already been received. This
    causes the protocol parser to skip waiting for the next STX byte and
    potentially process garbage data.

    Correctly set have_stx to false when resetting the packet state.

> diff --git a/drivers/input/misc/ims-pcu.c b/drivers/input/misc/ims-pcu.c
> index 51b1cdc226e65..55e5599cf5107 100644
> --- a/drivers/input/misc/ims-pcu.c
> +++ b/drivers/input/misc/ims-pcu.c
> @@ -440,7 +440,7 @@ static void ims_pcu_handle_response(struct ims_pcu *pcu)
>  
>  static void ims_pcu_reset_packet(struct ims_pcu *pcu)
>  {
> -	pcu->have_stx = true;
> +	pcu->have_stx = false;
>  	pcu->have_dle = false;
>  	pcu->read_pos = 0;
>  	pcu->check_sum = 0;

[Severity: High]
This is a pre-existing issue, but I noticed a potential DMA API violation in
the ims_pcu_buffers_free() function.

Looking at drivers/input/misc/ims-pcu.c: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);

Should this use pcu->max_in_size instead of pcu->max_out_size when
freeing pcu->urb_in_buf? Using the incorrect length could cause memory
leaks or memory corruption within the DMA allocator.

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

[PATCH 08/11] Input: ims-pcu - fix out-of-bounds read in ims_pcu_irq() debug logging

[Dmitry Torokhov]

The debug logging in ims_pcu_irq() unconditionally prints data from
pcu->urb_in_buf. However, if the interrupt fired for pcu->urb_ctrl, the
actual data resides in pcu->urb_ctrl_buf. If urb->actual_length for the
control URB exceeds pcu->max_in_size, this leads to an out-of-bounds
read.

Fix this by printing from the correct buffer associated with the URB.

Fixes: 628329d52474 ("Input: add IMS Passenger Control Unit driver")
Cc: stable@vger.kernel.org
Reported-by: Sashiko bot <sashiko-bot@kernel.org>
Assisted-by: Gemini:gemini-3.1-pro
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@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 cdb46b2297a2..23e576500890 100644
--- a/drivers/input/misc/ims-pcu.c
+++ b/drivers/input/misc/ims-pcu.c
@@ -1529,7 +1529,7 @@ static void ims_pcu_irq(struct urb *urb)
 	}
 
 	dev_dbg(pcu->dev, "%s: received %d: %*ph\n", __func__,
-		urb->actual_length, urb->actual_length, pcu->urb_in_buf);
+		urb->actual_length, urb->actual_length, urb->transfer_buffer);
 
 	if (urb == pcu->urb_in)
 		ims_pcu_process_data(pcu, urb);
-- 
2.54.0.746.g67dd491aae-goog

Re: [PATCH 08/11] Input: ims-pcu - fix out-of-bounds read in ims_pcu_irq() debug logging

[sashiko-bot]

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

Pre-existing issues:
- [High] The coherent DMA memory for the IN URB buffer is freed using the OUT endpoint's maximum packet size (`max_out_size`) instead of the IN endpoint's size (`max_in_size`).
--

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

Input: ims-pcu - fix out-of-bounds read in ims_pcu_irq() debug logging

This commit fixes an out-of-bounds read in the debug logging of
ims_pcu_irq(). It updates the dev_dbg() call to print data from the
transfer_buffer associated with the URB rather than unconditionally
using the input URB buffer.

> diff --git a/drivers/input/misc/ims-pcu.c b/drivers/input/misc/ims-pcu.c
> index 55e5599cf5107..d7a3f4c7214a0 100644
> --- a/drivers/input/misc/ims-pcu.c
> +++ b/drivers/input/misc/ims-pcu.c
> @@ -1529,7 +1529,7 @@ static void ims_pcu_irq(struct urb *urb)
>  	}
>  
>  	dev_dbg(pcu->dev, "%s: received %d: %*ph\n", __func__,
> -		urb->actual_length, urb->actual_length, pcu->urb_in_buf);
> +		urb->actual_length, urb->actual_length, urb->transfer_buffer);
>  
>  	if (urb == pcu->urb_in)
>  		ims_pcu_process_data(pcu, urb);

[Severity: High]
This is a pre-existing issue, but I noticed a problem with how the memory for
the IN URB buffer is managed when reviewing this driver.

Does this code pass the correct size to the DMA free function?

Looking at ims_pcu_buffers_free(), the input buffer is freed using
max_out_size instead of max_in_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);
    ...
}

The buffer is originally allocated in ims_pcu_buffers_alloc() using
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);
    ...
}

Is it possible that max_in_size and max_out_size are different?

USB endpoints can have asymmetric packet sizes, so max_in_size and max_out_size
are not guaranteed to be identical.

Could this mismatch between the allocation size and the free size lead to DMA
pool corruption or memory leaks?

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

[PATCH 09/11] Input: ims-pcu - fix DMA mapping violation in line setup

[Dmitry Torokhov]

In ims_pcu_line_setup(), the driver uses pcu->cmd_buf as a transfer
buffer for usb_control_msg(). However, pcu->cmd_buf is embedded in the
struct ims_pcu allocation, which violates DMA mapping rules regarding
cacheline alignment.

Use a heap-allocated buffer for the line coding data instead.

Fixes: 628329d52474 ("Input: add IMS Passenger Control Unit driver")
Cc: stable@vger.kernel.org
Reported-by: Sashiko bot <sashiko-bot@kernel.org>
Assisted-by: Gemini:gemini-3.1-pro
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/misc/ims-pcu.c | 9 +++++++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/drivers/input/misc/ims-pcu.c b/drivers/input/misc/ims-pcu.c
index 23e576500890..3b119bc81c85 100644
--- a/drivers/input/misc/ims-pcu.c
+++ b/drivers/input/misc/ims-pcu.c
@@ -1790,11 +1790,16 @@ static void ims_pcu_stop_io(struct ims_pcu *pcu)
 static int ims_pcu_line_setup(struct ims_pcu *pcu)
 {
 	struct usb_host_interface *interface = pcu->ctrl_intf->cur_altsetting;
-	struct usb_cdc_line_coding *line = (void *)pcu->cmd_buf;
+	struct usb_cdc_line_coding *line __free(kfree) =
+				kmalloc(sizeof(*line), GFP_KERNEL);
 	int error;
 
-	memset(line, 0, sizeof(*line));
+	if (!line)
+		return -ENOMEM;
+
 	line->dwDTERate = cpu_to_le32(57600);
+	line->bCharFormat = USB_CDC_1_STOP_BITS;
+	line->bParityType = USB_CDC_NO_PARITY;
 	line->bDataBits = 8;
 
 	error = usb_control_msg(pcu->udev, usb_sndctrlpipe(pcu->udev, 0),
-- 
2.54.0.746.g67dd491aae-goog

Re: [PATCH 09/11] Input: ims-pcu - fix DMA mapping violation in line setup

[sashiko-bot]

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

New issues:
- [High] Using GFP_KERNEL in a function called from a USB resume callback risks system deadlocks during suspend/resume.

Pre-existing issues:
- [High] Mismatch between allocation and free sizes for coherent DMA memory.
--

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

Input: ims-pcu - fix DMA mapping violation in line setup

This commit fixes a DMA mapping alignment violation by replacing the
embedded pcu->cmd_buf with a dynamically allocated buffer for line coding
data.

Fixes: 628329d52474 ("Input: add IMS Passenger Control Unit driver")
Cc: stable@vger.kernel.org
Reported-by: Sashiko bot <sashiko-bot@kernel.org>
Assisted-by: Gemini:gemini-3.1-pro
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

> diff --git a/drivers/input/misc/ims-pcu.c b/drivers/input/misc/ims-pcu.c
> index d7a3f4c7214a0..7746a8242f2a0 100644
> --- a/drivers/input/misc/ims-pcu.c
> +++ b/drivers/input/misc/ims-pcu.c
> @@ -1790,11 +1790,16 @@ static void ims_pcu_stop_io(struct ims_pcu *pcu)
>  static int ims_pcu_line_setup(struct ims_pcu *pcu)
>  {
>  	struct usb_host_interface *interface = pcu->ctrl_intf->cur_altsetting;
> -	struct usb_cdc_line_coding *line = (void *)pcu->cmd_buf;
> +	struct usb_cdc_line_coding *line __free(kfree) =
> +				kmalloc(sizeof(*line), GFP_KERNEL);

[Severity: High]
Does allocating memory with GFP_KERNEL here risk a system deadlock during
suspend/resume?

Since ims_pcu_line_setup() is called by the USB power management callback
ims_pcu_resume():

ims_pcu_resume() {
    if (alt->desc.bInterfaceClass == USB_CLASS_COMM) {
        retval = ims_pcu_start_io(pcu);
        if (retval == 0)
            retval = ims_pcu_line_setup(pcu);
    }
}

During the system resume phase, device locks are held by the PM core. Using
GFP_KERNEL allows direct reclaim, which can initiate filesystem and block I/O.

If these operations attempt to access a suspended device or wait on a shared
lock, it could block indefinitely. Should this use GFP_NOIO instead?

>  	int error;
>  
> -	memset(line, 0, sizeof(*line));
> +	if (!line)
> +		return -ENOMEM;
> +
>  	line->dwDTERate = cpu_to_le32(57600);
> +	line->bCharFormat = USB_CDC_1_STOP_BITS;
> +	line->bParityType = USB_CDC_NO_PARITY;
>  	line->bDataBits = 8;
>  
[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but is there a mismatch between the
allocation and free sizes for the coherent DMA memory?

Looking at ims_pcu_buffers_alloc(), pcu->urb_in_buf is dynamically allocated
with a size of pcu->max_in_size:

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(), the same buffer is freed using
pcu->max_out_size instead:

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

Since max_in_size and max_out_size are initialized independently from their
respective endpoint descriptors, they can differ. Can passing the incorrect
size to usb_free_coherent() result in memory corruption or page leaks?

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

[PATCH 10/11] Input: ims-pcu - add response length checks

[Dmitry Torokhov]

The driver processes response data from device buffers without verifying
that the device actually sent enough data. This can lead to
out-of-bounds reads or processing stale data.

Add checks for the expected response length before accessing the
buffers.

Fixes: 628329d52474 ("Input: add IMS Passenger Control Unit driver")
Cc: stable@vger.kernel.org
Reported-by: Sashiko bot <sashiko-bot@kernel.org>
Assisted-by: Gemini:gemini-3.1-pro
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/misc/ims-pcu.c | 53 +++++++++++++++++++++++++++++++++++-
 1 file changed, 52 insertions(+), 1 deletion(-)

diff --git a/drivers/input/misc/ims-pcu.c b/drivers/input/misc/ims-pcu.c
index 3b119bc81c85..422b1be62303 100644
--- a/drivers/input/misc/ims-pcu.c
+++ b/drivers/input/misc/ims-pcu.c
@@ -406,7 +406,16 @@ static void ims_pcu_destroy_gamepad(struct ims_pcu *pcu)
 
 static void ims_pcu_report_events(struct ims_pcu *pcu)
 {
-	u32 data = get_unaligned_be32(&pcu->read_buf[3]);
+	u32 data;
+
+	/* 6-axis setting (1 byte) + button data + checksum */
+	if (pcu->read_pos < IMS_PCU_DATA_OFFSET + 1 + sizeof(data) + 1) {
+		dev_warn(pcu->dev, "Short buttons report: %d bytes\n",
+			 pcu->read_pos);
+		return;
+	}
+
+	data = get_unaligned_be32(&pcu->read_buf[IMS_PCU_DATA_OFFSET + 1]);
 
 	ims_pcu_buttons_report(pcu, data & ~IMS_PCU_GAMEPAD_MASK);
 	if (pcu->gamepad)
@@ -718,6 +727,12 @@ static int ims_pcu_get_info(struct ims_pcu *pcu)
 		return error;
 	}
 
+	if (pcu->cmd_buf_len < IMS_PCU_DATA_OFFSET + IMS_PCU_SET_INFO_SIZE + 1) {
+		dev_err(pcu->dev, "Short GET_INFO response: %d bytes\n",
+			pcu->cmd_buf_len);
+		return -EIO;
+	}
+
 	memcpy(pcu->part_number,
 	       &pcu->cmd_buf[IMS_PCU_INFO_PART_OFFSET],
 	       sizeof(pcu->part_number));
@@ -1283,6 +1298,12 @@ static int ims_pcu_read_ofn_config(struct ims_pcu *pcu, u8 addr, u8 *data)
 	if (error)
 		return error;
 
+	if (pcu->cmd_buf_len < OFN_REG_RESULT_OFFSET + 2 + 1) {
+		dev_err(pcu->dev, "Short OFN_GET_CONFIG response: %d bytes\n",
+			pcu->cmd_buf_len);
+		return -EIO;
+	}
+
 	result = (s16)get_unaligned_le16(pcu->cmd_buf + OFN_REG_RESULT_OFFSET);
 	if (result < 0)
 		return -EIO;
@@ -1843,6 +1864,12 @@ static int ims_pcu_get_device_info(struct ims_pcu *pcu)
 		return error;
 	}
 
+	if (pcu->cmd_buf_len < IMS_PCU_DATA_OFFSET + 6 + 1) {
+		dev_err(pcu->dev, "Short GET_FW_VERSION response: %d bytes\n",
+			pcu->cmd_buf_len);
+		return -EIO;
+	}
+
 	snprintf(pcu->fw_version, sizeof(pcu->fw_version),
 		 "%02d%02d%02d%02d.%c%c",
 		 pcu->cmd_buf[2], pcu->cmd_buf[3], pcu->cmd_buf[4], pcu->cmd_buf[5],
@@ -1855,6 +1882,12 @@ static int ims_pcu_get_device_info(struct ims_pcu *pcu)
 		return error;
 	}
 
+	if (pcu->cmd_buf_len < IMS_PCU_DATA_OFFSET + 6 + 1) {
+		dev_err(pcu->dev, "Short GET_BL_VERSION response: %d bytes\n",
+			pcu->cmd_buf_len);
+		return -EIO;
+	}
+
 	snprintf(pcu->bl_version, sizeof(pcu->bl_version),
 		 "%02d%02d%02d%02d.%c%c",
 		 pcu->cmd_buf[2], pcu->cmd_buf[3], pcu->cmd_buf[4], pcu->cmd_buf[5],
@@ -1867,6 +1900,12 @@ static int ims_pcu_get_device_info(struct ims_pcu *pcu)
 		return error;
 	}
 
+	if (pcu->cmd_buf_len < IMS_PCU_DATA_OFFSET + 1 + 1) {
+		dev_err(pcu->dev, "Short RESET_REASON response: %d bytes\n",
+			pcu->cmd_buf_len);
+		return -EIO;
+	}
+
 	snprintf(pcu->reset_reason, sizeof(pcu->reset_reason),
 		 "%02x", pcu->cmd_buf[IMS_PCU_DATA_OFFSET]);
 
@@ -1893,6 +1932,12 @@ static int ims_pcu_identify_type(struct ims_pcu *pcu, u8 *device_id)
 		return error;
 	}
 
+	if (pcu->cmd_buf_len < IMS_PCU_DATA_OFFSET + 1 + 1) {
+		dev_err(pcu->dev, "Short GET_DEVICE_ID response: %d bytes\n",
+			pcu->cmd_buf_len);
+		return -EIO;
+	}
+
 	*device_id = pcu->cmd_buf[IMS_PCU_DATA_OFFSET];
 	dev_dbg(pcu->dev, "Detected device ID: %d\n", *device_id);
 
@@ -1984,6 +2029,12 @@ static int ims_pcu_init_bootloader_mode(struct ims_pcu *pcu)
 		return error;
 	}
 
+	if (pcu->cmd_buf_len < IMS_PCU_DATA_OFFSET + 15 + 4 + 1) {
+		dev_err(pcu->dev, "Short QUERY_DEVICE response: %d bytes\n",
+			pcu->cmd_buf_len);
+		return -EIO;
+	}
+
 	pcu->fw_start_addr =
 		get_unaligned_le32(&pcu->cmd_buf[IMS_PCU_DATA_OFFSET + 11]);
 	pcu->fw_end_addr =
-- 
2.54.0.746.g67dd491aae-goog

Re: [PATCH 10/11] Input: ims-pcu - add response length checks

[sashiko-bot]

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

Pre-existing issues:
- [High] The patch aims to fix out-of-bounds reads and processing of stale data by adding response length checks, but the fix is incomplete. Several functions continue to read from `pcu->cmd_buf` unconditionally without verifying `pcu->cmd_buf_len`.
--

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

Input: ims-pcu - add response length checks

The driver processes response data from device buffers without verifying
that the device actually sent enough data. This commit adds checks for
the expected response length before accessing the buffers.

> diff --git a/drivers/input/misc/ims-pcu.c b/drivers/input/misc/ims-pcu.c
> index 7746a8242f2a0..7cdf40ac42ce1 100644
> --- a/drivers/input/misc/ims-pcu.c
> +++ b/drivers/input/misc/ims-pcu.c
[ ... ]
> @@ -1283,6 +1298,12 @@ static int ims_pcu_read_ofn_config(struct ims_pcu *pcu, u8 addr, u8 *data)
>  	if (error)
>  		return error;
>  
> +	if (pcu->cmd_buf_len < OFN_REG_RESULT_OFFSET + 2 + 1) {
> +		dev_err(pcu->dev, "Short OFN_GET_CONFIG response: %d bytes\n",
> +			pcu->cmd_buf_len);
> +		return -EIO;
> +	}
> +
>  	result = (s16)get_unaligned_le16(pcu->cmd_buf + OFN_REG_RESULT_OFFSET);
>  	if (result < 0)
>  		return -EIO;

[Severity: High]
This is a pre-existing issue, but while this change fixes out-of-bounds reads
in several places, it seems to miss a few other functions that still read from
pcu->cmd_buf without verifying pcu->cmd_buf_len.

For example, in ims_pcu_write_ofn_config(), result is read unconditionally:

static int ims_pcu_write_ofn_config(struct ims_pcu *pcu, u8 addr, u8 data)
{
	...
	error = ims_pcu_execute_command(pcu, OFN_SET_CONFIG,
					&buffer, sizeof(buffer));
	if (error)
		return error;

	result = (s16)get_unaligned_le16(pcu->cmd_buf + OFN_REG_RESULT_OFFSET);
	...
}

Similarly, ims_pcu_backlight_get_brightness() accesses the buffer without
bounds checks:

static enum led_brightness
ims_pcu_backlight_get_brightness(struct led_classdev *cdev)
{
	...
	error = ims_pcu_execute_query(pcu, GET_BRIGHTNESS);
	if (error) {
		/* Assume the LED is OFF */
		brightness = LED_OFF;
	} else {
		brightness =
			get_unaligned_le16(&pcu->cmd_buf[IMS_PCU_DATA_OFFSET]);
	}
	...
}

In __ims_pcu_execute_bl_command(), pcu->cmd_buf[2] is accessed directly:

static int __ims_pcu_execute_bl_command(...)
{
	...
	error = __ims_pcu_execute_command(pcu, ...);
	if (error) {
		...
		return error;
	}

	if (expected_response && pcu->cmd_buf[2] != expected_response) {
	...
}

And ims_pcu_verify_block() casts into the buffer without length verification:

static int ims_pcu_verify_block(...)
{
	...
	error = ims_pcu_execute_bl_command(pcu, READ_APP, NULL, 5,
					IMS_PCU_CMD_RESPONSE_TIMEOUT);
	if (error) {
		...
		return error;
	}

	fragment = (void *)&pcu->cmd_buf[IMS_PCU_BL_DATA_OFFSET];
	if (get_unaligned_le32(&fragment->addr) != addr ||
	    fragment->len != len) {
	...
}

If a malfunctioning USB device sends short packets during these operations,
could the driver bypass the intended length checks and read stale data from
previous packets?

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

[PATCH 11/11] Input: ims-pcu - fix potential infinite loop in CDC union descriptor parsing

[Dmitry Torokhov]

The driver parses CDC union descriptors in ims_pcu_get_cdc_union_desc()
by iterating through the extra descriptor data. However, it does not
verify that the bLength of each descriptor is at least 2. A malicious
device could provide a descriptor with bLength = 0, leading to an
infinite loop in the driver.

Add a check to ensure bLength is at least 2 before proceeding with
parsing.

Fixes: 628329d52474 (Input: add IMS Passenger Control Unit driver)
Cc: stable@vger.kernel.org
Assisted-by: Gemini:gemini-3.1-pro
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/misc/ims-pcu.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/drivers/input/misc/ims-pcu.c b/drivers/input/misc/ims-pcu.c
index 422b1be62303..a04dd3ea3a48 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;
 		}
 
-- 
2.54.0.746.g67dd491aae-goog

Re: [PATCH 11/11] Input: ims-pcu - fix potential infinite loop in CDC union descriptor parsing

[sashiko-bot]

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

Re: [PATCH 01/11] Input: ims-pcu - release data interface on disconnect

[sashiko-bot]

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

Pre-existing issues:
- [High] Incorrect size parameter used when freeing the IN endpoint DMA buffer.
- [High] Use-After-Free race condition in sysfs handlers due to incorrect teardown order of the data interface.
--

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

Input: ims-pcu - release data interface on disconnect
    
During probe the driver claims the data interface, but it never releases
it. Release it in disconnect to avoid leaving it permanently claimed.

> diff --git a/drivers/input/misc/ims-pcu.c b/drivers/input/misc/ims-pcu.c
> index 4c022a36dbe84..89737ef381c1c 100644
> --- a/drivers/input/misc/ims-pcu.c
> +++ b/drivers/input/misc/ims-pcu.c
> @@ -2090,6 +2090,7 @@ static void ims_pcu_disconnect(struct usb_interface *intf)
>  		ims_pcu_destroy_application_mode(pcu);
>  
>  	ims_pcu_buffers_free(pcu);

[Severity: High]
This is a pre-existing issue, but are we using the correct size parameter
when freeing the IN endpoint DMA buffer?

Looking at ims_pcu_buffers_free():

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);
	...
}

It appears to free pcu->urb_in_buf using pcu->max_out_size instead of
pcu->max_in_size.

If a malicious USB device presents endpoints with different max packet sizes,
could passing a mismatched size to the DMA freeing function cause memory
corruption or leaks?

> +	usb_driver_release_interface(&ims_pcu_driver, pcu->data_intf);

[Severity: High]
This is a pre-existing issue, but could releasing the data interface after
freeing the buffers create a use-after-free race condition?

During probe, the driver claims the data interface, which automatically
creates sysfs attributes on it. If a concurrent sysfs operation accesses a
file on the data interface during disconnect, it might retrieve the
still-valid pcu pointer and attempt to use pcu->urb_out_buf to send a command.

Since ims_pcu_buffers_free() has already freed the buffers, could this result
in a use-after-free? Would it be safer to release the interface before
freeing the buffers?

>  	kfree(pcu);
>  }

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