Re: [PATCH 2/2] amd-sfh-tabletmode: interpret sfh tablet mode hid report

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] Unconditional `field->hidinput` dereference and tablet mode state corruption due to unconstrained `.event` callback.
- [High] The Kconfig symbol `INPUT_AMD_SFH_TABLETMODE` is missing a dependency on `HID`.
--

--- Patch [2]: [PATCH 2/2] amd-sfh-tabletmode: interpret sfh tablet mode hid report ---
commit 981d61514e0f554ad31b2f14f7fb391c059df340
Author: Helge Bahmann <hcb@chaoticmind.net>

amd-sfh-tabletmode: interpret sfh tablet mode hid report
    
Add a driver that hooks into the tablet mode hid reports
generated by the amd-sfh-hid driver. This enables tablet
mode switching on e.g. Asus Vivobook devices.

> diff --git a/drivers/input/misc/Kconfig b/drivers/input/misc/Kconfig
> index 94a753fcb64fa..5a1dacd6f7b9c 100644
> --- a/drivers/input/misc/Kconfig
> +++ b/drivers/input/misc/Kconfig
> @@ -1003,4 +1003,15 @@ config INPUT_STPMIC1_ONKEY
>  	  To compile this driver as a module, choose M here: the
>  	  module will be called stpmic1_onkey.
>  
> +config INPUT_AMD_SFH_TABLETMODE
> +	tristate "AMD sensor fusion hub tablet mode driver"

[Severity: High]
Is it possible this Kconfig entry is missing a depends on HID?

If the HID subsystem is disabled or configured as a module while this driver
is built-in, could this result in linking errors for HID core functions like
hid_register_driver() and hid_parse()?

> +	help
> +	  Say Y to enable support of the tablet mode switch driver for

[ ... ]

> diff --git a/drivers/input/misc/amd_sfh_tabletmode.c b/drivers/input/misc/amd_sfh_tabletmode.c
> new file mode 100644
> index 0000000000000..ee1207372207c
> --- /dev/null
> +++ b/drivers/input/misc/amd_sfh_tabletmode.c

[ ... ]

> +static int amd_sfh_tabletmode_event(struct hid_device *hid, struct hid_field *field,
> +				    struct hid_usage *usage, __s32 value)
> +{
> +	input_report_switch(field->hidinput->input, SW_TABLET_MODE, value);

[Severity: High]
Does this unconditionally dereference field->hidinput, which could be NULL
for unmapped usages?

Because the driver struct below lacks a .usage_table, hid_match_usage()
will default to true for all usages, meaning this callback intercepts every
incoming HID report field. This could lead to a NULL pointer dereference,
and will also blindly emit a SW_TABLET_MODE event for any report, causing
standard buttons or battery reports to randomly toggle tablet mode.

Since .input_mapping already maps the usage to EV_SW, allowing the generic
HID core to automatically report the event, could this callback be removed
entirely?

> +
> +	return 0;
> +}
> +
> +static const struct hid_device_id amd_sfh_tabletmode_devices[] = {
> +	{ HID_DEVICE(BUS_AMD_SFH, HID_GROUP_GENERIC, AMD_SFH_HID_VENDOR, AMD_SFH_HID_PRODUCT) },
> +	{ }
> +};
> +MODULE_DEVICE_TABLE(hid, amd_sfh_tabletmode_devices);
> +
> +static struct hid_driver amd_sfh_tabletmode_driver = {
> +	.name = "amd_sfh_tabletmode",
> +	.id_table = amd_sfh_tabletmode_devices,
> +	.probe = amd_sfh_tabletmode_probe,
> +	.remove = amd_sfh_tabletmode_remove,
> +	.input_mapping = amd_sfh_tabletmode_input_mapping,
> +	.event = amd_sfh_tabletmode_event,

[Severity: High]
Should this struct include a .usage_table?

Without it, all device usages are routed to the .event callback above.

> +};

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/ecadaac8-e222-420a-ac5b-4d529fb3317e@amd.com?part=2