From: Benjamin Tissoires <benjamin.tissoires@redhat.com>
To: Andrey Smirnov <andrew.smirnov@gmail.com>
Cc: "open list:HID CORE LAYER" <linux-input@vger.kernel.org>,
Sam Bazely <sambazley@fastmail.com>,
Jiri Kosina <jikos@kernel.org>,
Henrik Rydberg <rydberg@bitmath.org>,
"Pierre-Loup A . Griffais" <pgriffais@valvesoftware.com>,
Austin Palmer <austinp@valvesoftware.com>,
lkml <linux-kernel@vger.kernel.org>,
"3.8+" <stable@vger.kernel.org>
Subject: Re: [PATCH v2 2/3] HID: logitech-hidpp: rework device validation
Date: Wed, 16 Oct 2019 21:24:00 +0200 [thread overview]
Message-ID: <CAO-hwJ++YWtX29YefGzaEfCLDA=npZwUxDCkDzxALAmLLqv7FQ@mail.gmail.com> (raw)
In-Reply-To: <20191016182935.5616-3-andrew.smirnov@gmail.com>
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
>
next prev parent reply other threads:[~2019-10-16 19:24 UTC|newest]
Thread overview: 8+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-10-16 18:29 [PATCH v2 0/3] Logitech G920 fixes Andrey Smirnov
2019-10-16 18:29 ` [PATCH v2 1/3] HID: logitech-hidpp: split g920_get_config() Andrey Smirnov
2019-10-16 18:29 ` [PATCH v2 2/3] HID: logitech-hidpp: rework device validation Andrey Smirnov
2019-10-16 19:24 ` Benjamin Tissoires [this message]
2019-10-16 19:38 ` Andrey Smirnov
2019-10-17 8:48 ` Benjamin Tissoires
2019-10-16 18:29 ` [PATCH v2 3/3] HID: logitech-hidpp: do all FF cleanup in hidpp_ff_destroy() Andrey Smirnov
[not found] ` <20191017143149.449AA21D7C@mail.kernel.org>
2019-10-18 4:25 ` Andrey Smirnov
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to='CAO-hwJ++YWtX29YefGzaEfCLDA=npZwUxDCkDzxALAmLLqv7FQ@mail.gmail.com' \
--to=benjamin.tissoires@redhat.com \
--cc=andrew.smirnov@gmail.com \
--cc=austinp@valvesoftware.com \
--cc=jikos@kernel.org \
--cc=linux-input@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
--cc=pgriffais@valvesoftware.com \
--cc=rydberg@bitmath.org \
--cc=sambazley@fastmail.com \
--cc=stable@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).