Re: [PATCH v3] Input: ims-pcu - fix heap-buffer-overflow in ims_pcu_process_data()

[Dmitry Torokhov <dmitry.torokhov@gmail.com>]

Hi,

On Sun, Dec 21, 2025 at 04:14:42PM -0500, pip-izony wrote:
> From: Seungjin Bae <eeodqql09@gmail.com>
> 
> The `ims_pcu_process_data()` processes incoming URB data byte by byte.
> However, it fails to check if the `read_pos` index exceeds
> IMS_PCU_BUF_SIZE.
> 
> If a malicious USB device sends a packet larger than IMS_PCU_BUF_SIZE,
> `read_pos` will increment indefinitely. Moreover, since `read_pos` is
> located immediately after `read_buf`, the attacker can overwrite
> `read_pos` itself to arbitrarily control the index.
> 
> This manipulated `read_pos` is subsequently used in
> `ims_pcu_handle_response()` to copy data into `cmd_buf`, leading to a
> heap buffer overflow.
> 
> Specifically, an attacker can overwrite the `cmd_done.wait.head` located
> at offset 136 relative to `cmd_buf` in the `ims_pcu_handle_response()`.
> Consequently, when the driver calls `complete(&pcu->cmd_done)`, it
> triggers a control flow hijack by using the manipulated pointer.
> 
> Fix this by adding a bounds check for `read_pos` before writing to
> `read_buf`. If the packet is too long, discard it, log a warning,
> and reset the parser state.
> 
> Fixes: 628329d524743 ("Input: add IMS Passenger Control Unit driver")
> Co-developed-by: Sanghoon Choi <csh0052@gmail.com>
> Signed-off-by: Sanghoon Choi <csh0052@gmail.com>
> Signed-off-by: Seungjin Bae <eeodqql09@gmail.com>
> ---
>  v1 -> v2: Add warning and reset the state of the parser for bad packet
>  v2 -> v3: Add co-author information
> 
>  drivers/input/misc/ims-pcu.c | 20 ++++++++++++++++++++
>  1 file changed, 20 insertions(+)
> 
> diff --git a/drivers/input/misc/ims-pcu.c b/drivers/input/misc/ims-pcu.c
> index 4581f1c53644..c98ef71c841e 100644
> --- a/drivers/input/misc/ims-pcu.c
> +++ b/drivers/input/misc/ims-pcu.c
> @@ -450,6 +450,16 @@ static void ims_pcu_process_data(struct ims_pcu *pcu, struct urb *urb)
>  			continue;
>  
>  		if (pcu->have_dle) {
> +			if (pcu->read_pos >= IMS_PCU_BUF_SIZE) {
> +				dev_warn(pcu->dev,
> +					 "Packet too long (%d bytes), discarding\n",
> +					 pcu->read_pos);
> +				pcu->have_stx = false;
> +				pcu->have_dle = false;
> +				pcu->read_pos = 0;

Here and in the other place we also need to reset checksum, otherwise a
valid packet might get rejected.

I factored out the code resetting packet state and applied, thanks.

-- 
Dmitry