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

Marc Zyngier
2020-08-26T13:33:16Z

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.
--
Jazz is not dead. It just smells funny...