Re: [PATCH v2 2/3] Input: rmi_smbus - remove conditional return with no effect

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

Pre-existing issues:
- [High] Buffer overflow in smb_block_read() due to ignoring the requested length.
- [High] Out-of-bounds read in rmi_smb_write_block() due to using the total length instead of the remaining length.
--

--- Patch [2]: [PATCH v2 2/3] Input: rmi_smbus - remove conditional return with no effect ---
commit c43fcefd3c43ed84b5f32f45bd591ace30890382
Author: Sang-Heon Jeon <[email protected]>

Input: rmi_smbus - remove conditional return with no effect
    
This patch simplifies the return path in smb_block_read() by removing an
unnecessary conditional check. Both branches of the check returned the same
value, so it now returns the result of i2c_smbus_read_block_data() directly.

> diff --git a/drivers/input/rmi4/rmi_smbus.c b/drivers/input/rmi4/rmi_smbus.c
> index 6de68c602558b..3160714a514a5 100644
> --- a/drivers/input/rmi4/rmi_smbus.c
> +++ b/drivers/input/rmi4/rmi_smbus.c
> @@ -177,13 +177,8 @@ static int smb_block_read(struct rmi_transport_dev *xport,
>  	struct rmi_smb_xport *rmi_smb =
>  		container_of(xport, struct rmi_smb_xport, xport);
>  	struct i2c_client *client = rmi_smb->client;
> -	int retval;
>  
> -	retval = i2c_smbus_read_block_data(client, commandcode, buf);
> -	if (retval < 0)
> -		return retval;
> -
> -	return retval;
> +	return i2c_smbus_read_block_data(client, commandcode, buf);
>  }

[Severity: High]
This is a pre-existing issue, but by removing the intermediate check, it draws
attention to a buffer overflow in this function.

Can this corrupt memory? The smb_block_read() function takes a len parameter,
but it is completely ignored when calling i2c_smbus_read_block_data().

If a malicious or buggy I2C device returns up to 32 bytes (SMB_MAX_COUNT),
i2c_smbus_read_block_data() will copy that full amount into buf. If the
original len was less than 32 (for instance, on the final chunk of a read in
rmi_smb_read_block()), this could overflow the buffer and corrupt memory.

>  
>  static int rmi_smb_read_block(struct rmi_transport_dev *xport, u16 rmiaddr,
>  			      void *databuff, size_t len)
>  {

[ ... ]

While reviewing this, I also noticed another pre-existing issue in the nearby
rmi_smb_write_block() function:

drivers/input/rmi4/rmi_smbus.c:rmi_smb_write_block() {
    ...
	while (cur_len > 0) {
		/*
		 * break into 32 bytes chunks to write get command code
		 */
		int block_len = min_t(int, len, SMB_MAX_COUNT);
    ...
}

[Severity: High]
This is a pre-existing issue, but can this lead to an out-of-bounds read?

When chunking the write, the loop calculates block_len using the original total
len instead of the remaining cur_len.

If len is greater than 32, block_len will remain exactly 32 for every
iteration, even the final one where fewer than 32 bytes remain. This causes
smb_block_write() to read 32 bytes starting from the advanced databuff
pointer, potentially reading past the end of the original buffer.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2