Re: [PATCH v4] HID: core: Sanitize event code and type when mapping input

[Benjamin Tissoires]

On Tue, Sep 1, 2020 at 11:52 AM Marc Zyngier <maz@xxxxxxxxxx> wrote:
>
> When calling into hid_map_usage(), the passed event code is
> blindly stored as is, even if it doesn't fit in the associated bitmap.
>
> This event code can come from a variety of sources, including devices
> masquerading as input devices, only a bit more "programmable".
>
> Instead of taking the event code at face value, check that it actually
> fits the corresponding bitmap, and if it doesn't:
> - spit out a warning so that we know which device is acting up
> - NULLify the bitmap pointer so that we catch unexpected uses
>
> Code paths that can make use of untrusted inputs can now check
> that the mapping was indeed correct and bail out if not.
>
> Cc: stable@xxxxxxxxxxxxxxx
> Signed-off-by: Marc Zyngier <maz@xxxxxxxxxx>
> ---

Pushed to for-5.9/upstream-fixes

Cheers,
Benjamin

> * From v3:
>   - Drop totally unrelated mfd/syscon change from the patch
>
> * From v2:
>   - Don't prematurely narrow the event code so that hid_map_usage()
>     catches illegal values beyond the 16bit limit.
>
> * From v1:
>   - Dropped the input.c changes, and turned hid_map_usage() into
>     the validation primitive.
>   - Handle mapping failures in hidinput_configure_usage() and
>     mt_touch_input_mapping() (on top of hid_map_usage_clear() which
>     was already handled)
>
>  drivers/hid/hid-input.c      |  4 ++++
>  drivers/hid/hid-multitouch.c |  2 ++
>  include/linux/hid.h          | 42 +++++++++++++++++++++++++-----------
>  3 files changed, 35 insertions(+), 13 deletions(-)
>
> diff --git a/drivers/hid/hid-input.c b/drivers/hid/hid-input.c
> index b8eabf206e74..88e19996427e 100644
> --- a/drivers/hid/hid-input.c
> +++ b/drivers/hid/hid-input.c
> @@ -1132,6 +1132,10 @@ static void hidinput_configure_usage(struct hid_input *hidinput, struct hid_fiel
>         }
>
>  mapped:
> +       /* Mapping failed, bail out */
> +       if (!bit)
> +               return;
> +
>         if (device->driver->input_mapped &&
>             device->driver->input_mapped(device, hidinput, field, usage,
>                                          &bit, &max) < 0) {
> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
> index 3f94b4954225..e3152155c4b8 100644
> --- a/drivers/hid/hid-multitouch.c
> +++ b/drivers/hid/hid-multitouch.c
> @@ -856,6 +856,8 @@ static int mt_touch_input_mapping(struct hid_device *hdev, struct hid_input *hi,
>                         code = BTN_0  + ((usage->hid - 1) & HID_USAGE);
>
>                 hid_map_usage(hi, usage, bit, max, EV_KEY, code);
> +               if (!*bit)
> +                       return -1;
>                 input_set_capability(hi->input, EV_KEY, code);
>                 return 1;
>
> diff --git a/include/linux/hid.h b/include/linux/hid.h
> index 875f71132b14..c7044a14200e 100644
> --- a/include/linux/hid.h
> +++ b/include/linux/hid.h
> @@ -959,34 +959,49 @@ static inline void hid_device_io_stop(struct hid_device *hid) {
>   * @max: maximal valid usage->code to consider later (out parameter)
>   * @type: input event type (EV_KEY, EV_REL, ...)
>   * @c: code which corresponds to this usage and type
> + *
> + * The value pointed to by @bit will be set to NULL if either @type is
> + * an unhandled event type, or if @c is out of range for @type. This
> + * can be used as an error condition.
>   */
>  static inline void hid_map_usage(struct hid_input *hidinput,
>                 struct hid_usage *usage, unsigned long **bit, int *max,
> -               __u8 type, __u16 c)
> +               __u8 type, unsigned int c)
>  {
>         struct input_dev *input = hidinput->input;
> -
> -       usage->type = type;
> -       usage->code = c;
> +       unsigned long *bmap = NULL;
> +       unsigned int limit = 0;
>
>         switch (type) {
>         case EV_ABS:
> -               *bit = input->absbit;
> -               *max = ABS_MAX;
> +               bmap = input->absbit;
> +               limit = ABS_MAX;
>                 break;
>         case EV_REL:
> -               *bit = input->relbit;
> -               *max = REL_MAX;
> +               bmap = input->relbit;
> +               limit = REL_MAX;
>                 break;
>         case EV_KEY:
> -               *bit = input->keybit;
> -               *max = KEY_MAX;
> +               bmap = input->keybit;
> +               limit = KEY_MAX;
>                 break;
>         case EV_LED:
> -               *bit = input->ledbit;
> -               *max = LED_MAX;
> +               bmap = input->ledbit;
> +               limit = LED_MAX;
>                 break;
>         }
> +
> +       if (unlikely(c > limit || !bmap)) {
> +               pr_warn_ratelimited("%s: Invalid code %d type %d\n",
> +                                   input->name, c, type);
> +               *bit = NULL;
> +               return;
> +       }
> +
> +       usage->type = type;
> +       usage->code = c;
> +       *max = limit;
> +       *bit = bmap;
>  }
>
>  /**
> @@ -1000,7 +1015,8 @@ static inline void hid_map_usage_clear(struct hid_input *hidinput,
>                 __u8 type, __u16 c)
>  {
>         hid_map_usage(hidinput, usage, bit, max, type, c);
> -       clear_bit(c, *bit);
> +       if (*bit)
> +               clear_bit(usage->code, *bit);
>  }
>
>  /**
> --
> 2.27.0
>