[v2,2/3] HID: logitech-hidpp: rework device validation
diff mbox series

Message ID 20191016182935.5616-3-andrew.smirnov@gmail.com
State Superseded
Delegated to: Jiri Kosina
Headers show
Series
  • Logitech G920 fixes
Related show

Commit Message

Andrey Smirnov Oct. 16, 2019, 6:29 p.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.

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@fastmail.com>
Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
Cc: Jiri Kosina <jikos@kernel.org>
Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
Cc: Henrik Rydberg <rydberg@bitmath.org>
Cc: Pierre-Loup A. Griffais <pgriffais@valvesoftware.com>
Cc: Austin Palmer <austinp@valvesoftware.com>
Cc: linux-input@vger.kernel.org
Cc: linux-kernel@vger.kernel.org
Cc: stable@vger.kernel.org # 5.2+
---
 drivers/hid/hid-logitech-hidpp.c | 54 ++++++++++++++++++--------------
 1 file changed, 30 insertions(+), 24 deletions(-)

Comments

Benjamin Tissoires Oct. 16, 2019, 7:24 p.m. UTC | #1
Hi Andrey,

On Wed, Oct 16, 2019 at 8:30 PM Andrey Smirnov <andrew.smirnov@gmail.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.
>
> 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@fastmail.com>
> Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> Cc: Jiri Kosina <jikos@kernel.org>
> Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> Cc: Henrik Rydberg <rydberg@bitmath.org>
> Cc: Pierre-Loup A. Griffais <pgriffais@valvesoftware.com>
> Cc: Austin Palmer <austinp@valvesoftware.com>
> Cc: linux-input@vger.kernel.org
> Cc: linux-kernel@vger.kernel.org
> Cc: stable@vger.kernel.org # 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.

Other than that, I really like the series.

Cheers,
Benjamin

> +                       goto bad_device;
>
> -static bool hidpp_validate_device(struct hid_device *hdev)
> -{
> -       return hidpp_validate_report(hdev, REPORT_ID_HIDPP_SHORT,
> -                                    HIDPP_REPORT_SHORT_LENGTH, false) &&
> -              hidpp_validate_report(hdev, REPORT_ID_HIDPP_LONG,
> -                                    HIDPP_REPORT_LONG_LENGTH, true);
> +               supported_reports++;
> +               hidpp->very_long_report_length = report_length;
> +       }
> +
> +       return supported_reports;
> +
> +bad_device:
> +       hid_warn(hdev, "not enough values in hidpp report %d\n", id);
> +       return false;
>  }
>
>  static bool hidpp_application_equals(struct hid_device *hdev,
> @@ -3572,11 +3583,6 @@ static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
>                 return hid_hw_start(hdev, HID_CONNECT_DEFAULT);
>         }
>
> -       hidpp->very_long_report_length =
> -               hidpp_get_report_length(hdev, REPORT_ID_HIDPP_VERY_LONG);
> -       if (hidpp->very_long_report_length > HIDPP_REPORT_VERY_LONG_MAX_LENGTH)
> -               hidpp->very_long_report_length = HIDPP_REPORT_VERY_LONG_MAX_LENGTH;
> -
>         if (id->group == HID_GROUP_LOGITECH_DJ_DEVICE)
>                 hidpp->quirks |= HIDPP_QUIRK_UNIFYING;
>
> --
> 2.21.0
>
Andrey Smirnov Oct. 16, 2019, 7:38 p.m. UTC | #2
On Wed, Oct 16, 2019 at 12:24 PM Benjamin Tissoires
<benjamin.tissoires@redhat.com> wrote:
>
> Hi Andrey,
>
> On Wed, Oct 16, 2019 at 8:30 PM Andrey Smirnov <andrew.smirnov@gmail.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.
> >
> > 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@fastmail.com>
> > Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> > Cc: Jiri Kosina <jikos@kernel.org>
> > Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> > Cc: Henrik Rydberg <rydberg@bitmath.org>
> > Cc: Pierre-Loup A. Griffais <pgriffais@valvesoftware.com>
> > Cc: Austin Palmer <austinp@valvesoftware.com>
> > Cc: linux-input@vger.kernel.org
> > Cc: linux-kernel@vger.kernel.org
> > Cc: stable@vger.kernel.org # 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
Benjamin Tissoires Oct. 17, 2019, 8:48 a.m. UTC | #3
On Wed, Oct 16, 2019 at 9:38 PM Andrey Smirnov <andrew.smirnov@gmail.com> wrote:
>
> On Wed, Oct 16, 2019 at 12:24 PM Benjamin Tissoires
> <benjamin.tissoires@redhat.com> wrote:
> >
> > Hi Andrey,
> >
> > On Wed, Oct 16, 2019 at 8:30 PM Andrey Smirnov <andrew.smirnov@gmail.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.
> > >
> > > 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@fastmail.com>
> > > Signed-off-by: Andrey Smirnov <andrew.smirnov@gmail.com>
> > > Cc: Jiri Kosina <jikos@kernel.org>
> > > Cc: Benjamin Tissoires <benjamin.tissoires@redhat.com>
> > > Cc: Henrik Rydberg <rydberg@bitmath.org>
> > > Cc: Pierre-Loup A. Griffais <pgriffais@valvesoftware.com>
> > > Cc: Austin Palmer <austinp@valvesoftware.com>
> > > Cc: linux-input@vger.kernel.org
> > > Cc: linux-kernel@vger.kernel.org
> > > Cc: stable@vger.kernel.org # 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.
>

Oh, the issue is that the very long HID++ reports on the G920 are
HIDPP_REPORT_VERY_LONG_MAX_LENGTH long, which means the test will fail
for those.

Cheers,
Benjamin

Patch
diff mbox series

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)
+			goto bad_device;
 
-static bool hidpp_validate_device(struct hid_device *hdev)
-{
-	return hidpp_validate_report(hdev, REPORT_ID_HIDPP_SHORT,
-				     HIDPP_REPORT_SHORT_LENGTH, false) &&
-	       hidpp_validate_report(hdev, REPORT_ID_HIDPP_LONG,
-				     HIDPP_REPORT_LONG_LENGTH, true);
+		supported_reports++;
+		hidpp->very_long_report_length = report_length;
+	}
+
+	return supported_reports;
+
+bad_device:
+	hid_warn(hdev, "not enough values in hidpp report %d\n", id);
+	return false;
 }
 
 static bool hidpp_application_equals(struct hid_device *hdev,
@@ -3572,11 +3583,6 @@  static int hidpp_probe(struct hid_device *hdev, const struct hid_device_id *id)
 		return hid_hw_start(hdev, HID_CONNECT_DEFAULT);
 	}
 
-	hidpp->very_long_report_length =
-		hidpp_get_report_length(hdev, REPORT_ID_HIDPP_VERY_LONG);
-	if (hidpp->very_long_report_length > HIDPP_REPORT_VERY_LONG_MAX_LENGTH)
-		hidpp->very_long_report_length = HIDPP_REPORT_VERY_LONG_MAX_LENGTH;
-
 	if (id->group == HID_GROUP_LOGITECH_DJ_DEVICE)
 		hidpp->quirks |= HIDPP_QUIRK_UNIFYING;