| 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: <fac9f4d3-ed1d-4e20-b555-4b83434241a6@ljones.dev>
Date: Sun, 23 Mar 2025 13:10:40 +1300
From: "Luke D. Jones" <luke@...nes.dev>
To: Antheas Kapenekakis <lkml@...heas.dev>
Cc: platform-driver-x86@...r.kernel.org, linux-input@...r.kernel.org,
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 v3 01/10] HID: asus: refactor init sequence per spec
On 23/03/25 12:54, Antheas Kapenekakis wrote:
> On Sun, 23 Mar 2025 at 00:53, Antheas Kapenekakis <lkml@...heas.dev> wrote:
>>
>> On Sun, 23 Mar 2025 at 00:29, Luke D. Jones <luke@...nes.dev> wrote:
>>>
>>> On 23/03/25 12:05, Antheas Kapenekakis wrote:
>>>> On Sat, 22 Mar 2025 at 23:01, Luke D. Jones <luke@...nes.dev> wrote:
>>>>>
>>>>> On 22/03/25 23:27, 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 brightness drivers).
>>>>>>
>>>>>> 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.
>>>>>>
>>>>>> Then, refactor asus_kbd_get_functions() to use the same ID it is called
>>>>>> with, instead of hardcoding it to 0x5a so that it may be used for 0x0d
>>>>>> in the future.
>>>>>>
>>>>>> 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..8d4df1b6f143b 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
>>>>>>
>>>>>> @@ -388,14 +388,41 @@ static int asus_kbd_set_report(struct hid_device *hdev, const u8 *buf, size_t bu
>>>>>>
>>>>>> static int asus_kbd_init(struct hid_device *hdev, u8 report_id)
>>>>>> {
>>>>>> - 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.
>>>>>> + *
>>>>>> + * The handshake is first sent as a set_report, then retrieved
>>>>>> + * from a get_report to verify the response.
>>>>>> + */
>>>>>
>>>>> This entire comment is not required, especially not the last paragraph.
>>>>> From what I've seen in .dll reversing attempts there's no real
>>>>> distinction from driver/app and it's simply an init/enable sequence
>>>>> common to almost every ITE MCU that ASUS have used (slash, anime, Ally).
>>>>>
>>>>> Please remove.
>>>>
>>>> It is a context comment but can be removed.
>>>>
>>>>>> + const u8 buf[] = { 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, 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
>>>>>> + }
>>>>>> +
>>>>>> + kfree(readbuf);
>>>>>> return ret;
>>>>>> }
>>>>>>
>>>>>> @@ -417,7 +444,7 @@ static int asus_kbd_get_functions(struct hid_device *hdev,
>>>>>> if (!readbuf)
>>>>>> return -ENOMEM;
>>>>>>
>>>>>> - ret = hid_hw_raw_request(hdev, FEATURE_KBD_REPORT_ID, readbuf,
>>>>>> + ret = hid_hw_raw_request(hdev, report_id, readbuf,
>>>>>> FEATURE_KBD_REPORT_SIZE, HID_FEATURE_REPORT,
>>>>>> HID_REQ_GET_REPORT);
>>>>>> if (ret < 0) {
>>>>>> @@ -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;
>>>>>> + ret = asus_kbd_init(hdev, FEATURE_KBD_REPORT_ID);
>>>>>> + if (ret < 0)
>>>>>> + return ret;
>>>>>>
>>>>>
>>>>> I don't have any non-ROG equipment to test. There have been some cases
>>>>> of Vivobook using the same MCU, but these otherwise used something else.
>>>>> And the oldest hardware I have uses a 0x1866, which uses the same init
>>>>> sequence but works with both 0x5A and 0x5D report ID for init (on same
>>>>> endpoint). There are instances of other laptops that accept both 0x5A
>>>>> and 0x5D, and some that have only 0x5D.
>>>>
>>>> The driver sets the brightness using 0x5a and accepts keyboard
>>>> shortcuts only from 0x5a. Therefore it needs to init only with 0x5a.
>>>>
>>>> How would those laptops regress and in what way?
>>>>
>>>
>>> The media keys fail to work (vol, mic, rog). Can you please accept that
>>> I do know some laptops used only 0x5D, and these are older models,
>>> around 5+ years. The only thing I have to go on is my memory
>>> unfortunately, and I've been trying to find the concrete examples.
>>
>> I just looked at the history. Yeah it seems you added ID1 in 2020 with
>> some other commands. But on the same commit you blocked 0x5d and 0x5e,
>> so it means those keyboards use 0x5a to send keyboard events.
>>
>> Nevertheless, it is not worth looking up or risking regressions for
>> old hardware. I will readd 0x5d, 0x5e for
>> USB_DEVICE_ID_ASUSTEK_ROG_NKEY_KEYBOARD and
>> USB_DEVICE_ID_ASUSTEK_ROG_NKEY_KEYBOARD2, since those are the ones you
>> added the inits with.
>>
>> But I will still keep them off for the Z13 and Ally.
>>
>> By the way,
>
> I guess I did not finish this. I was going to express some concern
> about the Claymore keyboard. But it seems it does not have a backlight
> quirk so it is ok
>
The external keyboards are a bit funky yeah. I don't think the Claymore
is in very wide use at all either. In any case you're correct.
>> Antheas
>>
>>>>> I think you will need to change this to try both 0x5A and 0x5D sorry.
>>>>> The older models using 0x1854, 0x1869, 0x1866 PID may regress and
>>>>> although I'm reasonably confident there won't be issues due to age of
>>>>> those, it's not a risk I'm willing to take, I've spent all morning
>>>>> trawling through archives of info and I can't actually verify this other
>>>>> than from my memory.
>>>>
>>>> For devices that support RGB, only when RGB is set, 0x5D is initialized too.
>>>
>>> Sure. But as I've said above.. Please add both to init. It's only done
>>> once, and it doesn't hurt anything plus doesn't risk regressing older
>>> hardware.
>>>
>>> If I can get the proper evidence that only 0x5A is required I'm happy to
>>> use only that, but until then I don't want that risk. And it's only a
>>> small thing here.
>>>
>>> Cheers,
>>> Luke.
>>>
>>>>
>>>>> I mentioned 0x5E being used for some of the oddball devices like slash
>>>>> and anime, don't worry about that one, it's a bridge that can be crossed
>>>>> at a later time. But it does illustrate that other report ID have been
>>>>> used for init.
>>>>>
>>>>> Otherwise the cleanup is good.
>>>>>
>>>>> No other comments required and I'll sign off with the above corrections.
>>>>>
>>>>> Cheers,
>>>>> Luke
>>>>>
>>>>>> - 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);
>>>>>
>>>
Powered by blists - more mailing lists