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

[Marc Zyngier]

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@xxxxxxxxxxxxxxx
> > Signed-off-by: Marc Zyngier <maz@xxxxxxxxxx>
> > ---
> >  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...