Re: Detecting invalid gpio keys configuration

Hi Tomasz,

On Thu, Mar 10, 2022 at 07:57:53AM +0100, Tomasz Moń wrote:

Hello,

I have recently come across following invalid entry:
	gpio-keys {
		compatible = "gpio-keys";
		servicing-sw {
			label = "servicing-sw";
			gpios = <&gpio3 1 GPIO_ACTIVE_LOW>;
			linux,code = <BTN_0>;
			linux,input-type = <EV_SW>;
			debounce-interval = <50>;
		};
	};

While the entry is clearly invalid, no tools currently detect it.
Documentation/devicetree/bindings/input/gpio-keys.yaml allows any uint32
as linux,code. The actual allowed values do depend on linux,input-type.
Should the gpio-keys.yaml be updated to actually restrict the range
based on linux,input-type?

If the yaml should validate range, it would have to be updated each time
new input event code is added. Is it acceptable? Or is there some way to
use the definitions from include/uapi/linux/input-event-codes.h in yaml?

The code does not sanitize the linux,code values. On 32-bit arm system,
the above example causes kernel panic due to memory corruption:
  * gpio_keys_setup_key() in drivers/input/keyboard/gpio_keys.c calls
    input_set_capability(input, EV_SW, BTN_0)
  * input_set_capability() in drivers/input/input.c calls
    __set_bit(BTN_0, dev->swbit)
  * The setbit changes poller pointer least significant bit
  * input_register_device() calls input_dev_poller_finalize(dev->poller)
  * input_dev_poller_finalize() accesses memory at address 0x00000005

While provided example hopefully crashes every time making the issue
obvious, other code values can cause much more subtle issues.
Should the input_set_capability() warn if code is outside bitmap range?

Best Regards,
Tomasz Mon

What about something like the patch below? It of course can't prevent
dts from specifying something like KEY_ESC as a switch, but should at
least prevent kernel panic.

Kind regards,
Jeff LaBundy

diff --git a/drivers/input/input.c b/drivers/input/input.c
index 4456e82d370b..e5a668ce884d 100644
--- a/drivers/input/input.c
+++ b/drivers/input/input.c

@@ -47,6 +47,17 @@ static DEFINE_MUTEX(input_mutex);
 
 static const struct input_value input_value_sync = { EV_SYN, SYN_REPORT, 1 };
 
+static const unsigned int input_max_code[EV_CNT] = {
+	[EV_KEY] = KEY_MAX,
+	[EV_REL] = REL_MAX,
+	[EV_ABS] = ABS_MAX,
+	[EV_MSC] = MSC_MAX,
+	[EV_SW] = SW_MAX,
+	[EV_LED] = LED_MAX,
+	[EV_SND] = SND_MAX,
+	[EV_FF] = FF_MAX,
+};
+
 static inline int is_event_supported(unsigned int code,
 				     unsigned long *bm, unsigned int max)
 {

@@ -2110,6 +2121,14 @@ EXPORT_SYMBOL(input_get_timestamp);
  */
 void input_set_capability(struct input_dev *dev, unsigned int type, unsigned int code)
 {
+	if (type < EV_CNT && input_max_code[type] &&
+	    code > input_max_code[type]) {
+		pr_err("%s: invalid code %u for type %u\n", __func__, code,
+		       type);
+		dump_stack();
+		return;
+	}
+
 	switch (type) {
 	case EV_KEY:
 		__set_bit(code, dev->keybit);