Re: [PATCH] Input: stmpe-keypad - disable keypad on probe failure

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] Disabling hardware before freeing managed IRQ creates a race condition on probe failure.

Pre-existing issues:
- [High] Logic bug in `stmpe_keypad_altfunc_init()` aborts pull-up configuration on non-zero register values and ignores errors.
- [High] Heap buffer overflow in `keymap` parsing when device tree specifies dimensions exceeding 8x8.
--

--- Patch [1]: [PATCH] Input: stmpe-keypad - disable keypad on probe failure ---
commit 849a668c20609a76cff8c0630950b6029798e3f9
Author: Myeonghun Pak <mhun512@gmail.com>
Subject: Input: stmpe-keypad - disable keypad on probe failure

stmpe_keypad_chip_init() enables the keypad block before programming
its registers, but does not disable it on failure. This patch routes
initialization and probe failures through a cleanup path so a failed
probe does not leave the keypad block enabled.

Fixes: 76f10845318b ("input: Add STMPE keypad driver")
Cc: stable@vger.kernel.org

> diff --git a/drivers/input/keyboard/stmpe-keypad.c b/drivers/input/keyboard/stmpe-keypad.c
> --- a/drivers/input/keyboard/stmpe-keypad.c
> +++ b/drivers/input/keyboard/stmpe-keypad.c
[ ... ]
> @@ -280,35 +280,44 @@ static int stmpe_keypad_chip_init(struct stmpe_keypad *keypad)
>  
>  	ret = stmpe_keypad_altfunc_init(keypad);
>  	if (ret < 0)
> -		return ret;
> +		goto disable_keypad;
>  

[Severity: High]
This is a pre-existing issue, but does this logic correctly handle the return
value of the configuration function?

Looking at stmpe_keypad_altfunc_init(), it reads a register which returns a
positive value on success, but it treats any non-zero value as an error:

drivers/input/keyboard/stmpe-keypad.c:stmpe_keypad_altfunc_init() {
    ...
    ret = stmpe_reg_read(stmpe, pureg);
    if (ret)
        return ret;
    ...
}

This causes stmpe_keypad_altfunc_init() to return a positive value, skipping
the rest of the pull-up configuration. Because the check here in
stmpe_keypad_chip_init() only looks for (ret < 0), the positive error code is
silently ignored, leaving the matrix columns floating.

[ ... ]
> @@ -389,19 +398,24 @@ static int stmpe_keypad_probe(struct platform_device *pdev)
>  					  IRQF_ONESHOT, "stmpe-keypad", keypad);
>  	if (error) {
>  		dev_err(&pdev->dev, "unable to get irq: %d\n", error);
> -		return error;
> +		goto disable_keypad;
>  	}
>  
>  	error = input_register_device(input);
>  	if (error) {
>  		dev_err(&pdev->dev,
>  			"unable to register input device: %d\n", error);
> -		return error;
> +		goto disable_keypad;
>  	}

[Severity: High]
This is a pre-existing issue, but could the matrix_keypad_build_keymap()
call earlier in this probe function cause a heap buffer overflow?

The keypad->keymap array is statically sized to 64 elements
(STMPE_KEYPAD_MAX_ROWS * STMPE_KEYPAD_MAX_COLS). However, the device tree
parsing does not validate that the parsed rows and cols fit within these 8x8
bounds.

If a variant like STMPE24xx supports and specifies 12 rows in the device
tree, won't matrix_keypad_build_keymap() write up to 96 elements into the
64-element keypad->keymap array?

>  
>  	platform_set_drvdata(pdev, keypad);
>  
>  	return 0;
> +
> +disable_keypad:
> +	stmpe_disable(stmpe, STMPE_BLOCK_KEYPAD);
> +
> +	return error;
>  }

[Severity: Medium]
Because the IRQ is managed by devres via devm_request_threaded_irq(), it
won't be freed until after the probe function returns. By manually disabling
the keypad block here, does this introduce a race condition?

If an interrupt fires before devres cleans up the IRQ, won't the interrupt
handler (stmpe_keypad_irq) execute and attempt to read from the disabled
hardware registers?

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