Re: [PATCH v3 6/8] HID: wacom: cleanup usage of 'driver_data'

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Missing lower-bound check on the enum index allows an attacker to bypass bounds checking and trigger an out-of-bounds read by injecting a large value via the `new_id` sysfs interface.
--

--- Patch [6]: [PATCH v3 6/8] HID: wacom: cleanup usage of 'driver_data' ---
commit b219fca8b4be38408fca90649c9b3228cd0bfa8c
Author: Pawel Zalewski (The Capable Hub) <pzalewski@thegoodpenguin.co.uk>

HID: wacom: cleanup usage of 'driver_data'

This commit changes the driver_data field in the HID device ID table
from a direct pointer to an integer index. This index is then used to
look up the appropriate wacom_features struct from a static array.
This change aims to make it safer to inject new device IDs from userspace.

> diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
> index 0eafa483b7f75..834519751a5cf 100644
> --- a/drivers/hid/wacom_sys.c
> +++ b/drivers/hid/wacom_sys.c
[ ... ]
> @@ -2843,7 +2847,15 @@ static int wacom_probe(struct hid_device *hdev,
>  	wacom->hdev = hdev;
>  
>  	wacom_wac = &wacom->wacom_wac;
> -	wacom_wac->features = *((struct wacom_features *)id->driver_data);
> +
> +	const struct wacom_features *id_features = get_wacom_features(id->driver_data);
> +
> +	if (!id_features) {
> +		hid_err(wacom->hdev, "could not get wacom id_features.\n");
> +		return -EINVAL;
> +	}
> +	wacom_wac->features = *id_features;

[Severity: High]
If a large value is injected here via the new_id sysfs interface, it gets
passed as id->driver_data to get_wacom_features(). Could the returned
id_features pointer be out of bounds, causing a memory access violation
when dereferenced during this assignment in wacom_probe()?

> diff --git a/drivers/hid/wacom_wac.c b/drivers/hid/wacom_wac.c
> index da1f0ea85625d..d0a864d525098 100644
> --- a/drivers/hid/wacom_wac.c
> +++ b/drivers/hid/wacom_wac.c
[ ... ]
> +const struct wacom_features *get_wacom_features(enum wacom_features_table_index index)
> +{
> +	if (index >= NUM_OF_WACOM_FEATURES_IN_TABLE)
> +		return NULL;
> +
> +	return wacom_features_table[index];
> +}

[Severity: High]
Since the enum constants fit within a signed 32-bit integer, the compiler
typically implements the enum wacom_features_table_index as a signed int.

If id->driver_data is a large enough value, it could be implicitly downcast
to a negative signed integer when passed to get_wacom_features(). A negative
value would bypass the upper-bound check here since a negative number is
less than NUM_OF_WACOM_FEATURES_IN_TABLE.

Does this missing lower-bound check allow get_wacom_features() to return
an invalid pointer from before the wacom_features_table array? Would it be
safer to check if index is less than zero, or change the parameter to an
unsigned type?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260727-mod-devicetable-hid_device_id-v3-0-980896ca283e@thegoodpenguin.co.uk?part=6