Re: [PATCH v2 05/11] Input: samsung-keypad - remove support for platform data

[sashiko-bot@kernel.org]

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

New issues:
- [High] Legacy device tree properties for row and column counts are immediately overwritten and discarded by matrix_keypad_parse_properties(), breaking backward compatibility.

Pre-existing issues:
- [Critical] Stack variables `row`, `col`, and `key_code` are used uninitialized if property reads fail, potentially leading to out-of-bounds keymap generation and memory corruption.
- [High] The IRQ handler is requested before hardware clocks are enabled, leading to an external abort (system crash) if an interrupt fires early.
--

--- Patch [5]: [PATCH v2 05/11] Input: samsung-keypad - remove support for platform data ---
commit 6b84c0425d2f995a2e30b98f55d0febfc3a74cb7
Author: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Input: samsung-keypad - remove support for platform data

Because there are no more users of samsung_keypad_platdata left in
the kernel remove support for it from the driver. The driver supports
generic device properties so all configuration should be done using
them instead of a custom platform data.

> diff --git a/drivers/input/keyboard/samsung-keypad.c b/drivers/input/keyboard/samsung-keypad.c
> index b9d4ea5f202ae..6f1d766a4134d 100644
> --- a/drivers/input/keyboard/samsung-keypad.c
> +++ b/drivers/input/keyboard/samsung-keypad.c
[ ... ]
> @@ -231,144 +234,77 @@ static void samsung_keypad_close(struct input_dev *input_dev)
>  	samsung_keypad_stop(keypad);
>  }
>  
> -static const struct matrix_keymap_data *
> -samsung_parse_verbose_keymap(struct device *dev)
> +static int samsung_keypad_parse_keymap(struct samsung_keypad *keypad)
>  {
[ ... ]
> +		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);
> +			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);
> +			*keymap++ = KEY(row, col, key_code);

[Severity: Critical]
This is a pre-existing issue, but what happens if the property reads fail?

If the device tree child node is missing properties (like linux,code),
fwnode_property_read_u32() will return an error and leave row, col, and
key_code uninitialized.

Could this stack garbage be packed into the keymap array and subsequently
cause an out-of-bounds bitwise write in matrix_keypad_build_keymap() when it
calls __set_bit(code, input_dev->keybit)?

> +		}
>  	}
>  
[ ... ]
>  static int samsung_keypad_probe(struct platform_device *pdev)
>  {
[ ... ]
> +	device_property_read_u32(dev, "samsung,keypad-num-rows", &num_rows);
> +	device_property_read_u32(dev, "samsung,keypad-num-columns", &num_cols);
>  
> -	error = matrix_keypad_parse_properties(dev, &num_rows, &num_cols);
> -	if (error)
> -		return ERR_PTR(error);
> -
> -	pdata->rows = num_rows;
> -	pdata->cols = num_cols;
> -
> -	if (!device_property_present(dev, "linux,keymap")) {
> -		keymap_data = samsung_parse_verbose_keymap(dev);
> -		if (IS_ERR(keymap_data))
> -			return ERR_CAST(keymap_data);
> -
> -		pdata->keymap_data = keymap_data;
> -	}
> -
> -
> -	pdata->no_autorepeat =
> -		device_property_read_bool(dev, "linux,input-no-autorepeat");
> -
> -	pdata->wakeup = device_property_read_bool(dev, "wakeup-source") ||
> -			/* legacy name */
> -			device_property_read_bool(dev, "linux,input-wakeup");
> -
> -	return pdata;
> +	error = matrix_keypad_parse_properties(dev, &num_rows, &num_cols);
> +	if (error)
> +		return error;

[Severity: High]
Does matrix_keypad_parse_properties() overwrite the legacy properties we just
read?

Looking at matrix_keypad_parse_properties(), it unconditionally resets the
pointers passed to it:

matrix_keypad_parse_properties() {
	*rows = *cols = 0;
	device_property_read_u32(dev, "keypad,num-rows", rows);
	device_property_read_u32(dev, "keypad,num-columns", cols);
}

If a device tree uses the legacy bindings (samsung,keypad-num-rows and
samsung,keypad-num-columns) without the modern prefixes, it seems the values
read into num_rows and num_cols will be discarded here.

Since matrix_keypad_parse_properties() will return -EINVAL if the generic
properties are missing, will this cause probe to fail on existing platforms
using the older bindings?

[ ... ]
> @@ -418,7 +373,11 @@ static int samsung_keypad_probe(struct platform_device *pdev)
>  		return error;
>  	}

[Severity: High]
This is a pre-existing issue, but is the interrupt handler requested too early?

In samsung_keypad_probe(), devm_request_threaded_irq() is called just above
this block before the hardware clocks are fully enabled. During probe,
keypad->stopped is true.

If an interrupt fires immediately (for instance, if the bootloader left the
IP block active), samsung_keypad_irq() will run. While the handler calls
pm_runtime_get_sync(), the runtime resume callback skips enabling the clock
because keypad->stopped is true.

Could this result in the handler attempting an unclocked read:

samsung_keypad_irq() {
	...
	readl(keypad->base + SAMSUNG_KEYIFSTSCLR);
	...
}

and causing a synchronous external abort?

> -	device_init_wakeup(&pdev->dev, pdata->wakeup);
> +	wakeup = device_property_read_bool(dev, "wakeup-source") ||
> +		 /* legacy name */
> +		 device_property_read_bool(dev, "linux,input-wakeup");
> +	device_init_wakeup(&pdev->dev, wakeup);
> +
>  	platform_set_drvdata(pdev, keypad);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260708-samsung-kp-v2-0-3c6ed4c9b3b6@gmail.com?part=5