Re: [PATCH 16/36] Input: remove conditional return with no effect

[sashiko-bot@kernel.org]

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

New issues:
- [Low] The patch transforms an explicit error handling path into a direct return of an action function in a function with multiple failure points, violating input subsystem guidelines.

Pre-existing issues:
- [High] Stack/Heap buffer overflow in `smb_block_read()` due to unsafe buffer assumptions.
--

--- Patch [16]: [PATCH 16/36] Input: remove conditional return with no effect ---
commit 7d8880d335a34689e5a8927e5f647f213829163d
Author: Sang-Heon Jeon <ekffu200098@gmail.com>

Input: remove conditional return with no effect

This commit removes conditional returns where both branches return the same
value across multiple input drivers, applying a Coccinelle script
(cond_return_no_effect.cocci) to simplify the code.

[ ... ]

> diff --git a/drivers/input/mouse/synaptics_i2c.c b/drivers/input/mouse/synaptics_i2c.c
> index d4cf982f12632..85e37e2ce45ba 100644
> --- a/drivers/input/mouse/synaptics_i2c.c
> +++ b/drivers/input/mouse/synaptics_i2c.c
> @@ -257,11 +257,7 @@ static s32 synaptics_i2c_reg_set(struct i2c_client *client, u16 reg, u8 val)
>  	if (error)
>  		return error;
>  
> -	error = i2c_smbus_write_byte_data(client, reg & 0xff, val);
> -	if (error)
> -		return error;
> -
> -	return error;
> +	return i2c_smbus_write_byte_data(client, reg & 0xff, val);

[Severity: Low]
Does this violate the input subsystem maintainer preference to use explicit
failure paths? The guidelines suggest avoiding "return action(...);" for
functions with multiple failure points, like synaptics_i2c_reg_set() which
already has an earlier failure check.

[ ... ]

> 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 does ignoring the len parameter and passing
buf directly to i2c_smbus_read_block_data() cause a stack buffer overflow?

For instance, rmi_scan_pdt_page() in rmi_driver.c passes a 6-byte stack
buffer to rmi_read_block():

drivers/input/rmi4/rmi_driver.c:rmi_scan_pdt_page() {
    ...
    u8 buf[RMI_PDT_ENTRY_SIZE];
    ...
    error = rmi_read_block(rmi_dev, pdt_address, buf, RMI_PDT_ENTRY_SIZE);
    ...
}

If a malfunctioning or malicious I2C device returns up to 32 bytes, wouldn't
i2c_smbus_read_block_data() copy those 32 bytes directly into the 6-byte
stack buffer?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260723184538.3888637-1-ekffu200098@gmail.com?part=16