Re: [PATCH] HID: wacom: validate GRAPHIRE_BT report length

[sashiko-bot@kernel.org]

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 <elb12345@hotmail.co.uk>

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.

> 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

[ ... ]

> @@ -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?

> @@ -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