Re: [PATCH] HID: wacom: validate report length before Intuos Pro2 BT pen parse

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:

Pre-existing issues:
- [High] The length checks introduced by the patch are insufficient. While they correctly bound the reads in `wacom_intuos_pro2_bt_pen()`, they fail to prevent out-of-bounds reads in the subsequent unconditional calls to the touch, pad, and battery parsing functions if the report length falls between the checked bounds and the maximum read offset.
--

--- Patch [1]: [PATCH] HID: wacom: validate report length before Intuos Pro2 BT pen parse ---
commit 1ba7672ecbf2a3b1f2ac28c705ac40735f4dcbcf
Author: Ibrahim Hashimov <security@auditcode.ai>

HID: wacom: validate report length before Intuos Pro2 BT pen parse

This commit adds length validation in wacom_intuos_pro2_bt_irq() to
prevent out-of-bounds reads before parsing pen reports for Intuos Pro2
and Gen3 Bluetooth devices.

> diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
> index da1f0ea85625d..89a191cf1924c 100644
> --- a/drivers/hid/wacom_wac.c
> +++ b/drivers/hid/wacom_wac.c
> @@ -1548,6 +1548,19 @@ static int wacom_intuos_pro2_bt_irq(struct wacom_wac *wacom, size_t len)
>  		return 0;
>  	}
>  
> +	if (wacom->features.type == INTUOSP2_BT ||
> +	    wacom->features.type == INTUOSP2S_BT) {
> +		if (len < 109) {
> +			dev_warn(wacom->pen_input->dev.parent,
> +				 "Pro2 BT report too short: %zu bytes\n", len);
> +			return 0;
> +		}
> +	} else if (len < 43) {
> +		dev_warn(wacom->pen_input->dev.parent,
> +			 "Pro2 BT report too short: %zu bytes\n", len);
> +		return 0;
> +	}
> +
>  	wacom_intuos_pro2_bt_pen(wacom);
>  	if (wacom->features.type == INTUOSP2_BT ||
>  	    wacom->features.type == INTUOSP2S_BT) {

[Severity: High]
This is a pre-existing issue, but are these length checks sufficient to prevent
out-of-bounds reads in the subsequent parsing functions?

While this successfully protects wacom_intuos_pro2_bt_pen(), the code goes on
to unconditionally call other parsing functions:

drivers/hid/wacom_wac.c:wacom_intuos_pro2_bt_irq() {
    ...
	wacom_intuos_pro2_bt_pen(wacom);
	if (wacom->features.type == INTUOSP2_BT ||
	    wacom->features.type == INTUOSP2S_BT) {
		wacom_intuos_pro2_bt_touch(wacom);
		wacom_intuos_pro2_bt_pad(wacom);
    ...
}

In wacom_intuos_pro2_bt_pad(), it reads up to offset 285:

drivers/hid/wacom_wac.c:wacom_intuos_pro2_bt_pad() {
    ...
	int ring = data[285] & 0x7F;
	bool ringstatus = data[285] & 0x80;
    ...
}

And for Gen3 devices, after the len < 43 check, it calls
wacom_intuos_gen3_bt_battery() which reads up to offset 45:

drivers/hid/wacom_wac.c:wacom_intuos_gen3_bt_battery() {
    ...
	bool chg = data[45] & 0x80;
	int battery_status = data[45] & 0x7F;
    ...
}

If a spoofed device sends a report that passes the initial minimum bounds
check (e.g., exactly 109 bytes for Pro2 or 43 bytes for Gen3), could this
still result in out-of-bounds reads when parsing the pad, touch, or battery
data?

>  		wacom_intuos_pro2_bt_touch(wacom);
>  		wacom_intuos_pro2_bt_pad(wacom);
>  		wacom_intuos_pro2_bt_battery(wacom);
>  	} else {
>  		wacom_intuos_gen3_bt_pad(wacom);
>  		wacom_intuos_gen3_bt_battery(wacom);
>  	}
>  	return 0;
>  }

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260713075110.92469-1-security@auditcode.ai?part=1