Re: [PATCH] Input: mtk-pmic-keys: Use flexible array member for keys

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Removal of `platform_set_drvdata()` causes a NULL pointer dereference in power management callbacks.
- [High] Suspend and resume operations unconditionally iterate beyond the dynamically allocated bounds of the `keys` flexible array.
- [High] Switching to `for_each_available_child_of_node_scoped()` shifts positional indexing, assigning incorrect hardware resources if a preceding node is disabled.
--

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

Input: mtk-pmic-keys: Use flexible array member for keys

Convert the fixed-size keys array to a flexible array member and allocate it
based on the available child node count. The patch also modifies the child
iteration to skip disabled nodes and drops the driver data assignment since it
was mistakenly thought to be unused.

> diff --git a/drivers/input/keyboard/mtk-pmic-keys.c b/drivers/input/keyboard/mtk-pmic-keys.c
> index c78d9f6d97c4f..5b8650f461050 100644
> --- a/drivers/input/keyboard/mtk-pmic-keys.c
> +++ b/drivers/input/keyboard/mtk-pmic-keys.c
[ ... ]
> @@ -332,17 +331,25 @@ static int mtk_pmic_keys_probe(struct platform_device *pdev)
>  	static const char *const irqnames_r[] = { "powerkey_r", "homekey_r" };
>  	struct mtk_pmic_keys *keys;
>  	const struct mtk_pmic_regs *mtk_pmic_regs;
> +	struct mtk_pmic_keys_info *key;
>  	struct input_dev *input_dev;
> -	const struct of_device_id *of_id =
> -		of_match_device(of_mtk_pmic_keys_match_tbl, &pdev->dev);
>  
> -	keys = devm_kzalloc(&pdev->dev, sizeof(*keys), GFP_KERNEL);
> +	keycount = of_get_available_child_count(node);
> +	if (keycount > MTK_PMIC_MAX_KEY_COUNT ||
> +	    keycount > ARRAY_SIZE(irqnames)) {
> +		dev_err(&pdev->dev, "too many keys defined (%d)\n", keycount);
> +		return -EINVAL;
> +	}
> +
> +	keys = devm_kzalloc(&pdev->dev, struct_size(keys, keys, keycount), GFP_KERNEL);

[Severity: High]
Does this dynamic allocation create an out-of-bounds access in the suspend
and resume callbacks?

The PM callbacks retain a hardcoded loop using MTK_PMIC_MAX_KEY_COUNT:

mtk_pmic_keys_suspend() {
    ...
	for (index = 0; index < MTK_PMIC_MAX_KEY_COUNT; index++) {
		if (keys->keys[index].wakeup) {
    ...
}

If keycount is less than MTK_PMIC_MAX_KEY_COUNT (for example, if only one key
is available), won't the loop read out of bounds by accessing unallocated
memory in the flexible array?

>  	if (!keys)
>  		return -ENOMEM;
[ ... ]
> @@ -356,31 +363,21 @@ static int mtk_pmic_keys_probe(struct platform_device *pdev)
>  	input_dev->id.product = 0x0001;
>  	input_dev->id.version = 0x0001;
>  
> -	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_available_child_of_node_scoped(node, child) {

[Severity: High]
Does switching to for_each_available_child_of_node_scoped() assign incorrect
hardware resources if a preceding node is disabled?

The driver relies on positional indexing to map DT child nodes to specific
hardware registers and IRQs. For example:

	static const char *const irqnames[] = { "powerkey", "homekey" };
    ...
		key->irq = platform_get_irq_byname(pdev, irqnames[index]);

By skipping disabled nodes with the available child variant, if the first
node (powerkey) is disabled in the device tree, the second node (homekey)
will be processed at index 0. Won't this incorrectly assign the powerkey IRQ
and registers to the homekey node?

> +		key = &keys->keys[index];
> +		key->regs = &mtk_pmic_regs->keys_regs[index];
>  
> -	for_each_child_of_node_scoped(node, child) {
> -		keys->keys[index].regs = &mtk_pmic_regs->keys_regs[index];
[ ... ]
> @@ -407,8 +404,6 @@ static int mtk_pmic_keys_probe(struct platform_device *pdev)
>  
>  	mtk_pmic_keys_lp_reset_setup(keys, mtk_pmic_regs);
>  
> -	platform_set_drvdata(pdev, keys);
> -

[Severity: High]
Does removing this platform_set_drvdata() cause a NULL pointer dereference
during suspend and resume?

The power management callbacks mtk_pmic_keys_suspend() and
mtk_pmic_keys_resume() rely on dev_get_drvdata(dev) to retrieve the keys
pointer:

mtk_pmic_keys_suspend() {
	struct mtk_pmic_keys *keys = dev_get_drvdata(dev);
	int index;

	for (index = 0; index < MTK_PMIC_MAX_KEY_COUNT; index++) {
		if (keys->keys[index].wakeup) {
        ...
}

Because the driver data is no longer set, won't dev_get_drvdata() return NULL
and crash when accessing keys->keys[index].wakeup?

>  	return 0;
>  }

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