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

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

[Rosen Penev]

Convert the fixed-size keys array to a flexible array member and use
struct_size() for allocation. Move the key count validation before
allocation and use for_each_available_child_of_node_scoped() to iterate
only over available child nodes as the allocation uses
of_get_available_child_count.

Also use a key variable in the loop for clarity.

Use of_device_get_match_data() to fetch the PMIC key register data directly
instead of open-coding an of_match_device() lookup.

This also lets the driver drop the of_device.h include.

Remove platform_set_drvdata. Unused anywhere.

Assisted-by: OpenCode:BigPickle
Signed-off-by: Rosen Penev <rosenp@gmail.com>
---
 drivers/input/keyboard/mtk-pmic-keys.c | 55 ++++++++++++--------------
 1 file changed, 25 insertions(+), 30 deletions(-)

diff --git a/drivers/input/keyboard/mtk-pmic-keys.c b/drivers/input/keyboard/mtk-pmic-keys.c
index c78d9f6d97c4..5b8650f46105 100644
--- a/drivers/input/keyboard/mtk-pmic-keys.c
+++ b/drivers/input/keyboard/mtk-pmic-keys.c
@@ -16,7 +16,6 @@
 #include <linux/mfd/mt6397/core.h>
 #include <linux/mfd/mt6397/registers.h>
 #include <linux/module.h>
-#include <linux/of_device.h>
 #include <linux/of.h>
 #include <linux/platform_device.h>
 #include <linux/regmap.h>
@@ -147,7 +146,7 @@ struct mtk_pmic_keys {
 	struct input_dev *input_dev;
 	struct device *dev;
 	struct regmap *regmap;
-	struct mtk_pmic_keys_info keys[MTK_PMIC_MAX_KEY_COUNT];
+	struct mtk_pmic_keys_info keys[];
 };
 
 enum mtk_pmic_keys_lp_mode {
@@ -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);
 	if (!keys)
 		return -ENOMEM;
 
 	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);
+	if (!mtk_pmic_regs)
+		return -EINVAL;
 
 	keys->input_dev = input_dev = devm_input_allocate_device(keys->dev);
 	if (!input_dev) {
@@ -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) {
+		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];
-
-		keys->keys[index].irq =
-			platform_get_irq_byname(pdev, irqnames[index]);
-		if (keys->keys[index].irq < 0)
-			return keys->keys[index].irq;
+		key->irq = platform_get_irq_byname(pdev, irqnames[index]);
+		if (key->irq < 0)
+			return key->irq;
 
 		if (mtk_pmic_regs->key_release_irq) {
-			keys->keys[index].irq_r = platform_get_irq_byname(pdev,
-									  irqnames_r[index]);
-
-			if (keys->keys[index].irq_r < 0)
-				return keys->keys[index].irq_r;
+			key->irq_r = platform_get_irq_byname(pdev, irqnames_r[index]);
+			if (key->irq_r < 0)
+				return key->irq_r;
 		}
 
-		error = of_property_read_u32(child,
-			"linux,keycodes", &keys->keys[index].keycode);
+		error = of_property_read_u32(child, "linux,keycodes", &key->keycode);
 		if (error) {
 			dev_err(keys->dev,
 				"failed to read key:%d linux,keycode property: %d\n",
@@ -389,9 +386,9 @@ static int mtk_pmic_keys_probe(struct platform_device *pdev)
 		}
 
 		if (of_property_read_bool(child, "wakeup-source"))
-			keys->keys[index].wakeup = true;
+			key->wakeup = true;
 
-		error = mtk_pmic_key_setup(keys, &keys->keys[index]);
+		error = mtk_pmic_key_setup(keys, key);
 		if (error)
 			return error;
 
@@ -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);
-
 	return 0;
 }
 
-- 
2.54.0

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

[sashiko-bot]

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