| lists.openwall.net | lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC | |
|
Open Source and information security mailing list archives
| ||
|
Message-ID: <567b2056-8687-4f92-b4d2-7f289321275e@ljones.dev>
Date: Thu, 20 Mar 2025 20:19:16 +1300
From: "Luke D. Jones" <luke@...nes.dev>
To: Antheas Kapenekakis <lkml@...heas.dev>,
platform-driver-x86@...r.kernel.org, linux-input@...r.kernel.org
Cc: linux-kernel@...r.kernel.org, Jiri Kosina <jikos@...nel.org>,
Benjamin Tissoires <bentiss@...nel.org>,
Corentin Chary <corentin.chary@...il.com>,
Hans de Goede <hdegoede@...hat.com>,
Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Subject: Re: [PATCH 01/11] HID: asus: refactor init sequence per spec
On 20/03/25 08:13, Antheas Kapenekakis wrote:
> Currently, asus_kbd_init() uses a reverse engineered init sequence
> from Windows, which contains the handshakes from multiple programs.
> Keep the main one, which is 0x5a (meant for drivers).
0x5A is also used for Ally setup commands, used from userspace in
Windows. Only a nit but I don't think stating it's only for drivers is
accurate but then again asus kind of blurs the line a bit.
> In addition, perform a get_response and check if the response is the
> same. To avoid regressions, print an error if the response does not
> match instead of rejecting device.
>
> Signed-off-by: Antheas Kapenekakis <lkml@...heas.dev>
> ---
> drivers/hid/hid-asus.c | 82 +++++++++++++++++++++++-------------------
> 1 file changed, 46 insertions(+), 36 deletions(-)
>
> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index 46e3e42f9eb5f..aa4a481dc4f27 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c
> @@ -48,7 +48,7 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
> #define FEATURE_REPORT_ID 0x0d
> #define INPUT_REPORT_ID 0x5d
> #define FEATURE_KBD_REPORT_ID 0x5a
> -#define FEATURE_KBD_REPORT_SIZE 16
> +#define FEATURE_KBD_REPORT_SIZE 64
> #define FEATURE_KBD_LED_REPORT_ID1 0x5d
> #define FEATURE_KBD_LED_REPORT_ID2 0x5e
>
> @@ -386,16 +386,43 @@ static int asus_kbd_set_report(struct hid_device *hdev, const u8 *buf, size_t bu
> return ret;
> }
>
> -static int asus_kbd_init(struct hid_device *hdev, u8 report_id)
> +static int asus_kbd_init(struct hid_device *hdev)
> {
> - const u8 buf[] = { report_id, 0x41, 0x53, 0x55, 0x53, 0x20, 0x54,
> - 0x65, 0x63, 0x68, 0x2e, 0x49, 0x6e, 0x63, 0x2e, 0x00 };
> + /*
> + * Asus handshake identifying us as a driver (0x5A)
> + * 0x5A then ASCII for "ASUS Tech.Inc."
> + * 0x5D is for userspace Windows applications.
0x5D is the report ID used for commands such as RGB modes. Probably
don't need to mention it here, and only where it is used.
> + * The handshake is first sent as a set_report, then retrieved
> + * from a get_report to verify the response.
> + */
> + const u8 buf[] = { FEATURE_KBD_REPORT_ID, 0x41, 0x53, 0x55, 0x53, 0x20,
> + 0x54, 0x65, 0x63, 0x68, 0x2e, 0x49, 0x6e, 0x63, 0x2e, 0x00 };
> + u8 *readbuf;
> int ret;
>
> ret = asus_kbd_set_report(hdev, buf, sizeof(buf));
> - if (ret < 0)
> - hid_err(hdev, "Asus failed to send init command: %d\n", ret);
> + if (ret < 0) {
> + hid_err(hdev, "Asus failed to send handshake: %d\n", ret);
> + return ret;
> + }
>
> + readbuf = kzalloc(FEATURE_KBD_REPORT_SIZE, GFP_KERNEL);
> + if (!readbuf)
> + return -ENOMEM;
> +
> + ret = hid_hw_raw_request(hdev, FEATURE_KBD_REPORT_ID, readbuf,
> + FEATURE_KBD_REPORT_SIZE, HID_FEATURE_REPORT,
> + HID_REQ_GET_REPORT);
> + if (ret < 0) {
> + hid_err(hdev, "Asus failed to receive handshake ack: %d\n", ret);
> + } else if (memcmp(readbuf, buf, sizeof(buf)) != 0) {
> + hid_err(hdev, "Asus handshake returned invalid response: %*ph\n",
> + FEATURE_KBD_REPORT_SIZE, readbuf);
> + // Do not return error if handshake is wrong to avoid regressions
I'll have to test this on the oldest model I have. Hopefully it's a
non-issue and this can return error instead.
Side-note: I notice you're using a msleep to try and work around an
issue in a later patch - it might be worth trying replacing that with a
retry/count loop with an inner of small msleep + a call to this init,
see if it still responds to this during that critical period.
> + }
> +
> + kfree(readbuf);
> return ret;
> }
>
> @@ -540,42 +567,25 @@ static int asus_kbd_register_leds(struct hid_device *hdev)
> unsigned char kbd_func;
> int ret;
>
> - if (drvdata->quirks & QUIRK_ROG_NKEY_KEYBOARD) {
> - /* Initialize keyboard */
> - ret = asus_kbd_init(hdev, FEATURE_KBD_REPORT_ID);
> - if (ret < 0)
> - return ret;
> -
> - /* The LED endpoint is initialised in two HID */
> - ret = asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID1);
> - if (ret < 0)
> - return ret;
> -
> - ret = asus_kbd_init(hdev, FEATURE_KBD_LED_REPORT_ID2);
> - if (ret < 0)
> - return ret;
Ah, I recall now. Some devices like the Slash or AniMe Matrix required
the 0x5E and 0x5D report ID (device dependent) however these are
currently being done via userspace due to not being HID devices.
There *are* some older laptops still in use that require init on 0x5E or
0x5D for RGB to be usable, from memory. It's been over 5 years so I'll
pull out the laptop I have with 0x1866 PID MCU and see if that is
actually true and not just my imagination.
> + ret = asus_kbd_init(hdev);
> + if (ret < 0)
> + return ret;
>
> - if (dmi_match(DMI_PRODUCT_FAMILY, "ProArt P16")) {
> - ret = asus_kbd_disable_oobe(hdev);
> - if (ret < 0)
> - return ret;
> - }
> - } else {
> - /* Initialize keyboard */
> - ret = asus_kbd_init(hdev, FEATURE_KBD_REPORT_ID);
> - if (ret < 0)
> - return ret;
> + /* Get keyboard functions */
> + ret = asus_kbd_get_functions(hdev, &kbd_func, FEATURE_KBD_REPORT_ID);
> + if (ret < 0)
> + return ret;
>
> - /* Get keyboard functions */
> - ret = asus_kbd_get_functions(hdev, &kbd_func, FEATURE_KBD_REPORT_ID);
> + if (dmi_match(DMI_PRODUCT_FAMILY, "ProArt P16")) {
> + ret = asus_kbd_disable_oobe(hdev);
> if (ret < 0)
> return ret;
> -
> - /* Check for backlight support */
> - if (!(kbd_func & SUPPORT_KBD_BACKLIGHT))
> - return -ENODEV;
> }
>
> + /* Check for backlight support */
> + if (!(kbd_func & SUPPORT_KBD_BACKLIGHT))
> + return -ENODEV;
> +
> drvdata->kbd_backlight = devm_kzalloc(&hdev->dev,
> sizeof(struct asus_kbd_leds),
> GFP_KERNEL);
I've left only small comments on a few patches for now. I'll review in
full after I get testing done on a variety of devices whcih I'm aiming
for this weekend. Overall impression so far is everything looks good and
this is a nice improvement. Thank you for taking the time to implement it.
Cheers,
Luke.
Powered by blists - more mailing lists