Re: [PATCH 2/2] Input: cyapa - clamp SMBus block reads to the caller's buffer

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Input: cyapa - cyapa_smbus_read_block() returns full len on short block reads
--

commit 17f2cef41b86752525f835330272cc728242a63c
Author: Chengyu Liu <lui2012qwq@gmail.com>

Input: cyapa - clamp SMBus block reads to the caller's buffer

This commit bounces SMBus block reads in cyapa_smbus_read_block() through
a temporary I2C_SMBUS_BLOCK_MAX buffer and clamps the copy size to the
caller's requested length to prevent buffer overruns.

> diff --git a/drivers/input/mouse/cyapa_gen3.c b/drivers/input/mouse/cyapa_gen3.c
> index fc3fb954523b7..cd80ae51f4e52 100644
> --- a/drivers/input/mouse/cyapa_gen3.c
> +++ b/drivers/input/mouse/cyapa_gen3.c
> @@ -254,20 +254,30 @@ ssize_t cyapa_smbus_read_block(struct cyapa *cyapa, u8 cmd, size_t len,
>  		return -EINVAL;
>  
>  	if (SMBUS_GROUP_BLOCK_CMD_MASK & cmd) {
> +		u8 tmp[I2C_SMBUS_BLOCK_MAX];
> +
>  		/* read specific block registers command. */
>  		smbus_cmd = SMBUS_ENCODE_RW(cmd, SMBUS_READ);
> -		ret = i2c_smbus_read_block_data(client, smbus_cmd, values);
> +		ret = i2c_smbus_read_block_data(client, smbus_cmd, tmp);
> +		if (ret > 0)
> +			/* never trust the device-provided count */
> +			memcpy(values, tmp, min_t(size_t, ret, len));
>  		goto out;
>  	}
>  
>  	ret = 0;
>  	for (index = 0; index * I2C_SMBUS_BLOCK_MAX < len; index++) {
> +		u8 tmp[I2C_SMBUS_BLOCK_MAX];
> +		size_t chunk = min_t(size_t, len - index * I2C_SMBUS_BLOCK_MAX,
> +				     I2C_SMBUS_BLOCK_MAX);
> +
>  		smbus_cmd = SMBUS_ENCODE_IDX(cmd, index);
>  		smbus_cmd = SMBUS_ENCODE_RW(smbus_cmd, SMBUS_READ);
>  		buf = values + I2C_SMBUS_BLOCK_MAX * index;
> -		ret = i2c_smbus_read_block_data(client, smbus_cmd, buf);
> +		ret = i2c_smbus_read_block_data(client, smbus_cmd, tmp);
>  		if (ret < 0)
>  			goto out;
> +		memcpy(buf, tmp, min_t(size_t, ret, chunk));
>  	}

[Severity: High]
What happens in cyapa_smbus_read_block() if the device returns a short block
count where 0 < ret < len (or 0 <= ret < chunk in the indexed loop)?

Both branches copy only ret bytes into values or buf, leaving the remainder
of the caller's buffer untouched, and then jump to out:

drivers/input/mouse/cyapa_gen3.c:cyapa_smbus_read_block() {
	...
out:
	return ret > 0 ? len : ret;
}

Because cyapa_smbus_read_block() still returns the full requested len whenever
the last i2c_smbus_read_block_data() call returned a positive count, callers
that pass uninitialized stack buffers and check for a full read cannot detect
the short read:

drivers/input/mouse/cyapa_gen3.c:cyapa_gen3_get_query_data() {
	u8 query_data[QUERY_DATA_SIZE];
	int ret;

	/* query dataabda has 27 bytes */
	ret = cyapa_read_block(cyapa, CYAPA_CMD_GROUP_QUERY, query_data);
	if (ret != QUERY_DATA_SIZE)
		return (ret < 0) ? ret : -EIO;
	...
}

drivers/input/mouse/cyapa_gen3.c:cyapa_gen3_irq_handler() {
	struct cyapa_reg_data data;
	int ret;

	ret = cyapa_read_block(cyapa, CYAPA_CMD_GROUP_DATA, (u8 *)&data);
	if (ret != sizeof(data)) {
		dev_err(dev, "failed to read report data, (%d)\n", ret);
		return -EINVAL;
	}
	...
}

Wouldn't a device returning fewer bytes than requested cause
cyapa_gen3_get_query_data(), cyapa_gen3_irq_handler(),
cyapa_gen3_try_poll_handler(), and cyapa_get_state() to consume uninitialized
stack memory in query_data, data, and status (and expose uninitialized bytes
from query_data via the product_id and firmware_version sysfs attributes)?
Should cyapa_smbus_read_block() return an error such as -EIO when ret < len
in the group-read branch or ret < chunk in the indexed loop?

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