Linux Input/HID development
 help / color / mirror / Atom feed
* [PATCH v2 00/12] HID: storing pointers in 'hid_device_id::driver_data'
@ 2026-06-10 15:11 Pawel Zalewski (The Capable Hub)
  2026-06-10 15:11 ` [PATCH v2 01/12] HID: hid-input: use named initializer for 'hid_battery_quirks[]' Pawel Zalewski (The Capable Hub)
                   ` (11 more replies)
  0 siblings, 12 replies; 14+ messages in thread
From: Pawel Zalewski (The Capable Hub) @ 2026-06-10 15:11 UTC (permalink / raw)
  To: Jiri Kosina, Benjamin Tissoires, Ping Cheng, Jason Gerecke
  Cc: linux-kernel, linux-input, Pawel Zalewski (The Capable Hub)

The <linux/mod_devicetable.h> has multiple structs that follow
the pattern of having either 'driver_data' or 'driver_info'
fields which are of the 'kernel_ulong_t' type. Then how to 
interpret that field is user defined, some users will treat
the value as an actual integer, others as a valid pointer to
dereference. 

One of instances of the above is the 'hid_device_id::driver_data'
field, for the most part it is used for setting HID quirks and
treated as an integer value for storing metadata in the subsystem
drivers. But in a few instances it is used as a valid pointer to
dereference, namely in:
- hid-tmff
- wacom
 
Additionally this field can be set from the userspace via the
'hid_core::new_id_store' function - which is not memory safe and
could lead to OOB memory access in the above two cases. 

This series is addressing the issue by making the usage of the field
consistent, all drives treat the value as an integer and if the
driver has a need for dereferencing a pointer - it stores and 
index to a table of pointers. The index is bounded by a named enum
and checked at runtime at the module level to avoid developer bugs.

Furthermore a user-input sanitizer was added in the 'hid_core' module
that restricts injecting arbitrary values for 'hid_device_id::driver_data'
that are not defined and matched within the driver already.

It was found that some modules that use the field do not use a named
initializers for the field - which is inconsistent with rest of the
modules within the HID subsystem. This is also addressed in the series
as part of the pre-clean up to increase clarity in patches 1-4. 

It was found that some modules use a bit of a type-unsafe way of storing
integers in the 'void *driver_data' pointer of the 'struct hid_device'
- this required a cast during storage via 'hid_set_drvdata' and a cast
during retrieval when using 'hid_get_drvdata'. I can see why this was 
done - as we potentially save on an allocation - but really code is
more readable and better quality without resorting to this. This issue
is also addressed in this patch series in patches 5-8 as part of the
pre-clean up.

The actual implementation of the table of pointers approach 
can be found in patches 9-11. In case of wacom driver the sad thing
about it is that it requires a bit of macro magic for sanity. 

However, in doing so it was discovered that in the wacom driver
there is a 1:1 mapping between the 'driver_data' field and the 
product id - therefore patch 12 shows that the lookup can be
simplified as there is no need for using 'driver_data' at all
(it also does not require macro magic) - but it was left in a 
separate patch to show the working - happy to squash in v3 if
this would be accepted.

The change also makes the code more portable on architecture 
like CHERI [1], where a pointer is replaced with a new primitive
(called the capability) at the architecture level and is twice as
wide as the greatest representable address, ie. for 64 bit address
space capabilities are 128 bits wide (the other 64 bits are used to
store meta-data relating to the 64 bit address). So you can not store 
valid pointers inside 'unsigned long' as effectively a different set of
instructions is being generated by the compiler based on the data-type
that was used in C (ie. capabilities have their own set of load/store
that also copy over the meta-data which are orthogonal to the load/store
instructions used for plain integers that would invalidate the meta-data).
There is slightly more detail to this, but the above is enough to
explain the motivation - the proposed changes make the code a bit
better even without considering CHERI at all - as it is more readable 
and type-safe.

The series was built and tested under QEMU (boots with relevant 
configs set to Y) on arm64. Additionally the operation of the input 
sanitizer was tested via the following:

```

modprobe wacom
echo "0x0003 0x056a 0x0001 0x00000" > /sys/bus/hid/drivers/wacom/new_id

echo "0x0003 0x056a 0x0000 0x000A1" > /sys/bus/hid/drivers/wacom/new_id

echo "0x0003 0x056a 0x0000 0x000A2" > /sys/bus/hid/drivers/wacom/new_id
-sh: echo: write error: Invalid argument

modprobe wacom
echo "0x0003 0x056a 0x0001 0x00000" > /sys/bus/hid/drivers/wacom/new_id

echo "0x0003 0x056a 0x0001 0x00001" > /sys/bus/hid/drivers/wacom/new_id
-sh: echo: write error: Invalid argument
echo "0x0003 0x056a 0x0001 0x00010" > /sys/bus/hid/drivers/wacom/new_id
-sh: echo: write error: Invalid argument
```

The 'hid-tmff' and other randomly picked modules were also tested as 
above. Furthermore the './hid-wacom.sh' test was run from the user space
and all tests (except the expected to-do's) passed as required.

This series is part of a larger effort led by Uwe [2]

[1] https://cheri-alliance.org/discover-cheri/
[2] https://lore.kernel.org/all/cover.1776429984.git.u.kleine-koenig@baylibre.com/ 

---
Changes in v2:
- Patches 1-4: No change
- Patches 5-8: Additional signed-off-by as patches were applied.
- Patches 9-12: Rewritten and new.
- Link to v1: https://patch.msgid.link/20260518-mod-devicetable-hid_device_id-v1-0-a08e3989c283@thegoodpenguin.co.uk

To: Jiri Kosina <jikos@kernel.org>
To: Benjamin Tissoires <bentiss@kernel.org>
To: Ping Cheng <ping.cheng@wacom.com>
To: Jason Gerecke <jason.gerecke@wacom.com>
Cc: linux-input@vger.kernel.org
Cc: linux-kernel@vger.kernel.org

---
Pawel Zalewski (The Capable Hub) (12):
      HID: hid-input: use named initializer for 'hid_battery_quirks[]'
      HID: hid-quirks: use named initializer in 'hid_quirks[]'
      HID: hid-asus: use named initializer for 'asus_devices[]'
      HID: i2c-hid-dmi-quirks: use named initializer for 'i2c_hid_elan_flipped_quirks[]'
      HID: hid-belkin: clean up usage of 'driver_data'
      HID: hid-cypress: clean up usage of 'driver_data'
      HID: hid-gfrm: clean up usage of 'driver_data'
      HID: hid-ite: clean up usage of 'driver_data'
      HID: hid-tmff: clean up usage of 'driver_data'
      HID: wacom: cleanup usage of 'driver_data'
      HID: hid-core: sanitize user input in 'new_id_store'
      HID: wacom: do not use 'driver_data'

 drivers/hid/hid-asus.c                   |  46 ++-
 drivers/hid/hid-belkin.c                 |   5 +-
 drivers/hid/hid-core.c                   |  18 +
 drivers/hid/hid-cypress.c                |  32 +-
 drivers/hid/hid-gfrm.c                   |   8 +-
 drivers/hid/hid-input.c                  |  38 +-
 drivers/hid/hid-ite.c                    |   9 +-
 drivers/hid/hid-quirks.c                 | 578 ++++++++++++++++++++-----------
 drivers/hid/hid-tmff.c                   |  48 ++-
 drivers/hid/i2c-hid/i2c-hid-dmi-quirks.c |   2 +-
 drivers/hid/wacom.h                      |   1 +
 drivers/hid/wacom_sys.c                  |  25 +-
 drivers/hid/wacom_wac.c                  | 194 ++++++++++-
 drivers/hid/wacom_wac.h                  |   5 +
 14 files changed, 731 insertions(+), 278 deletions(-)
---
base-commit: c1f7d1aa16a4f3b5d9476bf3716ca6bb8271c910
change-id: 20260427-mod-devicetable-hid_device_id-7f30d877387c

Best regards,
--  
Pawel Zalewski (The Capable Hub) <pzalewski@thegoodpenguin.co.uk>


^ permalink raw reply	[flat|nested] 14+ messages in thread
end of thread, other threads:[~2026-06-10 15:32 UTC | newest]

Thread overview: 14+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2026-06-10 15:11 [PATCH v2 00/12] HID: storing pointers in 'hid_device_id::driver_data' Pawel Zalewski (The Capable Hub)
2026-06-10 15:11 ` [PATCH v2 01/12] HID: hid-input: use named initializer for 'hid_battery_quirks[]' Pawel Zalewski (The Capable Hub)
2026-06-10 15:11 ` [PATCH v2 02/12] HID: hid-quirks: use named initializer in 'hid_quirks[]' Pawel Zalewski (The Capable Hub)
2026-06-10 15:11 ` [PATCH v2 03/12] HID: hid-asus: use named initializer for 'asus_devices[]' Pawel Zalewski (The Capable Hub)
2026-06-10 15:11 ` [PATCH v2 04/12] HID: i2c-hid-dmi-quirks: use named initializer for 'i2c_hid_elan_flipped_quirks[]' Pawel Zalewski (The Capable Hub)
2026-06-10 15:11 ` [PATCH v2 05/12] HID: hid-belkin: clean up usage of 'driver_data' Pawel Zalewski (The Capable Hub)
2026-06-10 15:11 ` [PATCH v2 06/12] HID: hid-cypress: " Pawel Zalewski (The Capable Hub)
2026-06-10 15:11 ` [PATCH v2 07/12] HID: hid-gfrm: " Pawel Zalewski (The Capable Hub)
2026-06-10 15:11 ` [PATCH v2 08/12] HID: hid-ite: " Pawel Zalewski (The Capable Hub)
2026-06-10 15:11 ` [PATCH v2 09/12] HID: hid-tmff: " Pawel Zalewski (The Capable Hub)
2026-06-10 15:11 ` [PATCH v2 10/12] HID: wacom: cleanup " Pawel Zalewski (The Capable Hub)
2026-06-10 15:11 ` [PATCH v2 11/12] HID: hid-core: sanitize user input in 'new_id_store' Pawel Zalewski (The Capable Hub)
2026-06-10 15:32   ` sashiko-bot
2026-06-10 15:11 ` [PATCH v2 12/12] HID: wacom: do not use 'driver_data' Pawel Zalewski (The Capable Hub)

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox