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) [off-list ref]

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.

[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) {

[ ... ]

[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