| 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:
<CAGwozwGzN9Z5iQAVeYbmmAAf9jxhTw5Yy8f36BfwYdgN45kqNA@mail.gmail.com>
Date: Thu, 23 Oct 2025 20:27:32 +0200
From: Antheas Kapenekakis <lkml@...heas.dev>
To: Denis Benato <benato.denis96@...il.com>
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>,
"Luke D . Jones" <luke@...nes.dev>, Hans de Goede <hdegoede@...hat.com>,
Ilpo Järvinen <ilpo.jarvinen@...ux.intel.com>
Subject: Re: [PATCH v7 4/9] HID: asus: prevent binding to all HID devices on
ROG
On Thu, 23 Oct 2025 at 20:23, Denis Benato <benato.denis96@...il.com> wrote:
>
>
> On 10/18/25 12:17, Antheas Kapenekakis wrote:
> > Currently, when hid-asus is not loaded, NKEY keyboards load as ~6
> > event devices with a pretty ASUSTEK name. When it loads, it concatenates
> > all applications per HID endpoint, renames them, and prints errors
> > when some of them do not have an input device.
> >
> > Therefore, change probe so that this is no longer the case. Stop
> > renaming the devices, omit the check for .input which causes errors on
> the devices -> devices
> > e.g., the Z13 for some hiddev only devices, and add
> > HID_QUIRK_INPUT_PER_APP so that each application gets its own event.
>
> event -> event device (or evdev?)
>
> It is not clear from the message what HID_QUIRK_INPUT_PER_APP has to do with
> renaming the devices/having one evdev vs multiple: please make
> it explicit in the commit message (and perhaps make explicit if (and how),
> in case it could make any difference, how programs might change
> theirs behavior as a consequence).
>
> I like the fact that userspace only sees one keyboard for what is,
> effectively, one keyboard device.
>
> The code looks good to me: make the commit message more
> explanatory and I'll include my reviewed-by.
If I respin the series to a v8 I will reword this patch subject.
Antheas
> Thanks,
> Denis
>
> > When this is done, the probes are called multiple times. Due to this,
> > the rgb check needs to be moved into probe, and the report fixup should
> > be skipped for non-vendor endpoints (prevents multiple prints).
> >
> > Reviewed-by: Luke D. Jones <luke@...nes.dev>
> > Signed-off-by: Antheas Kapenekakis <lkml@...heas.dev>
> > ---
> > drivers/hid/hid-asus.c | 59 +++++++++++++++++++++++++++---------------
> > 1 file changed, 38 insertions(+), 21 deletions(-)
> >
> > diff --git a/drivers/hid/hid-asus.c b/drivers/hid/hid-asus.c
> > index 03f0d86936fc..bbbac98f76c6 100644
> > --- a/drivers/hid/hid-asus.c
> > +++ b/drivers/hid/hid-asus.c
> > @@ -47,6 +47,7 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
> > #define T100CHI_MOUSE_REPORT_ID 0x06
> > #define FEATURE_REPORT_ID 0x0d
> > #define INPUT_REPORT_ID 0x5d
> > +#define HID_USAGE_PAGE_VENDOR 0xff310000
> > #define FEATURE_KBD_REPORT_ID 0x5a
> > #define FEATURE_KBD_REPORT_SIZE 64
> > #define FEATURE_KBD_LED_REPORT_ID1 0x5d
> > @@ -89,6 +90,7 @@ MODULE_DESCRIPTION("Asus HID Keyboard and TouchPad");
> > #define QUIRK_ROG_NKEY_KEYBOARD BIT(11)
> > #define QUIRK_ROG_CLAYMORE_II_KEYBOARD BIT(12)
> > #define QUIRK_ROG_ALLY_XPAD BIT(13)
> > +#define QUIRK_SKIP_REPORT_FIXUP BIT(14)
> >
> > #define I2C_KEYBOARD_QUIRKS (QUIRK_FIX_NOTEBOOK_REPORT | \
> > QUIRK_NO_INIT_REPORTS | \
> > @@ -125,7 +127,6 @@ struct asus_drvdata {
> > struct input_dev *tp_kbd_input;
> > struct asus_kbd_leds *kbd_backlight;
> > const struct asus_touchpad_info *tp;
> > - bool enable_backlight;
> > struct power_supply *battery;
> > struct power_supply_desc battery_desc;
> > int battery_capacity;
> > @@ -316,7 +317,7 @@ static int asus_e1239t_event(struct asus_drvdata *drvdat, u8 *data, int size)
> > static int asus_event(struct hid_device *hdev, struct hid_field *field,
> > struct hid_usage *usage, __s32 value)
> > {
> > - if ((usage->hid & HID_USAGE_PAGE) == 0xff310000 &&
> > + if ((usage->hid & HID_USAGE_PAGE) == HID_USAGE_PAGE_VENDOR &&
> > (usage->hid & HID_USAGE) != 0x00 &&
> > (usage->hid & HID_USAGE) != 0xff && !usage->type) {
> > hid_warn(hdev, "Unmapped Asus vendor usagepage code 0x%02x\n",
> > @@ -931,11 +932,6 @@ static int asus_input_configured(struct hid_device *hdev, struct hid_input *hi)
> >
> > drvdata->input = input;
> >
> > - if (drvdata->enable_backlight &&
> > - !asus_kbd_wmi_led_control_present(hdev) &&
> > - asus_kbd_register_leds(hdev))
> > - hid_warn(hdev, "Failed to initialize backlight.\n");
> > -
> > return 0;
> > }
> >
> > @@ -1008,15 +1004,6 @@ static int asus_input_mapping(struct hid_device *hdev,
> > return -1;
> > }
> >
> > - /*
> > - * Check and enable backlight only on devices with UsagePage ==
> > - * 0xff31 to avoid initializing the keyboard firmware multiple
> > - * times on devices with multiple HID descriptors but same
> > - * PID/VID.
> > - */
> > - if (drvdata->quirks & QUIRK_USE_KBD_BACKLIGHT)
> > - drvdata->enable_backlight = true;
> > -
> > set_bit(EV_REP, hi->input->evbit);
> > return 1;
> > }
> > @@ -1133,8 +1120,10 @@ static int __maybe_unused asus_reset_resume(struct hid_device *hdev)
> >
> > static int asus_probe(struct hid_device *hdev, const struct hid_device_id *id)
> > {
> > - int ret;
> > + struct hid_report_enum *rep_enum;
> > struct asus_drvdata *drvdata;
> > + struct hid_report *rep;
> > + int ret, is_vendor = 0;
> >
> > drvdata = devm_kzalloc(&hdev->dev, sizeof(*drvdata), GFP_KERNEL);
> > if (drvdata == NULL) {
> > @@ -1218,18 +1207,42 @@ static int asus_probe(struct hid_device *hdev, const struct hid_device_id *id)
> > return ret;
> > }
> >
> > + /* Check for vendor for RGB init and handle generic devices properly. */
> > + rep_enum = &hdev->report_enum[HID_INPUT_REPORT];
> > + list_for_each_entry(rep, &rep_enum->report_list, list) {
> > + if ((rep->application & HID_USAGE_PAGE) == HID_USAGE_PAGE_VENDOR)
> > + is_vendor = true;
> > + }
> > +
> > + /*
> > + * For ROG keyboards, make them HID/hiddev compliant by creating one
> > + * input per application. For interfaces other than the vendor one,
> > + * disable report fixups.
> > + */
> > + if (drvdata->quirks & QUIRK_ROG_NKEY_KEYBOARD) {
> > + if (!is_vendor)
> > + drvdata->quirks |= QUIRK_SKIP_REPORT_FIXUP;
> > + hdev->quirks |= HID_QUIRK_INPUT_PER_APP;
> > + }
> > +
> > ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT);
> > if (ret) {
> > hid_err(hdev, "Asus hw start failed: %d\n", ret);
> > return ret;
> > }
> >
> > + if (is_vendor && (drvdata->quirks & QUIRK_USE_KBD_BACKLIGHT) &&
> > + !asus_kbd_wmi_led_control_present(hdev) &&
> > + asus_kbd_register_leds(hdev))
> > + hid_warn(hdev, "Failed to initialize backlight.\n");
> > +
> > /*
> > - * Check that input registration succeeded. Checking that
> > - * HID_CLAIMED_INPUT is set prevents a UAF when all input devices
> > - * were freed during registration due to no usages being mapped,
> > - * leaving drvdata->input pointing to freed memory.
> > + * For ROG keyboards, skip rename for consistency and ->input check as
> > + * some devices do not have inputs.
> > */
> > + if (drvdata->quirks & QUIRK_ROG_NKEY_KEYBOARD)
> > + return 0;
> > +
> > if (!drvdata->input || !(hdev->claimed & HID_CLAIMED_INPUT)) {
> > hid_err(hdev, "Asus input not registered\n");
> > ret = -ENOMEM;
> > @@ -1352,6 +1365,10 @@ static const __u8 *asus_report_fixup(struct hid_device *hdev, __u8 *rdesc,
> > rdesc = new_rdesc;
> > }
> >
> > + /* Vendor fixups should only apply to NKEY vendor devices. */
> > + if (drvdata->quirks & QUIRK_SKIP_REPORT_FIXUP)
> > + return rdesc;
> > +
> > if (drvdata->quirks & QUIRK_ROG_NKEY_KEYBOARD &&
> > *rsize == 331 && rdesc[190] == 0x85 && rdesc[191] == 0x5a &&
> > rdesc[204] == 0x95 && rdesc[205] == 0x05) {
>
Powered by blists - more mailing lists