Re: [PATCH v9 6/9] input: keyboard: mtk-pmic-keys: Add MT6392 support

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

Pre-existing issues:
- [High] Mismatch between bounds checking and child node iteration causes out-of-bounds array accesses if disabled child nodes are present.
- [High] Physical keys mapped to incorrect hardware resources and IRQs due to strict dependency on DT node parsing order.
--

--- Patch [6]: [PATCH v9 6/9] input: keyboard: mtk-pmic-keys: Add MT6392 support ---
commit 7d4906573abcc4f44ff8cbf94c0c7919e5484f08
Author: Val Packett <[email protected]>

input: keyboard: mtk-pmic-keys: Add MT6392 support

This commit adds support for the MT6392 PMIC to the keys driver.

> diff --git a/drivers/input/keyboard/mtk-pmic-keys.c b/drivers/input/keyboard/mtk-pmic-keys.c
> index c78d9f6d97c4f..8b4a89fce4fb1 100644
> --- a/drivers/input/keyboard/mtk-pmic-keys.c
> +++ b/drivers/input/keyboard/mtk-pmic-keys.c
[ ... ]
> @@ -301,6 +315,9 @@ static const struct of_device_id of_mtk_pmic_keys_match_tbl[] = {
>  	{
>  		.compatible = "mediatek,mt6397-keys",
>  		.data = &mt6397_regs,
> +	}, {
> +		.compatible = "mediatek,mt6392-keys",
> +		.data = &mt6392_regs,

[Severity: High]
This isn't a bug introduced by this patch, but I noticed a potential
out-of-bounds array access in mtk_pmic_keys_probe() when handling these keys.

The probe function limits the maximum number of keys by checking the count of
available nodes:

	keycount = of_get_available_child_count(node);
	if (keycount > MTK_PMIC_MAX_KEY_COUNT ||
	    keycount > ARRAY_SIZE(irqnames)) {
		dev_err(keys->dev, "too many keys defined (%d)\n", keycount);
		return -EINVAL;
	}

However, it then iterates over all child nodes, including disabled ones:

	for_each_child_of_node_scoped(node, child) {
		keys->keys[index].regs = &mtk_pmic_regs->keys_regs[index];

		keys->keys[index].irq =
			platform_get_irq_byname(pdev, irqnames[index]);

If a device tree defines disabled child nodes alongside available ones, such
that the available count passes the check but the total child count exceeds
MTK_PMIC_MAX_KEY_COUNT, could this loop iterate past the bounds of keys->keys[]
and irqnames[]?


[Severity: High]
This is also a pre-existing issue, but does this mapping logic safely correlate
device tree nodes to the correct physical hardware?

The driver maps the nodes based entirely on their iteration order (index)
rather than checking the node names:

	for_each_child_of_node_scoped(node, child) {
		keys->keys[index].regs = &mtk_pmic_regs->keys_regs[index];

Since index 0 is unconditionally treated as the power key and index 1 as the
home key, what happens if a device tree omits the power key and only defines
a home key? Will the home key node be improperly mapped to the power key
hardware resources and IRQs?

>  	}, {
>  		.compatible = "mediatek,mt6323-keys",
>  		.data = &mt6323_regs,

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=6