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

[sashiko-bot@kernel.org]

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