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

Commit Message

Andrey Smirnov Oct. 7, 2019, 5:12 a.m. UTC

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 <[e-mail]>
Signed-off-by: Andrey Smirnov <[e-mail]>
Cc: Jiri Kosina <[e-mail]>
Cc: Benjamin Tissoires <[e-mail]>
Cc: Henrik Rydberg <[e-mail]>
Cc: Sam Bazely <[e-mail]>
Cc: Pierre-Loup A. Griffais <[e-mail]>
Cc: Austin Palmer <[e-mail]>
Cc: [e-mail]
Cc: [e-mail]
Cc: [e-mail]
---
 drivers/hid/hid-logitech-hidpp.c | 6 ++++++
 1 file changed, 6 insertions(+)

Comments

Benjamin Tissoires Oct. 11, 2019, 2:55 p.m. UTC | #1

On Mon, Oct 7, 2019 at 7:13 AM Andrey Smirnov <[e-mail]> 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 <[e-mail]>
> Signed-off-by: Andrey Smirnov <[e-mail]>
> Cc: Jiri Kosina <[e-mail]>
> Cc: Benjamin Tissoires <[e-mail]>
> Cc: Henrik Rydberg <[e-mail]>
> Cc: Sam Bazely <[e-mail]>
> Cc: Pierre-Loup A. Griffais <[e-mail]>
> Cc: Austin Palmer <[e-mail]>
> Cc: [e-mail]
> Cc: [e-mail]
> Cc: [e-mail]
> ---
>  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

This can probably be handled later, as your patch fixes the current devices.

Cheers,
Benjamin

>         return hidpp_validate_report(hdev, REPORT_ID_HIDPP_SHORT,
>                                      HIDPP_REPORT_SHORT_LENGTH, false) &&
>                hidpp_validate_report(hdev, REPORT_ID_HIDPP_LONG,
> --
> 2.21.0
>

Andrey Smirnov Oct. 11, 2019, 7:38 p.m. UTC | #2

On Fri, Oct 11, 2019 at 7:56 AM Benjamin Tissoires
<[e-mail]> wrote:
>
> On Mon, Oct 7, 2019 at 7:13 AM Andrey Smirnov <[e-mail]> 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 <[e-mail]>
> > Signed-off-by: Andrey Smirnov <[e-mail]>
> > Cc: Jiri Kosina <[e-mail]>
> > Cc: Benjamin Tissoires <[e-mail]>
> > Cc: Henrik Rydberg <[e-mail]>
> > Cc: Sam Bazely <[e-mail]>
> > Cc: Pierre-Loup A. Griffais <[e-mail]>
> > Cc: Austin Palmer <[e-mail]>
> > Cc: [e-mail]
> > Cc: [e-mail]
> > Cc: [e-mail]
> > ---
> >  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

Benjamin Tissoires Oct. 11, 2019, 10:32 p.m. UTC | #3

On Fri, Oct 11, 2019 at 9:39 PM Andrey Smirnov <[e-mail]> wrote:
>
> On Fri, Oct 11, 2019 at 7:56 AM Benjamin Tissoires
> <[e-mail]> wrote:
> >
> > On Mon, Oct 7, 2019 at 7:13 AM Andrey Smirnov <[e-mail]> 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 <[e-mail]>
> > > Signed-off-by: Andrey Smirnov <[e-mail]>
> > > Cc: Jiri Kosina <[e-mail]>
> > > Cc: Benjamin Tissoires <[e-mail]>
> > > Cc: Henrik Rydberg <[e-mail]>
> > > Cc: Sam Bazely <[e-mail]>
> > > Cc: Pierre-Loup A. Griffais <[e-mail]>
> > > Cc: Austin Palmer <[e-mail]>
> > > Cc: [e-mail]
> > > Cc: [e-mail]
> > > Cc: [e-mail]
> > > ---
> > >  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.

Cheers,
Benjamin

>
> Thanks,
> Andrey Smirnov

Andrey Smirnov Oct. 11, 2019, 11:32 p.m. UTC | #4

On Fri, Oct 11, 2019 at 3:33 PM Benjamin Tissoires
<[e-mail]> wrote:
>
> On Fri, Oct 11, 2019 at 9:39 PM Andrey Smirnov <[e-mail]> wrote:
> >
> > On Fri, Oct 11, 2019 at 7:56 AM Benjamin Tissoires
> > <[e-mail]> wrote:
> > >
> > > On Mon, Oct 7, 2019 at 7:13 AM Andrey Smirnov <[e-mail]> 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 <[e-mail]>
> > > > Signed-off-by: Andrey Smirnov <[e-mail]>
> > > > Cc: Jiri Kosina <[e-mail]>
> > > > Cc: Benjamin Tissoires <[e-mail]>
> > > > Cc: Henrik Rydberg <[e-mail]>
> > > > Cc: Sam Bazely <[e-mail]>
> > > > Cc: Pierre-Loup A. Griffais <[e-mail]>
> > > > Cc: Austin Palmer <[e-mail]>
> > > > Cc: [e-mail]
> > > > Cc: [e-mail]
> > > > Cc: [e-mail]
> > > > ---
> > > >  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?

Thanks,
Andrey Smirnov

Benjamin Tissoires Oct. 14, 2019, 9:47 a.m. UTC | #5

On Sat, Oct 12, 2019 at 1:33 AM Andrey Smirnov <[e-mail]> wrote:
>
> On Fri, Oct 11, 2019 at 3:33 PM Benjamin Tissoires
> <[e-mail]> wrote:
> >
> > On Fri, Oct 11, 2019 at 9:39 PM Andrey Smirnov <[e-mail]> wrote:
> > >
> > > On Fri, Oct 11, 2019 at 7:56 AM Benjamin Tissoires
> > > <[e-mail]> wrote:
> > > >
> > > > On Mon, Oct 7, 2019 at 7:13 AM Andrey Smirnov <[e-mail]> 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 <[e-mail]>
> > > > > Signed-off-by: Andrey Smirnov <[e-mail]>
> > > > > Cc: Jiri Kosina <[e-mail]>
> > > > > Cc: Benjamin Tissoires <[e-mail]>
> > > > > Cc: Henrik Rydberg <[e-mail]>
> > > > > Cc: Sam Bazely <[e-mail]>
> > > > > Cc: Pierre-Loup A. Griffais <[e-mail]>
> > > > > Cc: Austin Palmer <[e-mail]>
> > > > > Cc: [e-mail]
> > > > > Cc: [e-mail]
> > > > > Cc: [e-mail]
> > > > > ---
> > > > >  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

Patch

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);
+
 	return hidpp_validate_report(hdev, REPORT_ID_HIDPP_SHORT,
 				     HIDPP_REPORT_SHORT_LENGTH, false) &&
 	       hidpp_validate_report(hdev, REPORT_ID_HIDPP_LONG,