Re: [PATCH] Input: mtk-pmic-keys - use of_device_get_match_data()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Missing NULL check for of_device_get_match_data() result leads to NULL pointer dereference.
- [High] Mismatch between child counting and iteration logic can cause out-of-bounds array accesses.
--

commit 0ee3fee49ccf02b7e629f90533e67fc9b40d2b9e
Author: Rosen Penev <rosenp@gmail.com>

Input: mtk-pmic-keys - use of_device_get_match_data()

This commit simplifies the driver to fetch PMIC key register data directly
using of_device_get_match_data(). It also removes the unneeded of_device.h
include.

> diff --git a/drivers/input/keyboard/mtk-pmic-keys.c b/drivers/input/keyboard/mtk-pmic-keys.c
> index c78d9f6d97c4f..fd684ac169386 100644
> --- a/drivers/input/keyboard/mtk-pmic-keys.c
> +++ b/drivers/input/keyboard/mtk-pmic-keys.c
[ ... ]
> @@ -342,7 +339,7 @@ static int mtk_pmic_keys_probe(struct platform_device *pdev)
>  
>  	keys->dev = &pdev->dev;
>  	keys->regmap = pmic_chip->regmap;
> -	mtk_pmic_regs = of_id->data;
> +	mtk_pmic_regs = of_device_get_match_data(&pdev->dev);

While this isn't a new issue introduced by this patch, can mtk_pmic_regs be
NULL here if the device is bound manually via sysfs without a matching
device tree node?

If mtk_pmic_regs is NULL, it appears it is unconditionally dereferenced
later when calling mtk_pmic_keys_lp_reset_setup().

Would it make sense to add a check for !mtk_pmic_regs right after fetching
the match data?


Also, while reviewing this probe function, I noticed another pre-existing
issue further down:

	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;
	}

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

Is there a mismatch between the child counting and iteration logic?

The validation check uses of_get_available_child_count(), which ignores
disabled nodes. But the loop uses for_each_child_of_node_scoped(), which
iterates over all children, including disabled ones.

If a disabled child node exists in the device tree alongside available ones,
could the loop execute more times than the initial validation accounted for?
This might increment the index variable past the allocated array bounds,
leading to out-of-bounds accesses on irqnames[] and keys->keys[].

Would updating the loop to use for_each_available_child_of_node_scoped()
prevent this?

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