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) + 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;
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
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
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
linux-stable-mirror@lists.linaro.org
-
Andrey Smirnov -
Benjamin Tissoires