Re: [PATCH v3 5/8] HID: hid-tmff: clean up usage of 'driver_data'

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Missing hid_hw_stop() in the probe error path when an invalid driver_data index is provided.
- [High] Implicit conversion of an unsigned long to an enum type can lead to a negative index and an out-of-bounds array read.
- [Low] Implicit sizing of ff_bits_table makes the code susceptible to future out-of-bounds reads.
--

--- Patch [5]: [PATCH v3 5/8] HID: hid-tmff: clean up usage of 'driver_data' ---
commit d405905589b1ceab25490f9b10c3aef95f73bffc
Author: Pawel Zalewski (The Capable Hub) <[email protected]>

HID: hid-tmff: clean up usage of 'driver_data'

This patch modifies the hid_device_id::driver_data field to store an
index into ff_bits_table instead of a direct pointer address. This change
is intended to safely support the command line injection of a new_id device
by bounding retrieval at runtime.

> diff --git a/drivers/hid/hid-tmff.c b/drivers/hid/hid-tmff.c
> index 423f395d01ac5..93de756dd30e6 100644
> --- a/drivers/hid/hid-tmff.c
> +++ b/drivers/hid/hid-tmff.c
> @@ -34,6 +34,17 @@ static const signed short ff_joystick[] = {
>  	-1
>  };
>  
> +enum ff_bits_table_index {
> +	FF_BITS_TABLE_RUMBLE,
> +	FF_BITS_TABLE_JOYSTICK,
> +	NUM_OF_FF_BITS_IN_TABLE
> +};
> +
> +static const signed short *ff_bits_table[] = {
> +	[FF_BITS_TABLE_RUMBLE]		= ff_rumble,
> +	[FF_BITS_TABLE_JOYSTICK]	= ff_joystick
> +};

[Severity: Low]
This isn't a bug, but could we add an explicit size to the array like
ff_bits_table[NUM_OF_FF_BITS_IN_TABLE]?

If a future developer adds a new device type to the enum but forgets to
add a corresponding initializer here, NUM_OF_FF_BITS_IN_TABLE will increase
but the array size will not. The bounds check would then allow accessing
an unallocated index.

[ ... ]

> +static const signed short *get_ff_bits(enum ff_bits_table_index index)
> +{
> +	if (index >= NUM_OF_FF_BITS_IN_TABLE)
> +		return NULL;
> +
> +	return ff_bits_table[index];
> +}

[Severity: High]
Does this bounds check properly handle negative values? 

id->driver_data is an unsigned long injected via the sysfs new_id interface.
When a large unsigned value like 0xFFFFFFFF is passed to get_ff_bits(), it is
implicitly cast to enum ff_bits_table_index. 

On standard architectures, an enum without negative constants defaults to a
signed int. This casts the value to a negative integer, which bypasses the
upper-bound check and results in an out-of-bounds array read at
ff_bits_table[-1].

>  static int tm_probe(struct hid_device *hdev, const struct hid_device_id *id)
>  {
>  	int ret;
> +	const signed short *ff_bits = get_ff_bits(id->driver_data);
>  
>  	ret = hid_parse(hdev);
>  	if (ret) {
[ ... ]
> @@ -226,7 +246,13 @@ static int tm_probe(struct hid_device *hdev, const struct hid_device_id *id)
>  		goto err;
>  	}
>  
> -	tmff_init(hdev, (void *)id->driver_data);
> +	if (!ff_bits) {
> +		hid_err(hdev, "no ff_bits found\n");
> +		ret = -EINVAL;
> +		goto err;
> +	}

[Severity: High]
Should this error path call hid_hw_stop() before jumping to err?

The probe function previously called hid_hw_start() right above this check.
If an invalid driver_data index is provided via sysfs new_id, get_ff_bits()
returns NULL. The probe function then executes this goto err and returns
-EINVAL without stopping the hardware, which leaks resources allocated
by hid_hw_start().

> +
> +	tmff_init(hdev, ff_bits);
>  
>  	return 0;
>  err:

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