Re: [PATCHv2 1/2] Input: mtk-pmic-keys: use of_device_get_match_data()

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

Pre-existing issues:
- [High] Mismatch between counting available child nodes and iterating over all child nodes leads to an out-of-bounds array access.
--

--- Patch [1]: [PATCHv2 1/2] Input: mtk-pmic-keys: use of_device_get_match_data() ---
commit 8b3f8694df03175d2fcd1e69a487343b4d70d629
Author: Rosen Penev <[email protected]>

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

This commit replaces open-coded match data retrieval with
of_device_get_match_data() and removes the 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
[ ... ]
> @@ -333,8 +332,6 @@ static int mtk_pmic_keys_probe(struct platform_device *pdev)
>  	struct mtk_pmic_keys *keys;
>  	const struct mtk_pmic_regs *mtk_pmic_regs;
>  	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);
>  	if (!keys)
> @@ -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);
>  
>  	keys->input_dev = input_dev = devm_input_allocate_device(keys->dev);

[Severity: High]
This is a pre-existing issue, but while reviewing mtk_pmic_keys_probe(),
I noticed a potential out-of-bounds array access further down in the
function.

The function correctly limits the expected number of keys by checking
the count of available child nodes:

drivers/input/keyboard/mtk-pmic-keys.c:mtk_pmic_keys_probe() {
    ...
    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, not just the available ones:

drivers/input/keyboard/mtk-pmic-keys.c:mtk_pmic_keys_probe() {
    ...
    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 provides more child nodes than MTK_PMIC_MAX_KEY_COUNT
but only a subset are marked as available (for example, with
status = "okay"), the bounds check will pass. Because the loop iterates over
all child nodes regardless of their availability status, could index exceed
the array limits?

Would it be safer to use for_each_available_child_of_node_scoped() here to
ensure the loop matches the earlier available child count check and prevents
an out-of-bounds read on irqnames[] and write to keys->keys[]?

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