Re: [PATCH v3 02/10] Input: samsung-keypad - handle compact binding

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] IRQ handler is requested before runtime PM is enabled, leading to potential bus faults.
- [High] Uninitialized stack variables `row`, `col`, and `key_code` leak kernel stack memory and can cause out-of-bounds array access.
- [Medium] Time-of-Check to Time-of-Use (TOCTOU) race condition when parsing child nodes causes heap buffer overflow.
--

--- Patch [2]: [PATCH v3 02/10] Input: samsung-keypad - handle compact binding ---
commit df7d9c0ab9ce4671eb0b2980bb819cb1ac325753
Author: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Subject: Input: samsung-keypad - handle compact binding

Add support for standard matrix keymap binding (in addition to the
existing verbose binding with a sub-node for each key). This will
allow easier conversions from platform data to device properties when
using static device properties.

> diff --git a/drivers/input/keyboard/samsung-keypad.c b/drivers/input/keyboard/samsung-keypad.c
> index 17127269e3f05..cc34f0146161e 100644
> --- a/drivers/input/keyboard/samsung-keypad.c
> +++ b/drivers/input/keyboard/samsung-keypad.c
> @@ -231,83 +231,89 @@ static void samsung_keypad_close(struct input_dev *input_dev)
>  	samsung_keypad_stop(keypad);
>  }
>  
> -#ifdef CONFIG_OF
> -static struct samsung_keypad_platdata *
> -samsung_keypad_parse_dt(struct device *dev)

[ ... ]

> +static const struct matrix_keymap_data *
> +samsung_parse_verbose_keymap(struct device *dev)
> +{
> +	struct matrix_keymap_data *keymap_data;
> +	struct fwnode_handle *child;
> +	u32 *keymap;
> +	unsigned int key_count;
> +
> +	keymap_data = devm_kzalloc(dev, sizeof(*keymap_data), GFP_KERNEL);
> +	if (!keymap_data)
> +		return ERR_PTR(-ENOMEM);
> +
> +	key_count = device_get_child_node_count(dev);
> +	keymap = devm_kcalloc(dev, key_count, sizeof(*keymap), GFP_KERNEL);
> +	if (!keymap)
> +		return ERR_PTR(-ENOMEM);
> +
> +	keymap_data->keymap_size = key_count;
> +	keymap_data->keymap = keymap;
> +
> +	device_for_each_child_node(dev, child) {
> +		u32 row, col, key_code;
> +
> +		fwnode_property_read_u32(child, "keypad,row", &row);
> +		fwnode_property_read_u32(child, "keypad,column", &col);
> +		fwnode_property_read_u32(child, "linux,code", &key_code);
> +
> +		*keymap++ = KEY(row, col, key_code);

[Severity: Medium]
This is a pre-existing issue, but could this loop overflow the allocated
keymap array?

The array size is determined earlier:

	key_count = device_get_child_node_count(dev);
	keymap = devm_kcalloc(dev, key_count, sizeof(*keymap), GFP_KERNEL);

Because there is no synchronization explicitly held, if a concurrent device
tree overlay adds a child node to the keypad between the count and the
iteration here in device_for_each_child_node(), could the loop execute more
times than allocated and cause an out-of-bounds heap write?

[Severity: High]
This isn't a bug introduced by this patch, but what happens if the device
tree child node is missing properties?

If fwnode_property_read_u32() fails for "keypad,row", "keypad,column", or
"linux,code", it leaves the output variables uninitialized.

Since the return values are ignored, the uninitialized stack variables row,
col, and key_code are passed into the KEY() macro and written to the keymap.
Could this leak kernel stack memory to userspace via the EVIOCGKEYCODE ioctl,
or create incorrect key mappings?

>  	}
>  
> +	return keymap_data;
> +}

[ ... ]

> @@ -430,11 +435,14 @@ static int samsung_keypad_probe(struct platform_device *pdev)
>  	if (error)
>  		return error;
>  
> -	if (pdev->dev.of_node) {
> -		devm_kfree(&pdev->dev, (void *)pdata->keymap_data->keymap);
> -		devm_kfree(&pdev->dev, (void *)pdata->keymap_data);
> +	if (!dev_get_platdata(&pdev->dev)) {
> +		if (pdata->keymap_data) {
> +			devm_kfree(&pdev->dev, (void *)pdata->keymap_data->keymap);
> +			devm_kfree(&pdev->dev, (void *)pdata->keymap_data);
> +		}
>  		devm_kfree(&pdev->dev, (void *)pdata);
>  	}
> +

[Severity: High]
This is a pre-existing issue, but could the IRQ handler run before runtime PM
is enabled?

In samsung_keypad_probe(), devm_request_threaded_irq() is called to unmask
the IRQ before devm_pm_runtime_enable() is called:

	error = devm_request_threaded_irq(&pdev->dev, keypad->irq, NULL,
					  samsung_keypad_irq, IRQF_ONESHOT,
					  dev_name(&pdev->dev), keypad);
	[ ... ]
	error = devm_pm_runtime_enable(&pdev->dev);

If an interrupt is pending (for example, left over from the bootloader),
samsung_keypad_irq() could fire immediately.  The handler calls
pm_runtime_get_sync(), which would return -EACCES because runtime PM is not
yet enabled.

If the handler then reads from keypad->base with a disabled clock, could
this cause a bus fault?

>  	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260711-samsung-kp-v3-0-b2fcaba77aff@gmail.com?part=2