Detecting invalid gpio keys configuration

Detecting invalid gpio keys configuration

[Tomasz Moń]

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

Re: Detecting invalid gpio keys configuration

[Jeff LaBundy]

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);

Re: Detecting invalid gpio keys configuration

[Tomasz Moń]

On 12.03.2022 02:11, Jeff LaBundy wrote:
> Hi Tomasz,

Hi Jeff,

> On Thu, Mar 10, 2022 at 07:57:53AM +0100, Tomasz Moń wrote:
>> 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.

Your patch does prevent the kernel panic. On top of that, it also makes
troubleshooting much easier. That is, directly pinpointing the issue in
error message essentially skips the debugging part.

I think this is the only place where a code check is missing. The other
places do already use is_event_supported() that checks if code is below
or equal to the maximum allowed.

So I am in favor of your patch.

Reviewed-by: Tomasz Moń <tomasz.mon@camlingroup.com>

Best Regards,
Tomasz Mon