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

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <CAHQ1cqHCYiaEXck3LMGBwYiHVDQcF=XuF=kHJ4f_v1ea6hDR2g@mail.gmail.com>
Date:   Fri, 11 Oct 2019 12:38:45 -0700
From:   Andrey Smirnov <andrew.smirnov@...il.com>
To:     Benjamin Tissoires <benjamin.tissoires@...hat.com>
Cc:     "open list:HID CORE LAYER" <linux-input@...r.kernel.org>,
        Sam Bazely <sambazley@...tmail.com>,
        Jiri Kosina <jikos@...nel.org>,
        Henrik Rydberg <rydberg@...math.org>,
        "Pierre-Loup A . Griffais" <pgriffais@...vesoftware.com>,
        Austin Palmer <austinp@...vesoftware.com>,
        lkml <linux-kernel@...r.kernel.org>,
        "3.8+" <stable@...r.kernel.org>
Subject: Re: [PATCH 3/3] HID: logitech-hidpp: add G920 device validation quirk

On Fri, Oct 11, 2019 at 7:56 AM Benjamin Tissoires
<benjamin.tissoires@...hat.com> wrote:
>
> On Mon, Oct 7, 2019 at 7:13 AM Andrey Smirnov <andrew.smirnov@...il.com> 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@...tmail.com>
> > Signed-off-by: Andrey Smirnov <andrew.smirnov@...il.com>
> > Cc: Jiri Kosina <jikos@...nel.org>
> > Cc: Benjamin Tissoires <benjamin.tissoires@...hat.com>
> > Cc: Henrik Rydberg <rydberg@...math.org>
> > Cc: Sam Bazely <sambazley@...tmail.com>
> > Cc: Pierre-Loup A. Griffais <pgriffais@...vesoftware.com>
> > Cc: Austin Palmer <austinp@...vesoftware.com>
> > Cc: linux-input@...r.kernel.org
> > Cc: linux-kernel@...r.kernel.org
> > Cc: stable@...r.kernel.org
> > ---
> >  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?

Thanks,
Andrey Smirnov