Re: [PATCH 00/11] HID: storing pointers in 'hid_device_id::driver_data'

[Benjamin Tissoires <bentiss@kernel.org>]

On May 18 2026, Pawel Zalewski (The Capable Hub) wrote:
> 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

I would agree with sashiko that the series introduces an anti-pattern by
allowing .driver_data to be an arbitrary pointer. The reason is that we
can dynamically set driver_data through the kernel command line, and
when it's used as a pointer, bad things will happen.

I would think we should fix the 2 offenders to enforce not using a
direct pointer but an offset in a table of pointers.

How does that sound for you?

Cheers,
Benjamin

>  
> One of the ways to fixing this duality and improve code readability
> and type-safety a bit is to use a '{kernel_ulong_t, const void *}'
> union. That way the current drivers that treat 'hid_device_id::driver_data'
> as an integer value for storing metadata are unaffected. The drivers
> that actually store pointers in there benefit from a removed cast
> (and more clear intent) at the cost of using the new 'const void *'
> field instead.
> 
> With the union in place, some of the existing initializers for static
> const data now need a named field for the 'driver_data' - this is
> also addressed in the series as part of the pre-clean up 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 and post-clean up can be found in 
> patches 9-11.
> 
> 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 as 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.
> 
> 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/ 
> 
> ---
> Pawel Zalewski (The Capable Hub) (11):
>       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: mod_devicetable: 'hid_device_id::driver_data' add union
>       HID: hid-tmff: use 'hid_device_id::driver_data_ptr'
>       HID: wacom: use 'hid_device_id::driver_data_ptr'
> 
>  drivers/hid/hid-asus.c                   |  46 ++-
>  drivers/hid/hid-belkin.c                 |   5 +-
>  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                 | 575 ++++++++++++++++++++-----------
>  drivers/hid/hid-tmff.c                   |  22 +-
>  drivers/hid/i2c-hid/i2c-hid-dmi-quirks.c |   2 +-
>  drivers/hid/wacom_sys.c                  |  14 +-
>  drivers/hid/wacom_wac.c                  |  10 +-
>  include/linux/mod_devicetable.h          |   5 +-
>  12 files changed, 496 insertions(+), 270 deletions(-)
> ---
> base-commit: 25ccf4586bead3fe3cf2c57ff0480f31a0e335ad
> change-id: 20260427-mod-devicetable-hid_device_id-7f30d877387c
> 
> Best regards,
> --  
> Pawel Zalewski (The Capable Hub) <pzalewski@thegoodpenguin.co.uk>
>