Re: [PATCH 3/3] HID: logitech-hidpp: add G920 device validation quirk

[Benjamin Tissoires]

On Sat, Oct 12, 2019 at 1:33 AM Andrey Smirnov <andrew.smirnov@xxxxxxxxx> wrote:
>
> On Fri, Oct 11, 2019 at 3:33 PM Benjamin Tissoires
> <benjamin.tissoires@xxxxxxxxxx> wrote:
> >
> > On Fri, Oct 11, 2019 at 9:39 PM Andrey Smirnov <andrew.smirnov@xxxxxxxxx> wrote:
> > >
> > > On Fri, Oct 11, 2019 at 7:56 AM Benjamin Tissoires
> > > <benjamin.tissoires@xxxxxxxxxx> wrote:
> > > >
> > > > On Mon, Oct 7, 2019 at 7:13 AM Andrey Smirnov <andrew.smirnov@xxxxxxxxx> wrote:
> > > > >
> > > > > G920 device only advertises REPORT_ID_HIDPP_LONG and
> > > > > REPORT_ID_HIDPP_VERY_LONG in its HID report descriptor, so querying
> > > > > for REPORT_ID_HIDPP_SHORT with optional=false will always fail and
> > > > > prevent G920 to be recognized as a valid HID++ device.
> > > > >
> > > > > Modify hidpp_validate_device() to check only REPORT_ID_HIDPP_LONG with
> > > > > optional=false on G920 to fix this.
> > > > >
> > > > > Fixes: fe3ee1ec007b ("HID: logitech-hidpp: allow non HID++ devices to be handled by this module")
> > > > > Bugzilla: https://bugzilla.kernel.org/show_bug.cgi?id=204191
> > > > > Reported-by: Sam Bazely <sambazley@xxxxxxxxxxxx>
> > > > > Signed-off-by: Andrey Smirnov <andrew.smirnov@xxxxxxxxx>
> > > > > Cc: Jiri Kosina <jikos@xxxxxxxxxx>
> > > > > Cc: Benjamin Tissoires <benjamin.tissoires@xxxxxxxxxx>
> > > > > Cc: Henrik Rydberg <rydberg@xxxxxxxxxxx>
> > > > > Cc: Sam Bazely <sambazley@xxxxxxxxxxxx>
> > > > > Cc: Pierre-Loup A. Griffais <pgriffais@xxxxxxxxxxxxxxxxx>
> > > > > Cc: Austin Palmer <austinp@xxxxxxxxxxxxxxxxx>
> > > > > Cc: linux-input@xxxxxxxxxxxxxxx
> > > > > Cc: linux-kernel@xxxxxxxxxxxxxxx
> > > > > Cc: stable@xxxxxxxxxxxxxxx
> > > > > ---
> > > > >  drivers/hid/hid-logitech-hidpp.c | 6 ++++++
> > > > >  1 file changed, 6 insertions(+)
> > > > >
> > > > > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> > > > > index cadf36d6c6f3..f415bf398e17 100644
> > > > > --- a/drivers/hid/hid-logitech-hidpp.c
> > > > > +++ b/drivers/hid/hid-logitech-hidpp.c
> > > > > @@ -3511,6 +3511,12 @@ static bool hidpp_validate_report(struct hid_device *hdev, int id,
> > > > >
> > > > >  static bool hidpp_validate_device(struct hid_device *hdev)
> > > > >  {
> > > > > +       struct hidpp_device *hidpp = hid_get_drvdata(hdev);
> > > > > +
> > > > > +       if (hidpp->quirks & HIDPP_QUIRK_CLASS_G920)
> > > > > +               return hidpp_validate_report(hdev, REPORT_ID_HIDPP_LONG,
> > > > > +                                            HIDPP_REPORT_SHORT_LENGTH, false);
> > > > > +
> > > >
> > > > with https://patchwork.kernel.org/patch/11184749/ we also have a need
> > > > for such a trick for BLE mice.
> > > >
> > > > I wonder if we should not have a more common way of validating the devices
> > > >
> > >
> > > What about just checking for:
> > >
> > > hidpp_validate_report(REPORT_ID_HIDPP_SHORT,
> > >                                     HIDPP_REPORT_SHORT_LENGTH, true) ||
> > > hidpp_validate_report(hdev, REPORT_ID_HIDPP_LONG,
> > >                                     HIDPP_REPORT_LONG_LENGTH, true);
> > >
> > > and probably dropping the "optional" argument for
> > > hidpp_validate_report()? Original code allows there to be devices
> > > supporting shorts reports only, but it seems that devices that support
> > > only long reports are legitimate too, so maybe the only "invalid"
> > > combination is if both are invalid length or missing?
> >
> > Well, the problem is we also want to detect 2 things:
> > - devices that do not have any of the HID++ collections, and handle
> > them as generic ones (the second mouse/keyboard collection in the
> > gaming mice should still be exported by the driver, or this will kill
> > the macros / rebinding capabilities
> > - malicious devices that pretends to have a HID++ collection but want
> > to trigger a buffer overflow by having a shorter than expected report
> > length
> >
> > Point 2 above should still be fine, but point 1 is why we have the
> > enforcement of the HID++ short report in the first place.
> >
>
> It sounds like the result of hidpp_validate_report() can't really be
> contained in a bool. If we modify it to return -EINVAL for bogus
> report length, -ENOTSUPP if report ID is not supported and 0 if
> everything is valid we should be able to capture all valid permutation
> by checking for with
>
> int id_short = hidpp_validate_report(ID_SHORT);
> int id_long  = hidpp_validate_report(ID_LONG);
>
> return (!id_short && !id_long) || (id_short == -ENOTSUPP && !id_long)
> || (id_long == -ENOTSUPP && !id_short)
>
> no?

Sounds like a good idea.

There is a few changes I'd like to do on this proposal:
- ideally, we should also check on very long HID++ reports, but as
d71b18f7c79993 ("HID: logitech-hidpp: do not hardcode very long report
length") mentioned, we should probably ensure that those very long
reports are at least bigger than HIDPP_REPORT_LONG_LENGTH and shorter
than HIDPP_REPORT_VERY_LONG_MAX_LENGTH.

- the boolean operation has a risk of being quite hard to follow if we
now have 3 IDs to check. So we would need a few comments for each
operation to explain which is which.

- maybe we should exit earlier if any of the id_short, id_long or
id_very_long is -EINVAL, as this should be detected has a hard failure

- the choice of the return codes should be changed:
  * ENOTSUPP is defined for the NFSv3 protocol, and I think ENOENT (no
such file or directory) or ENXIO (No such device or address) might be
better
  * EINVAL is for an invalid argument of a function, and here the
argument is correct, but the device is not. Maybe EBADR (Invalid
request descriptor)?

If we can get this patch in stable soon, this would also help of the
BLE series Mazin is currently working on.

Cheers,
Benjamin