[PATCH] Input: cyapa - bound the SMBus block reads to the caller buffer

[PATCH] Input: cyapa - bound the SMBus block reads to the caller buffer

[Bryam Vargas via B4 Relay]

From: Bryam Vargas <hexlabsecurity@proton.me>

cyapa_smbus_read_block() takes the expected length of the result but
never uses it to bound the destination:

	if (SMBUS_GROUP_BLOCK_CMD_MASK & cmd) {
		smbus_cmd = SMBUS_ENCODE_RW(cmd, SMBUS_READ);
		ret = i2c_smbus_read_block_data(client, smbus_cmd, values);
		goto out;
	}
	...
		buf = values + I2C_SMBUS_BLOCK_MAX * index;
		ret = i2c_smbus_read_block_data(client, smbus_cmd, buf);

i2c_smbus_read_block_data() has no destination-size argument; it copies
the block count reported by the device (the first SMBus byte, up to
I2C_SMBUS_BLOCK_MAX = 32) into the buffer. Several callers pass buffers
smaller than 32 bytes - cyapa_detect() reads CYAPA_CMD_BL_STATUS into an
on-stack u8 status[BL_STATUS_SIZE] (3 bytes), and the group-query path
reads into u8 query_data[QUERY_DATA_SIZE] (27 bytes) - so a
malfunctioning, malicious or counterfeit trackpad (or an attacker
tampering with the SMBus) that reports a larger block count overflows
the caller's stack buffer by up to 32 - 3 = 29 bytes, clobbering the
stack canary, saved registers and the return address.

Read each block into a local I2C_SMBUS_BLOCK_MAX-sized buffer and copy
back only the expected number of bytes, in both the group-block branch
and the multi-block loop, so the device can never write past the
caller's buffer.

Fixes: 6ddaf744c9f6 ("Input: cyapa - add support for smbus protocol")
Cc: stable@vger.kernel.org
Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
---
 drivers/input/mouse/cyapa_gen3.c | 16 +++++++++++++---
 1 file changed, 13 insertions(+), 3 deletions(-)

diff --git a/drivers/input/mouse/cyapa_gen3.c b/drivers/input/mouse/cyapa_gen3.c
index fc3fb954523b..832775bb7182 100644
--- a/drivers/input/mouse/cyapa_gen3.c
+++ b/drivers/input/mouse/cyapa_gen3.c
@@ -247,27 +247,37 @@ ssize_t cyapa_smbus_read_block(struct cyapa *cyapa, u8 cmd, size_t len,
 	ssize_t ret;
 	u8 index;
 	u8 smbus_cmd;
-	u8 *buf;
+	u8 buf[I2C_SMBUS_BLOCK_MAX];
 	struct i2c_client *client = cyapa->client;
 
 	if (!(SMBUS_BYTE_BLOCK_CMD_MASK & cmd))
 		return -EINVAL;
 
+	/*
+	 * i2c_smbus_read_block_data() copies the device-reported block count
+	 * (up to I2C_SMBUS_BLOCK_MAX) into its buffer and has no way to know
+	 * its size, so read into a local buffer and copy back at most the
+	 * expected number of bytes - never past the caller's buffer.
+	 */
 	if (SMBUS_GROUP_BLOCK_CMD_MASK & cmd) {
 		/* 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, buf);
+		if (ret > 0)
+			memcpy(values, buf, min_t(size_t, ret, len));
 		goto out;
 	}
 
 	ret = 0;
 	for (index = 0; index * I2C_SMBUS_BLOCK_MAX < len; index++) {
+		size_t offset = index * 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);
 		if (ret < 0)
 			goto out;
+		memcpy(values + offset, buf, min_t(size_t, ret, len - offset));
 	}
 
 out:

---
base-commit: 8e65320d91cdc3b241d4b94855c88459b91abf66
change-id: 20260613-b4-disp-8e550272-a1a447c54089

Best regards,
-- 
Bryam Vargas <hexlabsecurity@proton.me>

Re: [PATCH] Input: cyapa - bound the SMBus block reads to the caller buffer

[sashiko-bot]

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

Pre-existing issues:
- [High] cyapa_smbus_read_block masks short reads by unconditionally returning the expected length, causing callers to process and leak uninitialized stack memory.
- [High] Explicit `input_unregister_device()` on a `devm` allocated input device causes double-unregistration and memory leak.
--

commit 3fd76440d016e0f73dbb99ea7474006c10fb3123
Author: Bryam Vargas <hexlabsecurity@proton.me>

Input: cyapa - bound the SMBus block reads to the caller buffer

This commit prevents a potential buffer overflow in the cyapa driver's
SMBus block read path. It introduces a local buffer to ensure that the
number of bytes copied back does not exceed the size of the destination
buffer provided by the caller.

> diff --git a/drivers/input/mouse/cyapa_gen3.c b/drivers/input/mouse/cyapa_gen3.c
> index fc3fb954523b7..832775bb71825 100644
> --- a/drivers/input/mouse/cyapa_gen3.c
> +++ b/drivers/input/mouse/cyapa_gen3.c

[ ... ]

> @@ -247,27 +247,37 @@ ssize_t cyapa_smbus_read_block(struct cyapa *cyapa, u8 cmd, size_t len,
>  	ssize_t ret;
>  	u8 index;
>  	u8 smbus_cmd;
> -	u8 *buf;
> +	u8 buf[I2C_SMBUS_BLOCK_MAX];
>  	struct i2c_client *client = cyapa->client;
>  
>  	if (!(SMBUS_BYTE_BLOCK_CMD_MASK & cmd))
>  		return -EINVAL;
>  
> +	/*
> +	 * i2c_smbus_read_block_data() copies the device-reported block count
> +	 * (up to I2C_SMBUS_BLOCK_MAX) into its buffer and has no way to know
> +	 * its size, so read into a local buffer and copy back at most the
> +	 * expected number of bytes - never past the caller's buffer.
> +	 */
>  	if (SMBUS_GROUP_BLOCK_CMD_MASK & cmd) {
>  		/* 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, buf);
> +		if (ret > 0)
> +			memcpy(values, buf, min_t(size_t, ret, len));
>  		goto out;
>  	}
>  
>  	ret = 0;
>  	for (index = 0; index * I2C_SMBUS_BLOCK_MAX < len; index++) {
> +		size_t offset = index * 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);
>  		if (ret < 0)
>  			goto out;
> +		memcpy(values + offset, buf, min_t(size_t, ret, len - offset));
>  	}
>  
>  out:

[Severity: High]
This isn't a bug introduced by this patch, but does returning len in
cyapa_smbus_read_block() mask a short read if ret < len?

The function returns the requested length even if fewer bytes were read:

cyapa_smbus_read_block() {
    ...
out:
	return ret > 0 ? len : ret;
}

Callers seem to assume their buffers are fully populated if the return value
matches the requested length. For instance, in cyapa_gen3_get_query_data():

	u8 query_data[QUERY_DATA_SIZE];
	int ret;

	if (cyapa->state != CYAPA_STATE_OP)
		return -EBUSY;

	ret = cyapa_read_block(cyapa, CYAPA_CMD_GROUP_QUERY, query_data);
	if (ret != QUERY_DATA_SIZE)
		return (ret < 0) ? ret : -EIO;

	memcpy(&cyapa->product_id[0], &query_data[0], 5);

Could this lead to processing uninitialized stack memory and exposing it
to userspace if the read is short?


[Severity: High]
This is also a pre-existing issue, but does calling input_unregister_device()
on a devm-allocated device in cyapa_update_fw_store() cause a double-free or
resource leak?

Looking at cyapa_update_fw_store() in drivers/input/mouse/cyapa.c:

	if (cyapa->input) {
		/* ... */
		input_unregister_device(cyapa->input);
		cyapa->input = NULL;
	}

Since the device was originally allocated with devm_input_allocate_device()
in cyapa_create_input_dev(), manually unregistering it leaves the devres
cleanup action still queued.

Could the devres action trigger a double-unregistration when the device
unbinds, and does allocating a new device via cyapa_reinitialize() during
each firmware update leak the old devres actions?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260613-b4-disp-8e550272-v1-1-9658cf8292c2@proton.me?part=1