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 [off-list ref]
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.
quoted hunk ↗ jump to hunk
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
[ ... ]
quoted hunk ↗ jump to hunk
@@ -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)
{
[ ... ]
quoted hunk ↗ jump to hunk
+ 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)
{
[ ... ]
quoted hunk ↗ jump to hunk
+ 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?
[ ... ]quoted hunk ↗ jump to hunk
@@ -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?
quoted hunk ↗ jump to hunk
- 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