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

["Rosen Penev" <rosenp@gmail.com>]

On Sun May 17, 2026 at 9:34 PM PDT, sashiko-bot wrote:
> 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?
No because every compatible has a non NULL data component.
>
>
> 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?
Yes. Interesting that pre-existing issues are flagged...