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

[sashiko-bot@kernel.org]

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