1. Patchew
  2. linux
  3. View series

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

Pawel Zalewski (The Capable Hub) posted 11 patches 6 days, 12 hours ago 
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(-)
[PATCH 00/11] HID: storing pointers in 'hid_device_id::driver_data'
Posted by Pawel Zalewski (The Capable Hub) 6 days, 12 hours ago 
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
 
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>
Re: [PATCH 00/11] HID: storing pointers in 'hid_device_id::driver_data'
Posted by Pawel Zalewski 2 days, 14 hours ago 
> 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 have not considered the command line override ! This indeed would not
be a good case for having a pointer at all in there - in the current
form of the codebase.

The series does not introduce this anti-pattern - this is already the
current status quo in the codebase and the problem predates the patch
series itself. The series makes the problem more visible and validates
the existing status quo - agreed on this.

> 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?

That could work, probably via a named enum (so that the idx is bounded
and validated to avoid out of bounds memory access) with its items mapped
into a table of const pointers. That way the command line override still
works as before and my goals set out in the series are met as well.

Perhaps an alternative solution would be to sanitize the user-input instead in
the 'hid-core::new_id_store' function, such that only 'driver_data' values
that match an existing entry in the driver's 'id_table' are accepted, this
is how it is done currently in the PCI subsystem in 'pci-driver::new_id_store',
so it would be consistent as well.

The bonus here is that all other drivers in the  HID subsystem benefit from
this aproach as opposed to just the two current offenders, as providing
'driver_data' from the command line is now mandatory and must match the
existing table entry - which is what we want ?

So something among the lines of:

@hid-core::new_id_store

```
struct hid_driver *hdrv = to_hid_driver(drv);
const struct hid_device_id *ids = hdrv->id_table;

(...)

/* Only accept driver_data values that match an existing id_table
    entry */
if (ids) {
        ret = -EINVAL;
        while (ids->vendor || ids->product) {
                if (driver_data == ids->driver_data) {
                        ret = 0;
                        break;
                }
                ids++;
        }
        if (ret)        /* No match */
                return ret;
}
```

Would that approach work for you ? Do help in what is the correct
termination for the while loop - for this demo I went with 'ids->vendor'
and 'ids->product' fields as common-sense would indicate that quirks need
to be device-specific by definition but that was without any actual research.

The downside is that 'new_id' drivers that actually use a pointer for the
'driver_data' will be rejected - which would be actually covered by the
solution that you have proposed in the first place. To think of it the
input sanitizer could work additionaly in tandem with the enum-based
solution if this fact is an issue, so it might not be an alternative
actually but a complement for it.

In the meantime I think that patches 1-8 can be reviewed regardless
without any changes (as they predate patch 9 that adds the union and the
commit messages does not mention anything about it).

Best regards,
Pawel

Patchew 2.1-devel-ea86937

© 2016 - 2026 Red Hat, Inc.