Re: [PATCH 09/11] HID: mod_devicetable: 'hid_device_id::driver_data' add union

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Adding a pointer variant to `hid_device_id` union encourages a dangerous architectural anti-pattern involving `driver_data`.
- [Low] The commit message inaccurately claims that `struct hid_device_id` is part of the userspace ABI.
--

commit c7768796e9018aff03a8509ee8a35c62af4417cf
Author: Pawel Zalewski (The Capable Hub) <pzalewski@thegoodpenguin.co.uk>

HID: mod_devicetable: 'hid_device_id::driver_data' add union

This commit adds a union to the driver_data field in struct hid_device_id
to provide a direct pointer representation.

> The change doesn't affect the size and layout of struct hid_device_id
> which is part of the userspace ABI.

Is this description accurate regarding the userspace ABI?

The structure struct hid_device_id is defined in
include/linux/mod_devicetable.h, which is an internal kernel header and is
not exported to include/uapi/.

While modpost uses the binary layout during the kernel build to generate
the text-based modules.alias file, userspace tools interact with the
generated text interface rather than the binary structure itself.

Could the commit message be updated to clarify that this layout is an
internal build ABI rather than a userspace ABI?

> diff --git a/include/linux/mod_devicetable.h b/include/linux/mod_devicetable.h
> index 23ff24080dfd9..90b2dae75109c 100644
> --- a/include/linux/mod_devicetable.h
> +++ b/include/linux/mod_devicetable.h
> @@ -175,7 +175,10 @@ struct hid_device_id {
>  	__u16 group;
>  	__u32 vendor;
>  	__u32 product;
> -	kernel_ulong_t driver_data;
> +	union {
> +		kernel_ulong_t driver_data;
> +		const void *driver_data_ptr;
> +	};
>  };

Does this code introduce a regression by encouraging an architectural
pattern where drivers assume they can dereference driver_data as a pointer?

The HID core supports dynamic device ID injection via the sysfs new_id
interface. In drivers/hid/hid-core.c, new_id_store() parses user input as
an unsigned long and assigns it to driver_data.

By adding driver_data_ptr, it explicitly blesses treating this field as a
kernel pointer at the core API level.

If a driver dereferences this user-provided value as a pointer during
probe, could it lead to arbitrary memory dereferences or lockdown bypasses?

Does it make more sense to keep driver_data restricted to an index or enum
to avoid exposing an API that facilitates this pattern?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260518-mod-devicetable-hid_device_id-v1-0-a08e3989c283@thegoodpenguin.co.uk?part=9