Skip to content

HID: asus: fix Ally X input-report size validation (residual audit fixes after ally branch rewrite) - #6

Closed
jlobue10 wants to merge 14 commits into
NeroReflex:allyfrom
jlobue10:ally-audit-fixes
Closed

HID: asus: fix Ally X input-report size validation (residual audit fixes after ally branch rewrite)#6
jlobue10 wants to merge 14 commits into
NeroReflex:allyfrom
jlobue10:ally-audit-fixes

Conversation

@jlobue10

@jlobue10 jlobue10 commented Jul 26, 2026

Copy link
Copy Markdown

Important

Rebased 2026-07-26 onto the rewritten ally branch (9bc9553). The branch rewrite absorbed most of the original 9-fix series, so this PR is now just the residual delta (+40/−32). The one functional change remaining is critical: the rewrite kept the audit's report-size guard but not its field-tested buttons[4]buttons[3] correction, so the current branch tip rejects every 16-byte 0x0B gamepad report on Ally X hardware. Details in this comment. Remaining scope: (1) buttons[3] wire-size fix, (2) response-curve store style cleanup, (3) probe-error-path comment, (4) indentation normalization. The original pre-rebase description follows for reference.


Security/lifecycle audit of hid-asus.c: 9 fixes, field-validated on Xbox Ally X

This PR carries the fixes from a source audit of drivers/hid/hid-asus.c on
branch ally @ bbd3c71a752f ("HID: asus: tmp leds"), followed by a runtime
validation pass on real hardware. Full audit write-up (analysis, failure
scenarios, and 4 investigated-and-rejected false positives) is posted as the first comment below.

The 9 findings

# Severity Defect
1 HIGH hat_values[] indexed by a raw wire byte — OOB read of kernel .rodata leaked to userspace via ABS_HAT0X/Y. Also present on master.
2 CRITICAL Probe error path in ally_create_button_attributes(): inner loop clobbers the outer index — infinite loop, repeated double-kfree of 48 objects, UAF inside sysfs_remove_group()
3 HIGH hid_asus_ally_remove() never clears keyboard_hdev/keyboard_input — UAF write from handle_ally_event()/ally_resume_work_fn() when the kbd interface tears down first
4 MEDIUM ally_x_raw_event() casts a 15-byte payload onto &data[1] without checking size — a short transfer reads stale DMA buffer bytes (and feeds #1). Also present on master.
5 MEDIUM ally_x_play_effect() can queue ff_work after cancel_work_sync() — flag test and queueing now both under ff_lock
6 MEDIUM INIT_DELAYED_WORK on a possibly-queued resume_work + unbalanced cancel — statically initialized, cancelled on every interface teardown
7 MEDIUM NULL deref in the resume work when the kbd interface is unbound at wake
8 LOW Response-curve sysfs stores write shared state outside the lock
9 LOW/MEDIUM Stale globals and an attr-array leak on probe failure

Runtime validation (Xbox Ally X, 2026-07-25)

Test vehicle: 7.2.0-rc4 CachyOS-deckify kernel + this branch's hid-asus,
audited module built out-of-tree and exercised with slub_debug=FZP on the
kernel command line (freed-memory poisoning + redzones; neither installed
kernel has KASAN). Every phase below was followed by a kmsg scan for
BUG/oops/poison/redzone/WARN — all scans clean.

Exercise Targets Result
Cold boot with audited module as the in-tree module (initramfs probe path) #2 ChimeraOS#9 all 6 interfaces bound, full sysfs tree present
Unbind/rebind order matrix across the 3 HID interfaces (kbd-first / cfg-first / pad-first + kbd-only dwell) #3 #6 ChimeraOS#9 PASS
Rumble loop (69 effects) racing 5× gamepad-interface unbind/rebind #5 PASS
Suspend/resume ×3 permutations: normal, kbd interface unbound, all-interface unbind immediately after wake (races the 500 ms resume work) #3 #6 #7 PASS
Response-curve concurrent store/apply stress, both sticks (2 writer loops × 100 applies each) ChimeraOS#8 PASS, 0 apply errors
Full button matrix (25/25: ABXY, bumpers, sticks+clicks, dpad, triggers, paddles, front buttons short+long) #1 #4 regression PASS
Suspend soak ×3 rtcwake cycles: rebind set, input nodes, config-marker survival, post-resume rumble lifecycle PASS

Not runtime-covered, for honesty's sake:

The validation loop caught a bug in the audit's own #4 fix

Worth calling out because it's also new information about the hardware: the
first version of the #4 fix required size >= 1 + sizeof(struct ally_x_input_report) = 17 with the struct's existing buttons[4]. On the
wire, the Ally X's 0x0B interrupt-IN report is exactly 16 bytes (usbmon:
0b + 12 axis bytes + 2 button bitmaps + 1 hat byte) — buttons[3] does not
exist on the wire. That version rejected every real report; the custom gamepad
node went silent and input fell through to generic HID decode on the N-KEY
node, which kept a coincidental subset of buttons working (A/B/X, dpad,
triggers) while Y, bumpers, stick clicks, View/Menu/Xbox and both stick axes
died. The button matrix caught it immediately.

The shipped fix shrinks the struct to buttons[3] so sizeof matches the
wire payload, and the full matrix passes 25/25. Two takeaways for the tree:
the trailing buttons[4] byte in ally_x_input_report over-declares the
report, and any future size-validation there must use 16, not 17.

Notes

Test hardware: ASUS Xbox Ally X (0b05:1b4c). Happy to re-run any of this or
share raw logs/kmsg captures.

🤖 Generated with Claude Code

@jlobue10

Copy link
Copy Markdown
Author

hid-asus.c audit — NeroReflex/linux ally branch

Audit date: 2026-07-25
Tree: NeroReflex/linux, branch ally @ bbd3c71a752f ("HID: asus: tmp leds")
Scope: drivers/hid/hid-asus.c and its call chains into hid-core, input, LED
class and workqueue.

9 defects found and fixed. All fixes are in the accompanying
hid-asus-ally-audit-fixes.patch.

Verification performed

  • make W=1 drivers/hid/hid-asus.o — clean, zero compiler and kernel-doc
    warnings (the branch had 19 pre-existing kernel-doc warnings; those are fixed
    too).
  • scripts/checkpatch.pl — 0 errors, 0 warnings over 525 changed lines.
  • Every finding below was independently re-derived by a second reviewer before
    being accepted; 4 further candidate findings were investigated and rejected
    as false positives (listed at the end, so nobody re-treads them).

Verification NOT performed — please read

This is static analysis plus a compile test. There is no ROG Ally on the
machine this was done on, so nothing here is runtime-tested.
The teardown and
error-path fixes (#2, #3, #5, #6, ChimeraOS#9) are exactly the kind of thing that wants
exercising under CONFIG_KASAN + CONFIG_DEBUG_OBJECTS with FAILSLAB fault
injection before you trust them. Treat the patch as a reviewed proposal, not a
validated one.


Applies to master as well, not just ally

Findings #1 and #4 are also present in this repo's master branch
(commit ab2cdad44991, "HID: asus: add support for ROG Ally handhelds"), i.e.
they are not ally-branch regressions and are in whatever is being sent
upstream. Worth fixing in both places.


Findings

#1 — Out-of-bounds read of kernel .rodata, leaked to userspace — HIGH

ally_x_raw_event(), fix at drivers/hid/hid-asus.c:4721

hat_values[][2] has 9 entries. The index is in_report->buttons[2], a u8
taken verbatim off the wire, used with no bounds check:

byte = in_report->buttons[2];
input_report_abs(input, ABS_HAT0X, hat_values[byte][0]);
input_report_abs(input, ABS_HAT0Y, hat_values[byte][1]);

There is no clamp anywhere in the chain (asus_raw_event
hid_asus_ally_raw_eventally_x_raw_event). The report descriptor's logical
maximum does not constrain it, for two independent reasons:

  1. hdrv->raw_event is invoked in __hid_input_report() before
    hid_report_raw_event(), so no field extraction or clamping has happened.
  2. asus_raw_event() returns -1 for handled 0x0B reports, so HID parsing
    never runs for this report at all.

The input core does not save you either — input_handle_abs_event() applies
fuzz filtering and a change check but does not clamp to the [-1, 1] range
registered via input_set_abs_params(). The values reach evdev verbatim.

Failure scenario: device sends 0B ... <buttons[2] = 0xFF> on the Ally-X
interface. hat_values[255] reads at byte offset 255 * 8 = 2040 from the array
base — roughly 1970 bytes past its end — and two 32-bit words of adjacent kernel
read-only data are emitted to userspace as ABS_HAT0X/ABS_HAT0Y on
/dev/input/eventN. KASAN reports a global-out-of-bounds.

Note this is not purely adversarial: HID hat switches conventionally use 8 or
0x0F as the null/centered state, so any firmware that packs bits into the
upper nibble of that byte trips it immediately.

Fix: treat anything the table does not cover as centred.


#2 — Infinite loop with repeated double-free in the probe error path — CRITICAL

ally_create_button_attributes(), fix at drivers/hid/hid-asus.c:4445

err_cleanup:
	while (--i >= 0) {
		sysfs_remove_group(&hdev->dev.kobj, &entries[i].group);

		for (i = 0; i < ALLY_BTN_MAX; i++) {   /* clobbers the outer index */
			kfree(entries[i].turbo_attr);
			kfree(entries[i].remap_attr);
			kfree(entries[i].macro_attr);
		}
	}

The inner for reuses the outer loop's index. Trace, with failure at i == 5:

  1. --i → 4, enter body.
  2. sysfs_remove_group(&entries[4].group).
  3. Inner loop frees all 48 pointers and exits with i == 16. None are NULLed.
  4. Outer --i15, still >= 0, loop re-enters.
  5. sysfs_remove_group(&entries[15].group) — that group was never created, and
    entries[15].attrs[] point at the struct attributes embedded in objects
    just freed in step 3 → use-after-free read inside sysfs_remove_group(),
    which dereferences attr->name.
  6. Inner loop runs again → double kfree() on all 48 already-freed pointers.
  7. i is 16 again → back to step 4. The loop never terminates, and
    err_free_entries / return ret are unreachable.

These are genuine kmalloc'd objects, not devm — ally_btn_turbo_attr_create()
and button_remap_attr_create() both use kzalloc_obj() + no_free_ptr(). So
this is real SLUB freelist corruption followed by a soft lockup in probe context.

Reachability: three paths — ally_btn_turbo_attr_create() failure, either of
two -ENOMEM paths, or sysfs_create_group() failure (e.g. -EEXIST on a
rebind). The -ENOMEM and sysfs_create_group paths are all trivially
reachable under FAILSLAB.

Fix: separate the two loops and NULL the pointers after freeing.


#3 — Use-after-free write via the keyboard interface — HIGH

hid_asus_ally_remove(), fix at drivers/hid/hid-asus.c:5025

hid_asus_ally_remove() cleared ally_x_input/ally_x_hdev and
cfg_hdev/config, but never keyboard_hdev/keyboard_input (set during
probe). Both are dereferenced afterwards, from handle_ally_event() and
ally_resume_work_fn().

Failure scenario, no heap grooming needed: USB tears the composite device's
interfaces down sequentially. The keyboard interface goes first →
hid_asus_ally_remove(kbd_hdev, ally) matches neither the ally_x_hdev nor the
cfg_hdev branch, so nothing is cleared; kbd_hdev and its input_dev are
freed. Before the config interface's own removal completes, an in-flight
interrupt report 5A A6 arrives on the config endpoint → asus_raw_event
hid_asus_ally_raw_event (case HID_ALLY_INTF_CFG_IN) → handle_ally_event
input_report_key() writes into the freed input_dev.

The pending 500 ms resume_work scheduled from asus_resume() gives a second,
wider window across suspend/resume.

Fix: clear both fields when the keyboard interface is removed, and drain
resume_work on every interface teardown (see #6).


#4 — Missing report-size validation — MEDIUM

ally_x_raw_event(), fix at drivers/hid/hid-asus.c:4690

The fields read from the struct cast onto &data[1] span 15 bytes (12 axis
bytes + 2 button bitmaps + 1 hat byte), so a correct read needs size >= 16.
The size parameter was accepted and never used. No caller constrains it —
__hid_input_report() guarantees only size >= 1, and the zero-padding of
short reports happens in hid_report_raw_event(), which runs after the
driver callback.

This is not an out-of-bounds access in practice: binding is USB-only and
usbhid's inbuf is max(HID_MIN_BUFFER_SIZE /* 64 */, largest report), so
the read stays inside the allocation. The real bug is that a 2-byte transfer
(urb->actual_length == 2) leaves bytes 2..15 holding stale contents of
the DMA buffer from a previous report
— garbage axis and button events, and
the stale byte at data[15] becomes the out-of-bounds hat_values index
from #1. So this is a correctness bug and a #1 amplifier.

Fix: reject reports shorter than the payload being cast, and shrink the
struct's trailing buttons[4] to buttons[3] so sizeof matches the wire
payload.

Field-validation note (2026-07-25, Xbox Ally X): the original fix kept
Nero's buttons[4] declaration and required size >= 1 + sizeof(*in_report)
= 17, but usbmon shows the device's 0x0B interrupt-IN report is exactly 16
bytes
(ID + 15 payload; buttons[3] does not exist on the wire). That
version rejected every real report — the custom gamepad node went silent and
events fell through to generic HID decode on the N-KEY node, which surfaced
as a partial button-matrix failure (Y/LB/RB/L3/R3/View/Menu/Xbox/sticks dead,
A/B/X/dpad/triggers coincidentally alive). Corrected to the struct-shrink
form above; the hat-byte offset cited here was likewise corrected from
data[3] to data[15] (last byte of the report).


#5 — Force-feedback work queued after cancel_work_sync() — MEDIUM

ally_x_play_effect(), fix at drivers/hid/hid-asus.c:4777

ally_x_play_effect() read ally->ff_work_initialized without holding any
lock, then called schedule_work(). hid_asus_ally_remove() cleared the flag
and called cancel_work_sync() while holding ally_data_mutex.

Interleaving:

  1. CPU A (ally_x_play_effect, reachable during teardown — the input device is
    devm-bound and stays registered until after asus_remove() returns) sets
    update_ff = true, loads ff_work_initialized == true, is preempted before
    schedule_work().
  2. CPU B (hid_asus_ally_remove) clears the flag, runs cancel_work_sync()
    (nothing pending, returns), NULLs ally_x_hdev, returns; the core proceeds
    toward destroying hdev.
  3. CPU A resumes and calls schedule_work()work queued after the cancel,
    defeating the teardown guarantee.
  4. The worker reads ally->ally_x_hdev unlocked. There is no happens-before
    edge from B's NULL store to the worker, so it may observe the stale non-NULL
    pointer and issue hid_hw_raw_request() on a hid_device being freed.

Because ally_drvdata and ff_work are static module data, a work item queued
after the final cancel can also outlive module unload.

Note ally_x_play_effect() runs from the ff-memless timer (softirq), so it
cannot take ally_data_mutex — the fix uses the existing ff_lock spinlock
on both sides.

Fix: test the flag and queue the work under ff_lock; clear the flag under
the same lock before cancelling; snapshot ally_x_hdev under the lock in the
work function.


#6INIT_DELAYED_WORK on a possibly-queued work; unbalanced cancel — MEDIUM

fix at drivers/hid/hid-asus.c:724 and :5008

INIT_DELAYED_WORK(&ally->resume_work, ...) sat in hid_asus_ally_init(),
which is called from both probe and reset_resume. asus_resume() queues
that same work. Re-initializing a delayed work that may be queued or pending
corrupts the workqueue/timer state.

Separately, resume_work was never cancelled on any teardown path, while
the work reports through input devices owned by all three interfaces — which
unbind in arbitrary order.

Fix: initialise resume_work statically via __DELAYED_WORK_INITIALIZER()
so it is valid for the module's lifetime regardless of which interfaces probed,
and cancel_delayed_work_sync() it on every interface teardown.

Cancelling is deliberately done outside ally_data_mutex: a handler that
wants the mutex would otherwise deadlock against a cancel holding it.


#7 — NULL dereference in the resume work — MEDIUM

ally_resume_work_fn(), fix in the patch's first hunk

if (ally->keyboard_hdev) {
	input_report_key(ally->keyboard_input, KEY_F16, 0);

It tests one pointer and dereferences a different one. Probe sets
keyboard_hdev unconditionally but sets keyboard_input only
if (!list_empty(&hdev->inputs)). So on a keyboard interface that exposes no
input_dev, keyboard_hdev != NULL and keyboard_input == NULL
input_report_key(NULL, ...) → NULL deref on resume. Teardown (#3) makes the
window wider by clearing the two fields one after the other.

Fix: snapshot and test the pointers that are actually dereferenced.


ChimeraOS#8 — Response-curve writes performed outside the lock — LOW

response_curve_pct_store() / response_curve_move_store() /
response_curve_apply(), fix at drivers/hid/hid-asus.c:3077 and the two stores

scoped_guard(mutex, &cfg->config_mutex) wrapped only the switch that computes
the pointer curve_entry; the actual store *curve_entry = value; executed
after the guard released the mutex. The critical section protected a
computation over compile-time offsets — i.e. nothing.

(In the pct variant the guard had no braces so it covered only the switch;
in the move variant the braced block ended before the store. Both were wrong.)

The reader, response_curve_apply(), took no lock at all: it validated
monotonicity and then re-read all 8 entries when building the payload, so a
concurrent store between the two could push a non-monotonic curve to the MCU.

Severity is low — the store is a single u8, so no torn value is possible — but
the locking as written bought nothing.

Fix: move the stores inside the guard, and have the reader snapshot the
curve under the mutex before validating and sending.


ChimeraOS#9 — Stale globals and a leak on probe failure — LOW/MEDIUM

hid_asus_ally_probe(), fix at drivers/hid/hid-asus.c:4965

The HID_ALLY_INTF_CFG_IN case sets ally_drvdata.cfg_hdev and
ally_drvdata.config before failure paths that return ERR_PTR(). asus_probe()
only logs that failure and leaves drvdata->rog_ally == NULL, so
asus_remove() calls hid_asus_ally_remove(hdev, NULL), which returns early —
the globals are never cleared.

Reachable when ally_config_create() succeeds but hid_asus_ally_init() fails,
i.e. when ally_gamepad_check_ready() times out — a genuinely observed Ally
failure mode.

Two concrete consequences:

  • Leak (definite): ally_config_remove() is never called, so the 48
    kzalloc'd turbo/remap/macro attribute objects leak permanently.
  • Poisoned global (definite): ally_drvdata is one file-scope global shared
    by all Ally interfaces; a failed config probe leaves cfg_hdev pointing at a
    freed hid_device for the module's lifetime.

I could not construct a use-after-free from this on its own: cfg_hdev is
never dereferenced (only compared), and every ->config dereference is reached
through drvdata->rog_ally and NULL-guarded. It becomes exploitable only under
slab address reuse, which is too speculative to claim. Fixed anyway.

Fix: release the config and reset the globals on each error return.


Also fixed

19 pre-existing kernel-doc warnings (wrong/missing @param names on
handle_ctrl_alt_del, ally_check_capability, ally_set_joystick_thresholds,
ally_set_anti_deadzone, ally_set_trigger_ranges,
ally_set_joystick_resp_curve, and a /** block on hid_asus_ally_probe that
isn't kernel-doc). These would draw comment on the HID list.


Investigated and rejected — do not re-tread

  • NULL check after dereference in asus_usb_rgb_send_zone_effect() and
    asus_usb_rgb_commit() (the rgb->desc->... initialisers precede the
    if (!rgb ...) guard). rgb is provably non-NULL on every caller path, so
    the !rgb clause is dead code, not a live NULL deref. Reordered for hygiene
    only.
  • rgb->last_hw_level being per-device rather than per-zone. The MCU
    brightness level genuinely is global per device, so caching it per-device is
    correct. zone_count == 1 on all supported hardware today. Latent design note
    if multi-zone hardware ever ships, not a current defect.
  • asus_suspend() racing removal. Serialized by device_lock() around PM
    callbacks plus pm_runtime_get_sync() in __device_release_driver; no lock
    cycle in the flush path; rgb->desc points at a static const and cannot go
    stale.
  • data[0] read without a size check in asus_raw_event().
    __hid_input_report() rejects size == 0 before calling raw_event, so
    size >= 1 is a hard guarantee. (The data[1] accesses genuinely were
    unguarded — one of them a conditional write past the reported length — and
    are fixed.)

Structural note

Nearly every lifetime bug here (#3, #5, #6, ChimeraOS#9) traces back to one design
choice: ally_drvdata is a single file-scope global shared across the Ally's
three USB interfaces (keyboard 0x81, config 0x83, Ally-X gamepad 0x87).
The interfaces bind and unbind independently and in arbitrary order, and each
teardown path cleared only some of the shared fields.

The patch plugs the specific holes. Per-device state with proper refcounting
would remove the entire class — worth considering before this goes upstream,
since the "only one Ally can be connected at a time" comment justifies the
sharing but not the unsynchronized partial teardown.

The ROG ally needs to have the EC string sent back after resuming from
s2idle since the USB device can be turned completely off by the firmware
when mcu_powersave firmware-attribute is set to 1.

This may also be true for other laptops and certain features might stop
working after the device exit from sleep.

Signed-off-by: Denis Benato <denis.benato@linux.dev>
Implement the core functionality for ASUS handhelds:
- extend functionality of hid-asus if driver is enabled
- initialise the device to a "ready" state
- implement Ally X input mapping
- emit a single proper event for AC button long press

Assisted-by: Claude:claude-fable-5
Signed-off-by: Khamunetri Clark <khamunetriclark@gmail.com>
Signed-off-by: Denis Benato <denis.benato@linux.dev>
Signed-off-by: Matthew Schwartz <matthew.schwartz@linux.dev>
Signed-off-by: Luke Jones <luke@ljones.dev>
Signed-off-by: Jonathan LoBue <jlobue10@gmail.com>
Add the base configuration structures for the gamepad configuration,
detect capabilities and initialize the device in a known state.

Assisted-by: Claude:claude-fable-5
Signed-off-by: Denis Benato <denis.benato@linux.dev>
Signed-off-by: Luke Jones <luke@ljones.dev>
Signed-off-by: Jonathan LoBue <jlobue10@gmail.com>
ASUS ROG Ally handhelds support the vibration strength to be configured:
add sysfs attributes to allow userspace configure motors vibration
intensity.

Signed-off-by: Denis Benato <denis.benato@linux.dev>
Signed-off-by: Luke Jones <luke@ljones.dev>
ROG Ally devices supports configuring joysticks inner and outer range:
add sysfs attributes to allow userspace modifying the sensitivity
of those controllers.

Signed-off-by: Denis Benato <denis.benato@linux.dev>
Signed-off-by: Luke Jones <luke@ljones.dev>
ROG Ally devices allows configuring inner and outer ranges for triggers
buttons on the back: allow userspace to configure the sesitivity by
exposing sysfs attributes.

Signed-off-by: Denis Benato <denis.benato@linux.dev>
Signed-off-by: Luke Jones <luke@ljones.dev>
ROG Ally devices allow configuring the anti-deadzone parameter for
the resistive joysticks devices as over time those develops drift,
therefore allow userspace to configure the anti-deadzone by exposing
relevant sysfs attributes.

Signed-off-by: Denis Benato <denis.benato@linux.dev>
Signed-off-by: Luke Jones <luke@ljones.dev>
ROG ally devices allows configuring the response curve of both joysticks,
therefore add the ability of userspace to modify the response curve by
exposing relevant sysfs attributes.

Signed-off-by: Denis Benato <denis.benato@linux.dev>
Signed-off-by: Luke Jones <luke@ljones.dev>
ROG ally devices supports a feature called turbo buttons that allows
the user to quickly toggle button state by keeping a button pressed,
therefore allow userspace to configure the feature by exposing
relevant sysfs attributes.

Assisted-by: claude-fable-5
Signed-off-by: Denis Benato <denis.benato@linux.dev>
Signed-off-by: Khamunetri Clark <khamunetriclark@gmail.com>
Signed-off-by: Luke Jones <luke@ljones.dev>
Signed-off-by: Jonathan LoBue <jlobue10@gmail.com>
Add support for buttons remapping: the ability to assign different
button events to each one of the physical button on the device.

Assisted-by: Claude:claude-fable-5
Signed-off-by: Luke Jones <luke@ljones.dev>
Signed-off-by: Denis Benato <denis.benato@linux.dev>
Signed-off-by: John LoBue Jonathan LoBue <jlobue10@gmail.com>
ROG Ally devices can emulate either a mouse+keyboard (desktop mode)
or an gamepad device (xbox360 controller in ROG ally and a custom
DInput device on newer models): add support for switching the current
controller mode.

Signed-off-by: Luke Jones <luke@ljones.dev>
Signed-off-by: Denis Benato <denis.benato@linux.dev>
Unlike ROG ally the X version and following ones uses DInput protocol
and the force feedback needs to be implemented as its protocol is
vendor-specific, therefore add support for FF_RUMBLE with magnitude
scaling on a work-queue based approach to avoid using possibly
sleeping calls in atomic context.

Assisted-by: gpt-5.3-codex
Signed-off-by: Denis Benato <denis.benato@linux.dev>
Signed-off-by: Khamunetri Clark <khamunetriclark@gmail.com>
Signed-off-by: Luke Jones <luke@ljones.dev>
ASUS ROG laptops supports a protocol called Aura to control LEDs over
multiple zones: this protocol allows changing effects, colors,
brightness and transitioning speed across different colors on certain
effects.

Add support for said protocol to allow complete customization of
LEDs mounted on the ROG ally and laid out the foundation for all
other laptops.

Assisted-by: glm-5.2
Assisted-by: claude-fable-5
Signed-off-by: Denis Benato <denis.benato@linux.dev>
Signed-off-by: ALTRI
…xes)

Rebase onto the rewritten ally branch, which already incorporates most of
the original 9-fix audit series. Remaining changes:

- ally_x_input_report: shrink buttons[4] to buttons[3] so sizeof matches
  the real 16-byte 0x0B wire report (report ID + 12 axis bytes + 2 button
  bitmaps + 1 hat byte). The absorbed audit fix kept the over-declared
  struct, so the guard `size < 1 + sizeof(*in_report)` demands 17 bytes
  and rejects every gamepad report on Ally X hardware; input then falls
  through to generic HID handling on the N-KEY node. Field-verified on
  ROG Xbox Ally X (RC73XA): usbmon shows the interrupt-IN report is
  exactly 16 bytes.

- response_curve_pct_store/response_curve_move_store: assign through a
  single pointer selected in the switch so the store happens in one
  place under the config mutex.

- hid_asus_ally_probe error path: comment documenting why the cfg
  globals must be cleared on hid_asus_ally_init failure.

- Normalize indentation of the ally_drvdata block (stray leading spaces
  introduced when the resume_work initializer was applied to the branch).

Co-Authored-By: Claude Fable 5 <noreply@anthropic.com>
Claude-Session: https://claude.ai/code/session_01M3pWeekLNWsrqafc9erby3
@jlobue10

Copy link
Copy Markdown
Author

Rebased onto the rewritten ally branch (9bc9553) — and one critical piece of the audit series didn't survive the branch rewrite.

First: thanks for folding the audit fixes (and the PR #2 mapping/long-press fixes) into the new series — most of this PR became redundant overnight, which is the good outcome. I've rebased it down to the residual delta (+40/−32).

The critical bit that got missed: the buttons[3] correction. The rewritten branch kept the audit's original size guard but not its field-tested follow-up:

  • struct ally_x_input_report still declares uint8_t buttons[4]sizeof = 16, so the guard size < 1 + sizeof(*in_report) demands 17 bytes.
  • The real 0x0B interrupt-IN report on Ally X hardware is exactly 16 bytes (report ID + 12 axis bytes + 2 button bitmaps + 1 hat). Verified with usbmon on a ROG Xbox Ally X (RC73XA); HID_ALLY_X_INPUT_REPORT_SIZE 16 in the branch agrees.
  • Net effect on the current tip: every gamepad report is rejected in ally_x_raw_event() and input falls through to generic HID decode on the N-KEY node. The failure is deceptive — a subset of buttons (A/B/X, dpad, triggers) coincidentally still works through the generic path; Y/LB/RB/L3/R3/View/Menu/Xbox and both sticks go dead. We hit exactly this signature in field testing (10/25 button-matrix failures) before shrinking the struct to buttons[3], after which the same matrix passed 25/25.

What's left in this PR after the rebase:

  1. buttons[4]buttons[3] (the fix above; the guard expression itself is unchanged and correct once sizeof matches the wire).
  2. response_curve_pct_store/response_curve_move_store: switch selects a pointer, single store under the config mutex (style only).
  3. A comment on the hid_asus_ally_init failure path documenting why the cfg globals are cleared there.
  4. Indentation normalization of the ally_drvdata block (the hand-applied resume_work initializer left stray leading spaces).

Cross-model note: we scrubbed this against the other models before rebasing. buttons[3] is strictly more permissive than the current guard, so nothing that works today can regress; the original Ally (RC71L) never reaches this path (no 0x87 endpoint). One structural observation while doing that: ROG Ally X and Xbox Ally X share USB PID 0b05:1b4c, and cfg->is_ally_x is currently set but never read — if any Xbox-specific gating is ever needed, DMI board name (RC73XA = Xbox Ally X) is the only available discriminator.

Also: your KEY_PROG1 choice for the long-press codes is the right call over the KEY_F19 we proposed in #2 (F19 isn't in the keyboard input's keybit on the vendor-code models, and the suspend ghost-release list only covers PROG1/F16/F17) — with the mapping fix absorbed as well, #2 is fully superseded; happy to close it.

@jlobue10 jlobue10 changed the title HID: asus: fix 9 lifecycle/input-validation defects in Ally support (field-validated on Xbox Ally X) HID: asus: fix Ally X input-report size validation (residual audit fixes after ally branch rewrite) Jul 26, 2026
@jlobue10

Copy link
Copy Markdown
Author

Hardware validation of the current ally head eb5d9d59c088 on Xbox Ally X (RC73XA) — all green. Closing this PR as absorbed.

The rewritten head incorporates this PR's critical fix (buttons[4]buttons[3], restoring the 16-byte input-report size check to match the wire format) verbatim, so there is nothing left to merge. Only non-functional residue from my branch remains (a style refactor and one comment), not worth keeping a PR open for.

Test setup: kernel 7.2.0-rc4 with the head's hid-asus.c built as the in-tree module (installed into the initramfs, so cold boot exercised the real probe path). W=1 clean build.

Test Result
Full button matrix (face, dpad, bumpers, stick clicks, View/Menu/Xbox, triggers, stick rolls, paddles, front buttons short+long) 25/25 PASS — the input-report path this PR fixed is fully working
Suspend matrix, 3× s2idle cycles 41/41 PASS — all 6 interfaces rebind, config marker survives, post-resume button/rumble/front-button checks pass every cycle
Cold-boot capability detection (initramfs probe) PASS — xbox_controller=1, full per-button/trigger attr tree (the adapted #7 reordering works)
Trigger range init at probe PASS — reads 0/100 (previously 0/0)
RGB asus:rgb:joystick_rings: direction store/readback, strobe in effect_index PASS
LED brightness across suspend/resume (4 cycles total) PASS — brightness and visible ring survive; the removed last_brightness restore did not produce a dark ring in the normal resume path
kmsg scans (every phase/cycle) Clean — zero oops/UAF/WARN

Front-button semantics on this head, stable across every suspend cycle: front1 = F16 short / F17 long, front2 = PROG1 short and long (the merged convention — fine by us, see #2).

Two minor notes, not worth PRs:

  1. The comment above cdev->brightness = state->brightness; still describes the removed restore-last-non-zero-brightness behavior — stale.
  2. multi_intensity accepts and reads back values above multi_max_intensity (wrote 255 255 255, max declares 100 100 100) — readback/clamp inconsistency in the multicolor path.

Thanks for absorbing the fixes — happy to re-test future heads on this hardware.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants