[PATCH v2 0/3] Input: clean up conditional returns with no effect

[PATCH v2 0/3] Input: clean up conditional returns with no effect

[Sang-Heon Jeon]

Hello,

This series cleans up conditional returns where both branches return
the same value, so the check has no effect. The Input patch of the
treewide v1 series [1] is split per driver and posted separately.

Patches 1-2 are generated by the Coccinelle script, which you can
find in v1.

Patch 3 keeps the check in synaptics_i2c_reg_set() and returns 0
explicitly instead, which is the preferred way when there are
multiple failure points.

---
Changes from v1 [1]
- Split patches per driver
- Change base to input/next
- Filter only Input patch
- Return 0 instead of removing the check, as suggested by Dmitry

[1] https://lore.kernel.org/all/20260723184538.3888637-1-ekffu200098@gmail.com/
---

Sang-Heon Jeon (3):
  Input: pmic8xxx-keypad - remove conditional return with no effect
  Input: rmi_smbus - remove conditional return with no effect
  Input: synaptics_i2c - return 0 explicitly on success

 drivers/input/keyboard/pmic8xxx-keypad.c | 8 +-------
 drivers/input/mouse/synaptics_i2c.c      | 2 +-
 drivers/input/rmi4/rmi_smbus.c           | 7 +------
 3 files changed, 3 insertions(+), 14 deletions(-)

-- 
2.43.0

[PATCH v2 1/3] Input: pmic8xxx-keypad - remove conditional return with no effect

[Sang-Heon Jeon]

Both branches of the check return the same value, so the check has no
effect. Remove it and return the value directly.

This is the result of running the Coccinelle script from
scripts/coccinelle/misc/cond_return_no_effect.cocci.

Signed-off-by: Sang-Heon Jeon <ekffu200098@gmail.com>
---
 drivers/input/keyboard/pmic8xxx-keypad.c | 8 +-------
 1 file changed, 1 insertion(+), 7 deletions(-)

diff --git a/drivers/input/keyboard/pmic8xxx-keypad.c b/drivers/input/keyboard/pmic8xxx-keypad.c
index 35d1aa2a22a5..c916e80e2f58 100644
--- a/drivers/input/keyboard/pmic8xxx-keypad.c
+++ b/drivers/input/keyboard/pmic8xxx-keypad.c
@@ -462,15 +462,9 @@ static int pmic8xxx_kp_enable(struct pmic8xxx_kp *kp)
 
 static int pmic8xxx_kp_disable(struct pmic8xxx_kp *kp)
 {
-	int rc;
-
 	kp->ctrl_reg &= ~KEYP_CTRL_KEYP_EN;
 
-	rc = regmap_write(kp->regmap, KEYP_CTRL, kp->ctrl_reg);
-	if (rc < 0)
-		return rc;
-
-	return rc;
+	return regmap_write(kp->regmap, KEYP_CTRL, kp->ctrl_reg);
 }
 
 static int pmic8xxx_kp_open(struct input_dev *dev)
-- 
2.43.0

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

[Sang-Heon Jeon]

Both branches of the check return the same value, so the check has no
effect. Remove it and return the value directly.

This is the result of running the Coccinelle script from
scripts/coccinelle/misc/cond_return_no_effect.cocci.

Signed-off-by: Sang-Heon Jeon <ekffu200098@gmail.com>
---
 drivers/input/rmi4/rmi_smbus.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/drivers/input/rmi4/rmi_smbus.c b/drivers/input/rmi4/rmi_smbus.c
index 6de68c602558..3160714a514a 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);
 }
 
 static int rmi_smb_read_block(struct rmi_transport_dev *xport, u16 rmiaddr,
-- 
2.43.0

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

[sashiko-bot]

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 <ekffu200098@gmail.com>

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/20260729171001.260698-1-ekffu200098@gmail.com?part=2

[PATCH v2 3/3] Input: synaptics_i2c - return 0 explicitly on success

[Sang-Heon Jeon]

error is always zero at the last return in synaptics_i2c_reg_set().
Explicitly return 0 on the success path instead of returning error,
which is the preferred way when there are multiple failure points.

No functional change.

Signed-off-by: Sang-Heon Jeon <ekffu200098@gmail.com>
---
 drivers/input/mouse/synaptics_i2c.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/input/mouse/synaptics_i2c.c b/drivers/input/mouse/synaptics_i2c.c
index d4cf982f1263..66e833974c6d 100644
--- a/drivers/input/mouse/synaptics_i2c.c
+++ b/drivers/input/mouse/synaptics_i2c.c
@@ -261,7 +261,7 @@ static s32 synaptics_i2c_reg_set(struct i2c_client *client, u16 reg, u8 val)
 	if (error)
 		return error;
 
-	return error;
+	return 0;
 }
 
 static s32 synaptics_i2c_word_get(struct i2c_client *client, u16 reg)
-- 
2.43.0

Re: [PATCH v2 3/3] Input: synaptics_i2c - return 0 explicitly on success

[sashiko-bot]

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

Pre-existing issues:
- [High] The delayed work `touch->dwork` is not cancelled on driver unbind or probe failure, leading to a use-after-free.
--

--- Patch [3]: [PATCH v2 3/3] Input: synaptics_i2c - return 0 explicitly on success ---
commit 2cc2d0ff5c54be0c06b9f9251e169b1467e86f9b
Author: Sang-Heon Jeon <ekffu200098@gmail.com>

Input: synaptics_i2c - return 0 explicitly on success

error is always zero at the last return in synaptics_i2c_reg_set().
Explicitly return 0 on the success path instead of returning error,
which is the preferred way when there are multiple failure points.

> diff --git a/drivers/input/mouse/synaptics_i2c.c b/drivers/input/mouse/synaptics_i2c.c
> index d4cf982f12632..66e833974c6da 100644
> --- a/drivers/input/mouse/synaptics_i2c.c
> +++ b/drivers/input/mouse/synaptics_i2c.c
> @@ -261,7 +261,7 @@ static s32 synaptics_i2c_reg_set(struct i2c_client *client, u16 reg, u8 val)
>  	if (error)
>  		return error;
>  
> -	return error;
> +	return 0;
>  }
>  
>  static s32 synaptics_i2c_word_get(struct i2c_client *client, u16 reg)

[Severity: High]
This is a pre-existing issue, but I noticed that the delayed work touch->dwork
doesn't appear to be cancelled on driver unbind or probe failure.

In synaptics_i2c_probe(), interrupts are enabled and devm_request_irq() is
called before input_register_device(). The IRQ handler can immediately schedule
touch->dwork.

If input_register_device() fails:

drivers/input/mouse/synaptics_i2c.c:synaptics_i2c_probe() {
	...
	error = input_register_device(touch->input);
	if (error) {
		dev_err(dev, "Input device register failed: %d\n", error);
		return error;
	}
	...
}

The probe function returns the error directly. Since devres will automatically
free the IRQ and the touch structure, but the pending work on system_dfl_wq is
never cancelled, could the delayed work run later and access the freed touch
memory?

Furthermore, the driver lacks a remove callback entirely:

drivers/input/mouse/synaptics_i2c.c:
static struct i2c_driver synaptics_i2c_driver = {
	.driver = {
		.name	= DRIVER_NAME,
		.of_match_table = of_match_ptr(synaptics_i2c_of_match),
		.pm	= pm_sleep_ptr(&synaptics_i2c_pm),
	},

	.probe		= synaptics_i2c_probe,
	.id_table	= synaptics_i2c_id_table,
};

If the device is unbound via sysfs, devres will automatically tear down
resources. However, the delayed work is only explicitly cancelled if the input
device is currently open (via synaptics_i2c_close()).

If the device was never opened before it is unbound, does this mean the work
is never cancelled before the memory is freed?

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