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 <[email protected]>
Signed-off-by: Andrey Smirnov <[email protected]>
Cc: Jiri Kosina <[email protected]>
Cc: Benjamin Tissoires <[email protected]>
Cc: Henrik Rydberg <[email protected]>
Cc: Pierre-Loup A. Griffais <[email protected]>
Cc: Austin Palmer <[email protected]>
Cc: [email protected]
Cc: [email protected]
Cc: [email protected] # 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)
+ 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
Hi Andrey,
On Wed, Oct 16, 2019 at 8:30 PM Andrey Smirnov <[email protected]> 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 <[email protected]>
> Signed-off-by: Andrey Smirnov <[email protected]>
> Cc: Jiri Kosina <[email protected]>
> Cc: Benjamin Tissoires <[email protected]>
> Cc: Henrik Rydberg <[email protected]>
> Cc: Pierre-Loup A. Griffais <[email protected]>
> Cc: Austin Palmer <[email protected]>
> Cc: [email protected]
> Cc: [email protected]
> Cc: [email protected] # 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
>
On Wed, Oct 16, 2019 at 12:24 PM Benjamin Tissoires
<[email protected]> wrote:
>
> Hi Andrey,
>
> On Wed, Oct 16, 2019 at 8:30 PM Andrey Smirnov <[email protected]> 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 <[email protected]>
> > Signed-off-by: Andrey Smirnov <[email protected]>
> > Cc: Jiri Kosina <[email protected]>
> > Cc: Benjamin Tissoires <[email protected]>
> > Cc: Henrik Rydberg <[email protected]>
> > Cc: Pierre-Loup A. Griffais <[email protected]>
> > Cc: Austin Palmer <[email protected]>
> > Cc: [email protected]
> > Cc: [email protected]
> > Cc: [email protected] # 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
On Wed, Oct 16, 2019 at 9:38 PM Andrey Smirnov <[email protected]> wrote:
>
> On Wed, Oct 16, 2019 at 12:24 PM Benjamin Tissoires
> <[email protected]> wrote:
> >
> > Hi Andrey,
> >
> > On Wed, Oct 16, 2019 at 8:30 PM Andrey Smirnov <[email protected]> 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 <[email protected]>
> > > Signed-off-by: Andrey Smirnov <[email protected]>
> > > Cc: Jiri Kosina <[email protected]>
> > > Cc: Benjamin Tissoires <[email protected]>
> > > Cc: Henrik Rydberg <[email protected]>
> > > Cc: Pierre-Loup A. Griffais <[email protected]>
> > > Cc: Austin Palmer <[email protected]>
> > > Cc: [email protected]
> > > Cc: [email protected]
> > > Cc: [email protected] # 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