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

[Marc Zyngier <maz@kernel.org>]

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...