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
>
