| 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: <e55711bb-5e5c-40a5-a2e1-a5a4aac9816c@amd.com>
Date: Tue, 25 Feb 2025 07:16:33 -0800
From: Mario Limonciello <mario.limonciello@....com>
To: Luke Jones <luke@...nes.dev>, linux-kernel@...r.kernel.org
Cc: hdegoede@...hat.com, ilpo.jarvinen@...ux.intel.com,
platform-driver-x86@...r.kernel.org, linux-input@...r.kernel.org,
bentiss@...nel.org, jikos@...nel.org
Subject: Re: [PATCH 1/2] hid-asus: check ROG Ally MCU version and warn
On 2/25/2025 00:17, Luke Jones wrote:
> From: "Luke D. Jones" <luke@...nes.dev>
>
> ASUS have fixed suspend issues arising from a flag not being cleared in
> the MCU FW in both the ROG Ally 1 and the ROG Ally X.
>
> Implement a check and a warning to encourage users to update the FW to
> a minimum supported version.
>
> Signed-off-by: Luke D. Jones <luke@...nes.dev>
> ---
> drivers/hid/hid-asus.c | 97 +++++++++++++++++++++++++++++++++++++++++-
> 1 file changed, 95 insertions(+), 2 deletions(-)
>
> diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> index 46e3e42f9eb5..e1e60b80115a 100644
> --- a/drivers/hid/hid-asus.c
> +++ b/drivers/hid/hid-asus.c
> @@ -52,6 +52,10 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
> #define FEATURE_KBD_LED_REPORT_ID1 0x5d
> #define FEATURE_KBD_LED_REPORT_ID2 0x5e
>
> +#define ROG_ALLY_REPORT_SIZE 64
> +#define ROG_ALLY_X_MIN_MCU 313
> +#define ROG_ALLY_MIN_MCU 319
> +
> #define SUPPORT_KBD_BACKLIGHT BIT(0)
>
> #define MAX_TOUCH_MAJOR 8
> @@ -84,6 +88,7 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
> #define QUIRK_MEDION_E1239T BIT(10)
> #define QUIRK_ROG_NKEY_KEYBOARD BIT(11)
> #define QUIRK_ROG_CLAYMORE_II_KEYBOARD BIT(12)
> +#define QUIRK_ROG_ALLY_XPAD BIT(13)
>
> #define I2C_KEYBOARD_QUIRKS (QUIRK_FIX_NOTEBOOK_REPORT | \
> QUIRK_NO_INIT_REPORTS | \
> @@ -534,9 +539,89 @@ static bool asus_kbd_wmi_led_control_present(struct hid_device *hdev)
> return !!(value & ASUS_WMI_DSTS_PRESENCE_BIT);
> }
>
> +/*
> + * We don't care about any other part of the string except the version section.
> + * Example strings: FGA80100.RC72LA.312_T01, FGA80100.RC71LS.318_T01
> + */
> +static int mcu_parse_version_string(const u8 *response, size_t response_size)
> +{
> + const u8 *end = response + response_size;
> + const u8 *p = response;
> + int dots, err;
> + long version;
> +
> + dots = 0;
> + while (p < end && dots < 2) {
> + if (*p++ == '.')
> + dots++;
> + }
> +
I think it would be good to use strsep() here.
> + if (dots != 2 || p >= end)
> + return -EINVAL;
> +
> + err = kstrtol((const char *)p, 10, &version);
> + if (err || version < 0)
> + return -EINVAL;
> +
> + return version;
> +}
> +
> +static int mcu_request_version(struct hid_device *hdev)
> +{
> + const u8 request[] = { 0x5a, 0x05, 0x03, 0x31, 0x00, 0x20 };
> + u8 *response;
> + int ret;
> +
> + response = kzalloc(ROG_ALLY_REPORT_SIZE, GFP_KERNEL);
> + if (!response)
> + return -ENOMEM;
> +
> + ret = asus_kbd_set_report(hdev, request, sizeof(request));
> + if (ret < 0)
> + goto out;
> +
> + ret = hid_hw_raw_request(hdev, FEATURE_REPORT_ID, response,
> + ROG_ALLY_REPORT_SIZE, HID_FEATURE_REPORT,
> + HID_REQ_GET_REPORT);
> + if (ret < 0)
> + goto out;
> +
> + ret = mcu_parse_version_string(response, ROG_ALLY_REPORT_SIZE);
> +
> +out:
> + if (ret < 0)
> + hid_err(hdev, "Failed to get MCU version: %d, response: %*ph\n",
> + ret, ROG_ALLY_REPORT_SIZE, response);
> + kfree(response);
> + return ret;
> +}
> +
> +static void validate_mcu_fw_version(struct hid_device *hdev, int idProduct)
> +{
> + int min_version = ROG_ALLY_X_MIN_MCU;
> + int version;
> +
> + version = mcu_request_version(hdev);
> + if (version < 0)
> + return;
> +
> + if (idProduct == USB_DEVICE_ID_ASUSTEK_ROG_NKEY_ALLY)
> + min_version = ROG_ALLY_MIN_MCU;
> +
> + hid_info(hdev, "Ally device MCU version: %d\n", version);
> + if (version < min_version) {
> + hid_warn(hdev,
> + "The MCU firmware version must be %d or greater\n"
What do you think about:
"The MCU firmware version must be %d or greater to avoid issues with
suspend.\n"
> + "Please update your MCU with official ASUS firmware release\n",
> + min_version);
> + }
> +}
Thinking forward to any future hypothetical devices that don't have a
min MCU version type of bug I have a suggestion that you put the
min_version into a lookup method of some sort.
So the flow can be something like this:
static void validate_mcu_fw_version(struct hid_device *hdev, int idProduct)
{
int min_version = get_min_version(idProduct);
if (!min_version)
return;
.
.
.
}
Or you can do a switch/case instead of get_min_version().
static void validate_mcu_fw_version(struct hid_device *hdev, int idProduct)
{
int min_version;
switch(idProduct) {
case USB_DEVICE_ID_ASUSTEK_ROG_NKEY_ALLY:
min_version = ROG_ALLY_MIN_MCU;
break;
case USB_DEVICE_ID_ASUSTEK_ROG_NKEY_ALLYX:
min_version = ROG_ALLYX_MIN_MCU;
break;
default:
return;
}
.
.
.
}
That way you have really straight forward logic that
validate_mcu_version only runs on devices that you specify.
> +
> static int asus_kbd_register_leds(struct hid_device *hdev)
> {
> struct asus_drvdata *drvdata = hid_get_drvdata(hdev);
> + struct usb_interface *intf;
> + struct usb_device *udev;
> unsigned char kbd_func;
> int ret;
>
> @@ -560,6 +645,14 @@ static int asus_kbd_register_leds(struct hid_device *hdev)
> if (ret < 0)
> return ret;
> }
> +
> + if (drvdata->quirks & QUIRK_ROG_ALLY_XPAD) {
> + intf = to_usb_interface(hdev->dev.parent);
> + udev = interface_to_usbdev(intf);
> + validate_mcu_fw_version(hdev,
> + le16_to_cpu(udev->descriptor.idProduct));
> + }
> +
> } else {
> /* Initialize keyboard */
> ret = asus_kbd_init(hdev, FEATURE_KBD_REPORT_ID);
> @@ -1280,10 +1373,10 @@ static const struct hid_device_id asus_devices[] = {
> QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
> { HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
> USB_DEVICE_ID_ASUSTEK_ROG_NKEY_ALLY),
> - QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
> + QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD | QUIRK_ROG_ALLY_XPAD},
> { HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
> USB_DEVICE_ID_ASUSTEK_ROG_NKEY_ALLY_X),
> - QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD },
> + QUIRK_USE_KBD_BACKLIGHT | QUIRK_ROG_NKEY_KEYBOARD | QUIRK_ROG_ALLY_XPAD },
> { HID_USB_DEVICE(USB_VENDOR_ID_ASUSTEK,
> USB_DEVICE_ID_ASUSTEK_ROG_CLAYMORE_II_KEYBOARD),
> QUIRK_ROG_CLAYMORE_II_KEYBOARD },
Powered by blists - more mailing lists