Re: [PATCH v3] Input: tca8418_keypad - fix potential infinite loop and OOB access on invalid keycodes

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] The patch fails to resolve the infinite loop caused by invalid keycodes (e.g., 0xFF from a stuck I2C bus) and introduces continuous log spam that causes a soft lockup.
- [High] Hardware interrupts are never disabled on driver unbind or during a probe failure, which can lead to an IRQ storm on shared interrupt lines.
- [High] Clearing the interrupt status register after draining the FIFO introduces a race condition that can result in dropped or delayed key events.
--

--- Patch [1]: [PATCH v3] Input: tca8418_keypad - fix potential infinite loop and OOB access on invalid keycodes ---
commit 6913c66fea4ba1ffd746fcc00c1d3e4e7e05ef21
Author: Zhian Liang <liangzhan5dev@gmail.com>
Subject: Input: tca8418_keypad - fix potential infinite loop and OOB access on invalid keycodes

This patch modifies the empty-FIFO check to evaluate the extracted keycode
rather than the raw register. It also adds a bounds check against the
configured matrix dimensions to skip invalid events.

Cc: stable@vger.kernel.org

> diff --git a/drivers/input/keyboard/tca8418_keypad.c b/drivers/input/keyboard/tca8418_keypad.c
> index 4616afa0286c3..41836c79261a1 100644
> --- a/drivers/input/keyboard/tca8418_keypad.c
> +++ b/drivers/input/keyboard/tca8418_keypad.c
> @@ -171,19 +173,28 @@ static void tca8418_read_keypad(struct tca8418_keypad *keypad_data)
>  			break;
>  		}
>  
> -		/* Assume that key code 0 signifies empty FIFO */
> -		if (reg <= 0)
> -			break;
>  
>  		state = reg & KEY_EVENT_VALUE;
>  		code  = reg & KEY_EVENT_CODE;
>  
> +		/* Key code 0 signifies empty FIFO */
> +		if (!code)
> +			break;
> +
>  		row = code / TCA8418_MAX_COLS;
>  		col = code % TCA8418_MAX_COLS;
>  
>  		row = (col) ? row : row - 1;
>  		col = (col) ? col - 1 : TCA8418_MAX_COLS - 1;
>  
> +		/* Validate against configured matrix size */
> +		if (row >= keypad_data->rows || col >= keypad_data->cols) {
> +			dev_err(&keypad_data->client->dev,
> +				"invalid key code %d (row %d, col %d)\n",
> +				code, row, col);
> +			continue;
> +		}

[Severity: High]
This isn't a bug introduced by this patch, but does this actually fix the
infinite loop on hardware faults as described in the commit message?

If the hardware returns 0xFF due to a stuck I2C bus, the code variable
evaluates to 127 because of the bitwise AND operation:

code = reg & KEY_EVENT_CODE;

Since 127 is not 0, the new empty FIFO check (!code) is bypassed.

The execution then falls through to the new validation check, which catches
the invalid bounds, prints the dev_err message, and executes continue.

Will this cause the do-while loop to immediately fetch 0xFF again without
terminating, creating a continuous stream of dev_err log spam and a soft
lockup? 

Should this loop have a safeguard or break on sequential invalid reads
instead of continuing?

[ ... ]

> @@ -299,6 +310,8 @@ static int tca8418_keypad_probe(struct i2c_client *client)
>  
>  	keypad_data->client = client;
>  	keypad_data->row_shift = row_shift;
> +	keypad_data->rows = rows;
> +	keypad_data->cols = cols;
>  
>  	/* Read key lock register, if this fails assume device not present */
>  	error = tca8418_read_byte(keypad_data, REG_KEY_LCK_EC, &reg);

[Severity: High]
This is a pre-existing issue, but does the driver safely handle probe
failures or driver unbinding?

Looking at tca8418_keypad_probe(), if input_register_device() fails, the
probe function returns an error and devres unwinds. However,
tca8418_configure() was already called earlier in the probe, which enables
interrupts on the hardware. 

Since there is no .remove callback in tca8418_keypad_driver, and no devm
cleanup action to disable the hardware interrupt, an unbind or probe failure
leaves the device asserting the shared IRQ line indefinitely. 

Could this lead to an unhandled IRQ storm that forces the kernel to disable
the shared IRQ line, breaking other devices?

[Severity: High]
This is another pre-existing issue, but is there a race condition in
tca8418_irq_handler() that drops key events?

The IRQ handler drains the FIFO by calling tca8418_read_keypad()
and then subsequently clears the interrupt status register:

drivers/input/keyboard/tca8418_keypad.c:tca8418_irq_handler() {
    ...
    if (reg & INT_STAT_K_INT)
        tca8418_read_keypad(keypad_data);

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

If a new key is pressed immediately after the FIFO is drained but before
tca8418_write_byte() executes, the hardware queues the event and asserts
K_INT again. The driver then writes 0xff, clearing the newly asserted K_INT
without ever reading the event. 

Could this cause a key release event to be permanently left in the FIFO,
resulting in an indefinitely stuck key?

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