Re: [PATCH v2 2/3] HID: logitech-hidpp: rework device validation

[Andrey Smirnov]

On Wed, Oct 16, 2019 at 12:24 PM Benjamin Tissoires
<benjamin.tissoires@xxxxxxxxxx> wrote:
>
> Hi Andrey,
>
> On Wed, Oct 16, 2019 at 8:30 PM 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.
> >
> > To fix this and improve some other aspects, modify
> > hidpp_validate_device() as follows:
> >
> >   - Inline the code of hidpp_validate_report() to simplify
> >     distingushing between non-present and invalid report descriptors
> >
> >   - Drop the check for id >= HID_MAX_IDS || id < 0 since all of our
> >     IDs are static and known to satisfy that at compile time
> >
> >   - Change the algorithms to check all possible report
> >     types (including very long report) and deem the device as a valid
> >     HID++ device if it supports at least one
> >
> >   - Treat invalid report length as a hard stop for the validation
> >     algorithm, meaning that if any of the supported reports has
> >     invalid length we assume the worst and treat the device as a
> >     generic HID device.
> >
> >   - Fold initialization of hidpp->very_long_report_length into
> >     hidpp_validate_device() since it already fetches very long report
> >     length and validates its value
> >
> > 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: Pierre-Loup A. Griffais <pgriffais@xxxxxxxxxxxxxxxxx>
> > Cc: Austin Palmer <austinp@xxxxxxxxxxxxxxxxx>
> > Cc: linux-input@xxxxxxxxxxxxxxx
> > Cc: linux-kernel@xxxxxxxxxxxxxxx
> > Cc: stable@xxxxxxxxxxxxxxx # 5.2+
> > ---
> >  drivers/hid/hid-logitech-hidpp.c | 54 ++++++++++++++++++--------------
> >  1 file changed, 30 insertions(+), 24 deletions(-)
> >
> > diff --git a/drivers/hid/hid-logitech-hidpp.c b/drivers/hid/hid-logitech-hidpp.c
> > index 85911586b3b6..8c4be991f387 100644
> > --- a/drivers/hid/hid-logitech-hidpp.c
> > +++ b/drivers/hid/hid-logitech-hidpp.c
> > @@ -3498,34 +3498,45 @@ static int hidpp_get_report_length(struct hid_device *hdev, int id)
> >         return report->field[0]->report_count + 1;
> >  }
> >
> > -static bool hidpp_validate_report(struct hid_device *hdev, int id,
> > -                                 int expected_length, bool optional)
> > +static bool hidpp_validate_device(struct hid_device *hdev)
> >  {
> > -       int report_length;
> > +       struct hidpp_device *hidpp = hid_get_drvdata(hdev);
> > +       int id, report_length, supported_reports = 0;
> > +
> > +       id = REPORT_ID_HIDPP_SHORT;
> > +       report_length = hidpp_get_report_length(hdev, id);
> > +       if (report_length) {
> > +               if (report_length < HIDPP_REPORT_SHORT_LENGTH)
> > +                       goto bad_device;
> >
> > -       if (id >= HID_MAX_IDS || id < 0) {
> > -               hid_err(hdev, "invalid HID report id %u\n", id);
> > -               return false;
> > +               supported_reports++;
> >         }
> >
> > +       id = REPORT_ID_HIDPP_LONG;
> >         report_length = hidpp_get_report_length(hdev, id);
> > -       if (!report_length)
> > -               return optional;
> > +       if (report_length) {
> > +               if (report_length < HIDPP_REPORT_LONG_LENGTH)
> > +                       goto bad_device;
> >
> > -       if (report_length < expected_length) {
> > -               hid_warn(hdev, "not enough values in hidpp report %d\n", id);
> > -               return false;
> > +               supported_reports++;
> >         }
> >
> > -       return true;
> > -}
> > +       id = REPORT_ID_HIDPP_VERY_LONG;
> > +       report_length = hidpp_get_report_length(hdev, id);
> > +       if (report_length) {
> > +               if (report_length > HIDPP_REPORT_LONG_LENGTH &&
> > +                   report_length < HIDPP_REPORT_VERY_LONG_MAX_LENGTH)
>
> Can you double check the conditions here?
> It's late, but I think you inverted the tests as we expect the report
> length to be between HIDPP_REPORT_LONG_LENGTH and
> HIDPP_REPORT_VERY_LONG_MAX_LENGTH inclusive, while here this creates a
> bad_device.

Hmm, I think you are right. Not sure why I didn't catch it on G920
since it support very long reports AFAIR. Will re-spin and double
check in v3. Sorry about that.

Thanks,
Andrey Smirnov