Re: [PATCH] staging: nvec: kbd: bound keycode table lookups

[Marc Dietrich <marvin24@gmx.de>]

Hallo Mohamed,

On Thu, 12 Nov 2026, Mohamed Anis Ben Azza wrote:

> nvec_keys_notifier() indexes code_tabs[_size][code] using values taken
> directly from the embedded controller's event message without checking
> them against the tables:
>
> - _size is a 2-bit field (0..3) from the message header, but code_tabs[]
>   only has entries for NVEC_2BYTES and NVEC_3BYTES. NVEC_VAR_SIZE is
>   rejected, but the reserved encoding 3 is not, so code_tabs[3] reads a
>   pointer past the end of the array and then dereferences it.

this is only a theroretical case. The size by protocol can either be
0, 1, or 2. As you already said, 2 is skipped in the code, so only 0 and
1 are values which we could expect.

> - code is msg[1] & 0x7f (0..127), but code_tab_102us[] has only 126
>   entries and extcode_tab_us102[] only 110, so scancodes at or above
>   those sizes read past the end of the tables.

well, the keyboard has only a limited number of keys and values > 126 (for 
basic) and >110 for extended codes cannot be produced (while theoretical 
possible by protocol).

> Reject any size encoding without a table and any scancode beyond the
> end of the selected table, dropping the event as is already done for
> the power button.
>
> This only matters for an EC that sends malformed or out-of-spec
> events, so it is a robustness fix rather than a security issue.
>
> The issue was found and the fix drafted with the help of an AI coding
> assistant (Claude). It was compile-tested with
> ARCH=arm LLVM=1 tegra_defconfig + MFD_NVEC/KEYBOARD_NVEC at W=1 with no
> new warnings, and the lookup logic was exercised in a userspace harness
> under AddressSanitizer for all 4 size encodings x 256 scancodes: the old
> code reports a global-buffer-overflow, the new code none. It has not
> been tested on real Tegra/AC100 hardware.
>
> Fixes: 32890b983086 ("Staging: initial version of the nvec driver")
> Signed-off-by: Mohamed Anis Ben Azza <benazzaanis783@gmail.com>
> ---
> drivers/staging/nvec/nvec_kbd.c | 15 +++++++++++++--
> 1 file changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/staging/nvec/nvec_kbd.c b/drivers/staging/nvec/nvec_kbd.c
> index d2b91318f..9b976f0f5 100644
> --- a/drivers/staging/nvec/nvec_kbd.c
> +++ b/drivers/staging/nvec/nvec_kbd.c
> @@ -28,6 +28,11 @@ enum kbd_subcmds {
> static unsigned char keycodes[ARRAY_SIZE(code_tab_102us)
> 			      + ARRAY_SIZE(extcode_tab_us102)];
>
> +static const unsigned int code_tab_sizes[] = {
> +	ARRAY_SIZE(code_tab_102us),
> +	ARRAY_SIZE(extcode_tab_us102),
> +};
> +
> struct nvec_keys {
> 	struct input_dev *input;
> 	struct notifier_block notifier;
> @@ -59,8 +64,11 @@ static int nvec_keys_notifier(struct notifier_block *nb,
> 	if (event_type == NVEC_KB_EVT) {
> 		int _size = (msg[0] & (3 << 5)) >> 5;
>
> -/* power on/off button */
> -		if (_size == NVEC_VAR_SIZE)
> +		/*
> +		 * NVEC_VAR_SIZE is the power on/off button; the fourth
> +		 * encoding is reserved. Neither has a keycode table.
> +		 */
> +		if (_size >= ARRAY_SIZE(code_tabs))
> 			return NOTIFY_STOP;

are you doing the right thing here? The original code exists the function 
when _size is 2 (NVEC_VAR_SIZE), but you just let it fall through.

> 		if (_size == NVEC_3BYTES)
> @@ -69,6 +77,9 @@ static int nvec_keys_notifier(struct notifier_block *nb,
> 		code = msg[1] & 0x7f;
> 		state = msg[1] & 0x80;
>
> +		if (code >= code_tab_sizes[_size])
> +			return NOTIFY_STOP;
> +
> 		if (code_tabs[_size][code] == KEY_CAPSLOCK && state)
> 			nvec_kbd_toggle_led();

As I said, this is only a theroretical scenario which could happen if the 
protocol is violated or someone manages to connect an internal keybaord 
with more keys. While it is not impossible that machines exist out there
which have such keyboards, the is very unlikely.

Best wishes,
Marc