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

[sashiko-bot@kernel.org]

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