[PATCH 1/2] Input: tca8418_keypad - Add devm cleanup to disable interrupts on unload

[PATCH 1/2] Input: tca8418_keypad - Add devm cleanup to disable interrupts on unload

[Zhian Liang]

tca8418_configure() enables TCA8418 interrupts by writing the CFG
register.  If input_register_device() fails afterwards, probe returns
an error and devres releases the IRQ handler, but the hardware is
left with interrupts still enabled.

Fix it by registering a devm action that clears the CFG register
 when the device is released, disabling
all interrupts on both probe failure and driver unbind.

Signed-off-by: Zhian Liang <liangzhan5dev@gmail.com>
---
 drivers/input/keyboard/tca8418_keypad.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/input/keyboard/tca8418_keypad.c b/drivers/input/keyboard/tca8418_keypad.c
index 36d4a7c38bb1..9f0aace6bd21 100644
--- a/drivers/input/keyboard/tca8418_keypad.c
+++ b/drivers/input/keyboard/tca8418_keypad.c
@@ -270,6 +270,20 @@ static int tca8418_configure(struct tca8418_keypad *keypad_data,
 	return error;
 }
 
+static void tca8418_disable_hw(void *data)
+{
+	struct tca8418_keypad *keypad_data = data;
+	int error;
+
+	error = tca84818_write_byte(keypad_data, REG_CFG, 0);
+	if (error)
+		dev_warn(&keypad_data->client->dev, "unable to disable interrupts: %d\n", error);
+
+	error = tca8418_write_byte(keypad_data, REG_INT_STAT, 0xff);
+	if (error)
+		dev_warn(&keypad_data->client->dev, "unable to clear interrupt status: %d\n", error);
+}
+
 static int tca8418_keypad_probe(struct i2c_client *client)
 {
 	struct device *dev = &client->dev;
@@ -356,6 +370,10 @@ static int tca8418_keypad_probe(struct i2c_client *client)
 	if (error < 0)
 		return error;
 
+	error = devm_add_action_or_reset(dev, tca8418_diable_hw, keypad_data);
+	if (error)
+		return error;
+
 	error = input_register_device(input);
 	if (error) {
 		dev_err(dev, "Unable to register input device, error: %d\n",
-- 
2.34.1

[PATCH 2/2] Input: tca8418_keypad - fix race condition in interrupt handler

[Zhian Liang]

There is a race condition in tca8418_irq_handler() where a new key event
can be lost if it arrives after the FIFO is drained but before the
interrupt status register is cleared.

Fix this by re-reading INT_STAT after draining the FIFO and repeating
the process if K_INT is still asserted. This ensures the FIFO is truly
empty before clearing interrupts.

The FIFO depth is 10 events, so a loop limit of 16 provides sufficient
margin while preventing infinite loops in case of hardware misbehavior.

Signed-off-by: Zhian Liang <liangzhan5dev@gmail.com>
---
 drivers/input/keyboard/tca8418_keypad.c | 15 +++++++++++++--
 1 file changed, 13 insertions(+), 2 deletions(-)

diff --git a/drivers/input/keyboard/tca8418_keypad.c b/drivers/input/keyboard/tca8418_keypad.c
index 9f0aace6bd21..0952370c0636 100644
--- a/drivers/input/keyboard/tca8418_keypad.c
+++ b/drivers/input/keyboard/tca8418_keypad.c
@@ -212,6 +212,7 @@ static irqreturn_t tca8418_irq_handler(int irq, void *dev_id)
 	struct tca8418_keypad *keypad_data = dev_id;
 	u8 reg;
 	int error;
+	int max_loops = 16;
 
 	error = tca8418_read_byte(keypad_data, REG_INT_STAT, &reg);
 	if (error) {
@@ -225,9 +226,19 @@ static irqreturn_t tca8418_irq_handler(int irq, void *dev_id)
 
 	if (reg & INT_STAT_OVR_FLOW_INT)
 		dev_warn(&keypad_data->client->dev, "overflow occurred\n");
+	do {
+		if (reg & INT_STAT_K_INT)
+			tca8418_read_keypad(keypad_data);
+
+		/* Re-read interrupt status to check for new events */
+		error = tca8418_read_byte(keypad_data, REG_INT_STAT, &reg);
+		if (error) {
+			dev_err(&keypad_data->client->dev,
+				"unable to re-read REG_INT_STAT\n");
+			return IRQ_HANDLED;
+		}
 
-	if (reg & INT_STAT_K_INT)
-		tca8418_read_keypad(keypad_data);
+	} while ((reg & INT_STAT_K_INT) && --max_loops);
 
 	/* Clear all interrupts, even IRQs we didn't check (GPI, CAD, LCK) */
 	reg = 0xff;
-- 
2.34.1

Re: [PATCH 2/2] Input: tca8418_keypad - fix race condition in interrupt handler

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] The patch fails to resolve the lost-interrupt race condition and introduces a 16-iteration busy loop due to misunderstanding the Write-1-to-Clear (W1C) interrupt status register.
- [High] Returning IRQ_HANDLED on I2C read failure bypasses clearing the interrupt, which could lead to an IRQ storm.
- [Medium] The outer loop and its 16-iteration limit are redundant and based on a flawed assumption about the FIFO reading logic.
--

commit b07bd14b2796c804f03cf9efdf000891fe041cc5
Author: Zhian Liang <liangzhan5dev@gmail.com>

Input: tca8418_keypad - fix race condition in interrupt handler

This patch modifies the tca8418 interrupt handler to re-read the
interrupt status register and loop if the interrupt bit is still set.
This is intended to prevent lost key events when new data arrives
after the FIFO is drained but before the interrupt status is cleared.

> diff --git a/drivers/input/keyboard/tca8418_keypad.c b/drivers/input/keyboard/tca8418_keypad.c
> index d917de848a213..be700137b0fb7 100644
> --- a/drivers/input/keyboard/tca8418_keypad.c
> +++ b/drivers/input/keyboard/tca8418_keypad.c

[ ... ]

> @@ -214,9 +215,19 @@ static irqreturn_t tca8418_irq_handler(int irq, void *dev_id)
>  
>  	if (reg & INT_STAT_OVR_FLOW_INT)
>  		dev_warn(&keypad_data->client->dev, "overflow occurred\n");
> +	do {
> +		if (reg & INT_STAT_K_INT)
> +			tca8418_read_keypad(keypad_data);
> +
> +		/* Re-read interrupt status to check for new events */
> +		error = tca8418_read_byte(keypad_data, REG_INT_STAT, &reg);
> +		if (error) {
> +			dev_err(&keypad_data->client->dev,
> +				"unable to re-read REG_INT_STAT\n");
> +			return IRQ_HANDLED;

[Severity: High]
If the I2C read fails inside the new loop, returning IRQ_HANDLED bypasses the
interrupt clear step at the end of the function. Could this leave the interrupt
line asserted in hardware? If the interrupt is level-triggered or shared,
might this cause an IRQ storm because the kernel is told the interrupt was
handled while it remains asserted?

> +		}
>  
> -	if (reg & INT_STAT_K_INT)
> -		tca8418_read_keypad(keypad_data);
> +	} while ((reg & INT_STAT_K_INT) && --max_loops);

[Severity: High]
The REG_INT_STAT register is Write-1-to-Clear. Because reading it does not
clear the interrupt bit by itself, wouldn't reg & INT_STAT_K_INT evaluate to
true on every iteration, causing the handler to always spin for the full
16-iteration limit on every valid interrupt?

[Severity: Medium]
Is the max_loops limit of 16 necessary to drain the 10-event FIFO? Looking at
tca8418_read_keypad(), it has its own do/while loop that continuously reads
until the FIFO reports empty. It seems a single call fully drains the FIFO,
making the outer loop based on FIFO depth redundant.

>  
>  	/* Clear all interrupts, even IRQs we didn't check (GPI, CAD, LCK) */
>  	reg = 0xff;
>  	error = tca8418_write_byte(keypad_data, REG_INT_STAT, reg);

[Severity: High]
Does this unconditional write of 0xff clear any new interrupts that triggered
just after the loop finished? If a new event sets the interrupt flag between
the end of the loop and this write, it looks like it will be cleared without
reading the corresponding event. Could this permanently drop events and
preserve the race condition this patch aims to fix?

>  	if (error)
>  		dev_err(&keypad_data->client->dev,
>  			"unable to clear REG_INT_STAT\n");

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260903192942.21906-1-liangzhan5dev@gmail.com?part=2

Re: [PATCH 2/2] Input: tca8418_keypad - fix race condition in interrupt handler

[Zhian Liang]

You are right. Reading REG_INT_STAT does not clear any bits and IRQF_ONESHOT already provides protection. This patch is not necessary. I shall learn more next time.

Re: [PATCH 1/2] Input: tca8418_keypad - Add devm cleanup to disable interrupts on unload

[sashiko-bot]

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

New issues:
- [Low] The patch introduces two typos in function names that prevent the code from compiling due to undefined references.

Pre-existing issues:
- [High] An attacker controlling the I2C device can send a malicious key event code to trigger an out-of-bounds read on the `keymap` array, potentially causing a kernel panic or leaking memory.
--

commit b2a2e3b9a8117d6e6005216e2f4bd8fe76dd2fc6
Author: Zhian Liang <liangzhan5dev@gmail.com>

Input: tca8418_keypad - Add devm cleanup to disable interrupts on unload

This patch adds a devm action to disable interrupts on the TCA8418 device
upon probe failure or driver unbind. It clears the configuration register and
the interrupt status to ensure the hardware is left in a safe state.

> diff --git a/drivers/input/keyboard/tca8418_keypad.c b/drivers/input/keyboard/tca8418_keypad.c
> index 4616afa0286c3..d917de848a213 100644
> --- a/drivers/input/keyboard/tca8418_keypad.c
> +++ b/drivers/input/keyboard/tca8418_keypad.c
> @@ -260,6 +260,20 @@ static int tca8418_configure(struct tca8418_keypad *keypad_data,
>  	return error;
>  }
>  
> +static void tca8418_disable_hw(void *data)
> +{
> +	struct tca8418_keypad *keypad_data = data;
> +	int error;
> +
> +	error = tca84818_write_byte(keypad_data, REG_CFG, 0);
[Severity: Low]
Is there a typo in the function name here? It looks like an extra '8' was
added to tca8418_write_byte, which will cause an undefined reference error
during compilation.

> +	if (error)
> +		dev_warn(&keypad_data->client->dev, "unable to disable interrupts: %d\n", error);
> +
> +	error = tca8418_write_byte(keypad_data, REG_INT_STAT, 0xff);
> +	if (error)
> +		dev_warn(&keypad_data->client->dev, "unable to clear interrupt status: %d\n", error);
> +}
> +
>  static int tca8418_keypad_probe(struct i2c_client *client)
>  {
>  	struct device *dev = &client->dev;
> @@ -344,6 +358,10 @@ static int tca8418_keypad_probe(struct i2c_client *client)
>  	if (error < 0)
>  		return error;
>  
> +	error = devm_add_action_or_reset(dev, tca8418_diable_hw, keypad_data);
[Severity: Low]
Does this refer to the tca8418_disable_hw() function added above? It seems
to be missing an 's', which will also prevent the driver from compiling.

> +	if (error)
> +		return error;
> +
>  	error = input_register_device(input);
>  	if (error) {
>  		dev_err(dev, "Unable to register input device, error: %d\n",

While reviewing this patch, I noticed a pre-existing issue in the
tca8418_read_keypad() function:

drivers/input/keyboard/tca8418_keypad.c:tca8418_read_keypad() {
[ ... ]
		state = reg & KEY_EVENT_VALUE;
		code  = reg & KEY_EVENT_CODE;

		row = code / TCA8418_MAX_COLS;
		col = code % TCA8418_MAX_COLS;

		row = (col) ? row : row - 1;
		col = (col) ? col - 1 : TCA8418_MAX_COLS - 1;

		code = MATRIX_SCAN_CODE(row, col, keypad_data->row_shift);
		input_event(input, EV_MSC, MSC_SCAN, code);
		input_report_key(input, keymap[code], state);
[ ... ]
}

[Severity: High]
This isn't a bug introduced by this patch, but can a malfunctioning or
malicious I2C device cause an out-of-bounds read on the keymap array here?

If the device returns a large register value (e.g., reg & 0x7F is 127), the
calculated row and col are not verified against the actual matrix dimensions.
Will this produce a code index that exceeds the keymap array's allocated
size when passed to input_report_key()?

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

[PATCH v2] Input: tca8418_keypad - Add devm cleanup to disable interrupts on unload

[Zhian Liang]

tca8418_configure() enables TCA8418 interrupts by writing the CFG
register.  If input_register_device() fails afterwards, probe returns
an error and devres releases the IRQ handler, but the hardware is
left with interrupts still enabled.

Fix it by registering a devm action that clears the CFG register
 when the device is released, disabling
all interrupts on both probe failure and driver unbind.

Signed-off-by: Zhian Liang <liangzhan5dev@gmail.com>

---
Changes in v2:
- Fixed typo: tca84818_write_byte -> tca8418_write_byte
- Fixed typo: tca8418_diable_hw -> tca8418_disable_hw
---
 drivers/input/keyboard/tca8418_keypad.c | 18 ++++++++++++++++++
 1 file changed, 18 insertions(+)

diff --git a/drivers/input/keyboard/tca8418_keypad.c b/drivers/input/keyboard/tca8418_keypad.c
index 36d4a7c38bb1..413b404a08ba 100644
--- a/drivers/input/keyboard/tca8418_keypad.c
+++ b/drivers/input/keyboard/tca8418_keypad.c
@@ -270,6 +270,20 @@ static int tca8418_configure(struct tca8418_keypad *keypad_data,
 	return error;
 }
 
+static void tca8418_disable_hw(void *data)
+{
+	struct tca8418_keypad *keypad_data = data;
+	int error;
+
+	error = tca8418_write_byte(keypad_data, REG_CFG, 0);
+	if (error)
+		dev_warn(&keypad_data->client->dev, "unable to disable interrupts: %d\n", error);
+
+	error = tca8418_write_byte(keypad_data, REG_INT_STAT, 0xff);
+	if (error)
+		dev_warn(&keypad_data->client->dev, "unable to clear interrupt status: %d\n", error);
+}
+
 static int tca8418_keypad_probe(struct i2c_client *client)
 {
 	struct device *dev = &client->dev;
@@ -356,6 +370,10 @@ static int tca8418_keypad_probe(struct i2c_client *client)
 	if (error < 0)
 		return error;
 
+	error = devm_add_action_or_reset(dev, tca8418_disable_hw, keypad_data);
+	if (error)
+		return error;
+
 	error = input_register_device(input);
 	if (error) {
 		dev_err(dev, "Unable to register input device, error: %d\n",
-- 
2.34.1

Re: [PATCH v2] Input: tca8418_keypad - Add devm cleanup to disable interrupts on unload

[sashiko-bot]

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

Pre-existing issues:
- [High] Missing bounds check on hardware-provided scan code leads to an out-of-bounds read on the keymap array.
--

commit a7ecc5efa5741df5c7a115258a1df26344bc93c3
Author: Zhian Liang <liangzhan5dev@gmail.com>

Input: tca8418_keypad - Add devm cleanup to disable interrupts on unload

This patch adds a devm action during probe to clear the CFG register and
interrupt status when the device is released. This ensures interrupts are
properly disabled on probe failure and driver unbind.

> diff --git a/drivers/input/keyboard/tca8418_keypad.c b/drivers/input/keyboard/tca8418_keypad.c
> index 4616afa0286c3..a7c7e23f16617 100644
> --- a/drivers/input/keyboard/tca8418_keypad.c
> +++ b/drivers/input/keyboard/tca8418_keypad.c

[ ... ]

While reviewing this patch, I noticed a pre-existing issue in the event
handling path:

drivers/input/keyboard/tca8418_keypad.c:tca8418_read_keypad() {
    ...
		code = MATRIX_SCAN_CODE(row, col, keypad_data->row_shift);
		input_event(input, EV_MSC, MSC_SCAN, code);
		input_report_key(input, keymap[code], state);
    ...
}

[Severity: High]
This isn't a bug introduced by this patch, but pre-existed in the codebase.

Does this code lack a bounds check on the hardware-provided scan code?

If a malfunctioning or maliciously crafted I2C device returns a large row
or column value, it appears the computed code index could exceed the
allocated keymap size. Using this index directly in input_report_key()
might lead to an out-of-bounds read on the dynamically allocated keymap
array, potentially causing a kernel panic or leaking adjacent kernel memory.

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