diff mbox series

[1/2] Input; Sanitize event code before modifying bitmaps

Message ID 20200817112700.468743-2-maz@kernel.org
State New
Headers show
Series input/hid: Fix bitmap boundary validation | expand

Commit Message

Marc Zyngier Aug. 17, 2020, 11:26 a.m. UTC
When calling into input_set_capability(), the passed event code is
blindly used to set a bit in a number of bitmaps, without checking
whether this actually fits the expected size of the 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 raw event code, sanitize it to the actual bitmap
size and output a warning to let the user know.

These checks are, at least in spirit, in keeping with cb222aed03d7
("Input: add safety guards to input_set_keycode()").

Cc: stable@vger.kernel.org
Signed-off-by: Marc Zyngier <maz@kernel.org>
---
 drivers/input/input.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)

Comments

Dmitry Torokhov Aug. 24, 2020, 7:51 p.m. UTC | #1
Hi Marc,

On Mon, Aug 17, 2020 at 12:26:59PM +0100, Marc Zyngier wrote:
> When calling into input_set_capability(), the passed event code is
> blindly used to set a bit in a number of bitmaps, without checking
> whether this actually fits the expected size of the 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 raw event code, sanitize it to the actual bitmap
> size and output a warning to let the user know.
> 
> These checks are, at least in spirit, in keeping with cb222aed03d7
> ("Input: add safety guards to input_set_keycode()").
> 
> Cc: stable@vger.kernel.org
> Signed-off-by: Marc Zyngier <maz@kernel.org>
> ---
>  drivers/input/input.c | 16 +++++++++++++++-
>  1 file changed, 15 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/input/input.c b/drivers/input/input.c
> index 3cfd2c18eebd..1e77cf47aa44 100644
> --- a/drivers/input/input.c
> +++ b/drivers/input/input.c
> @@ -1974,14 +1974,18 @@ EXPORT_SYMBOL(input_get_timestamp);
>   * In addition to setting up corresponding bit in appropriate capability
>   * bitmap the function also adjusts dev->evbit.
>   */
> -void input_set_capability(struct input_dev *dev, unsigned int type, unsigned int code)
> +void input_set_capability(struct input_dev *dev, unsigned int type, unsigned int raw_code)
>  {
> +	unsigned int code = raw_code;
> +
>  	switch (type) {
>  	case EV_KEY:
> +		code &= KEY_MAX;
>  		__set_bit(code, dev->keybit);


I would much rather prefer we did not simply set some random bits in
this case, but instead complained loudly and refused to alter anything.

The function is not exported directly to userspace, so we expect callers
to give us sane inputs, and I believe WARN() splash in case of bad
inputs would be the best solution here.

Thanks.
Marc Zyngier Aug. 26, 2020, 1:32 p.m. UTC | #2
Hi Dmitry,

On 2020-08-24 20:51, Dmitry Torokhov wrote:
> Hi Marc,
> 
> On Mon, Aug 17, 2020 at 12:26:59PM +0100, Marc Zyngier wrote:
>> When calling into input_set_capability(), the passed event code is
>> blindly used to set a bit in a number of bitmaps, without checking
>> whether this actually fits the expected size of the 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 raw event code, sanitize it to the actual bitmap
>> size and output a warning to let the user know.
>> 
>> These checks are, at least in spirit, in keeping with cb222aed03d7
>> ("Input: add safety guards to input_set_keycode()").
>> 
>> Cc: stable@vger.kernel.org
>> Signed-off-by: Marc Zyngier <maz@kernel.org>
>> ---
>>  drivers/input/input.c | 16 +++++++++++++++-
>>  1 file changed, 15 insertions(+), 1 deletion(-)
>> 
>> diff --git a/drivers/input/input.c b/drivers/input/input.c
>> index 3cfd2c18eebd..1e77cf47aa44 100644
>> --- a/drivers/input/input.c
>> +++ b/drivers/input/input.c
>> @@ -1974,14 +1974,18 @@ EXPORT_SYMBOL(input_get_timestamp);
>>   * In addition to setting up corresponding bit in appropriate 
>> capability
>>   * bitmap the function also adjusts dev->evbit.
>>   */
>> -void input_set_capability(struct input_dev *dev, unsigned int type, 
>> unsigned int code)
>> +void input_set_capability(struct input_dev *dev, unsigned int type, 
>> unsigned int raw_code)
>>  {
>> +	unsigned int code = raw_code;
>> +
>>  	switch (type) {
>>  	case EV_KEY:
>> +		code &= KEY_MAX;
>>  		__set_bit(code, dev->keybit);
> 
> 
> I would much rather prefer we did not simply set some random bits in
> this case, but instead complained loudly and refused to alter anything.
> 
> The function is not exported directly to userspace, so we expect 
> callers
> to give us sane inputs, and I believe WARN() splash in case of bad
> inputs would be the best solution here.

Fair enough. I've moved the checking to the HID layer (using
hid_map_usage() as the validation primitive), which makes
changing input_set_capability() irrelevant.

I'll post v2 shortly in the form of a single patch.

Thanks,

         M.
diff mbox series

Patch

diff --git a/drivers/input/input.c b/drivers/input/input.c
index 3cfd2c18eebd..1e77cf47aa44 100644
--- a/drivers/input/input.c
+++ b/drivers/input/input.c
@@ -1974,14 +1974,18 @@  EXPORT_SYMBOL(input_get_timestamp);
  * In addition to setting up corresponding bit in appropriate capability
  * bitmap the function also adjusts dev->evbit.
  */
-void input_set_capability(struct input_dev *dev, unsigned int type, unsigned int code)
+void input_set_capability(struct input_dev *dev, unsigned int type, unsigned int raw_code)
 {
+	unsigned int code = raw_code;
+
 	switch (type) {
 	case EV_KEY:
+		code &= KEY_MAX;
 		__set_bit(code, dev->keybit);
 		break;
 
 	case EV_REL:
+		code &= REL_MAX;
 		__set_bit(code, dev->relbit);
 		break;
 
@@ -1990,26 +1994,32 @@  void input_set_capability(struct input_dev *dev, unsigned int type, unsigned int
 		if (!dev->absinfo)
 			return;
 
+		code &= ABS_MAX;
 		__set_bit(code, dev->absbit);
 		break;
 
 	case EV_MSC:
+		code &= MSC_MAX;
 		__set_bit(code, dev->mscbit);
 		break;
 
 	case EV_SW:
+		code &= SW_MAX;
 		__set_bit(code, dev->swbit);
 		break;
 
 	case EV_LED:
+		code &= LED_MAX;
 		__set_bit(code, dev->ledbit);
 		break;
 
 	case EV_SND:
+		code &= SND_MAX;
 		__set_bit(code, dev->sndbit);
 		break;
 
 	case EV_FF:
+		code &= FF_MAX;
 		__set_bit(code, dev->ffbit);
 		break;
 
@@ -2023,6 +2033,10 @@  void input_set_capability(struct input_dev *dev, unsigned int type, unsigned int
 		return;
 	}
 
+	if (unlikely(code != raw_code))
+		pr_warn_ratelimited("%s: Truncated code %d to %d for type %d\n",
+				    dev->name, raw_code, code, type);
+
 	__set_bit(type, dev->evbit);
 }
 EXPORT_SYMBOL(input_set_capability);