Re: [PATCH] HID: wacom: validate GRAPHIRE_BT report length
From: sashiko-bot@kernel.org
Date: 2026-09-05 10:29:09
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider: Pre-existing issues: - [High] The length check added to wacom_graphire_irq is scoped exclusively to GRAPHIRE_BT devices, leaving other device types (e.g., WACOM_MO, WACOM_G4, GRAPHIRE) and other handlers (e.g., wacom_intuos_irq) vulnerable to identical stale-buffer reads. - [High] Missing cancellation of asynchronous timers and workqueues in the wacom_probe() error path leads to Use-After-Free. -- commit 60032e23affbdf87e0c943f82b7987db425fb939 Author: Aamir Ahmed [off-list ref] HID: wacom: validate GRAPHIRE_BT report length This commit adds a length check for GRAPHIRE_BT reports in wacom_graphire_irq() to prevent out-of-bounds reads on short packets. If the report length is less than expected, it logs a warning and exits.
quoted hunk ↗ jump to hunk
diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c index 8feb8027be95e..31da8b9566077 100644 --- a/drivers/hid/wacom_wac.c +++ b/drivers/hid/wacom_wac.c
[ ... ]
quoted hunk ↗ jump to hunk
@@ -350,6 +350,12 @@ static int wacom_graphire_irq(struct wacom_wac *wacom) int retval = 0; if (features->type == GRAPHIRE_BT) { + if (len < WACOM_PKGLEN_PENABLED) { + dev_warn(input->dev.parent, + "Graphire BT report too short: %zu bytes\n", + len); + goto exit; + }
[Severity: High]
This isn't a bug introduced by this patch, but does this bounds check
mistakenly limit itself to GRAPHIRE_BT devices?
If a spoofed or malfunctioning WACOM_MO, WACOM_G4, or GRAPHIRE HID device
sends a report with a length shorter than 9 bytes and a valid report ID,
wacom_graphire_irq() bypasses this new check.
It then proceeds to the fallback branch:
} else if (data[0] != WACOM_REPORT_PENABLED) {
and later accesses data[8] unconditionally for WACOM_MO:
case WACOM_MO:
prox = (data[7] & 0xf8) || data[8];
Can this result in an out-of-bounds read of the HID transport buffer,
leaking up to 8 bytes of uninitialized or stale memory into the input
subsystem?
quoted hunk ↗ jump to hunk
@@ -3475,7 +3481,7 @@ void wacom_wac_irq(struct wacom_wac *wacom_wac, size_t len) case GRAPHIRE: case GRAPHIRE_BT: case WACOM_MO: - sync = wacom_graphire_irq(wacom_wac); + sync = wacom_graphire_irq(wacom_wac, len); break; case PTU:
[Severity: High] This is a pre-existing issue, but is it possible to validate the report length for other handlers like wacom_intuos_irq()? Currently, wacom_intuos_irq() does not accept a len argument, which seems to leave it vulnerable to identical stale-buffer reads. [Severity: High] This is a pre-existing issue, but is there a missing cancellation of asynchronous timers and workqueues in the wacom_probe() error path? In drivers/hid/wacom_sys.c:wacom_probe(), if a subsequent initialization step fails after wacom_parse_and_register(), it propagates the failure: error = wacom_parse_and_register(wacom, false); if (error) return error; Inside wacom_parse_and_register(), the failure path stops the hardware but fails to cancel pending works like wacom->init_work or wacom->battery_work: fail_hw_stop: hid_hw_stop(hdev); fail: wacom_release_resources(wacom); return error; Since wacom is allocated via devm_kzalloc, it is immediately freed upon probe failure. When the pending work subsequently executes, might it access the freed wacom struct, causing a use-after-free? -- Sashiko AI review · https://sashiko.dev/#/patchset/AS8P251MB00012F559DCFF177B19F767BC8B42@AS8P251MB0001.EURP251.PROD.OUTLOOK.COM?part=1