Re: [PATCH 1/2] amd-sfh-hid: tablet mode hid report and asus quirk

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:

New issues:
- [Medium] Missing HID Input Mapping for tablet mode switch
- [Low] Incomplete Implementation / Dead Code for Feature Report
- [Low] Unintentional Parameter Override via DMI Quirk
- [Low] Unused Macros for Tablet State Evaluated by Magic Number
- [Low] Introduction of Non-English Comments

Pre-existing issues:
- [High] Double Free / Use-After-Free in amdtp_hid_probe error path
--

--- Patch [1]: [PATCH 1/2] amd-sfh-hid: tablet mode hid report and asus quirk ---
commit 8025fc07a2e48355e8daeee6c47f30c940eb5a30
Author: Helge Bahmann <hcb@chaoticmind.net>

amd-sfh-hid: tablet mode hid report and asus quirk

Add an input driver that interprets the "operation mode" sensor offered
by the amd sfh as a hid driver to generate a custom hid report (tablet
mode switch).

Add a quirk to restore compatibility of the driver with the Asus
VivoBook (turn off the "disable interrupts flag). Expose the
intr_disable flag as a module parameter in case it turns out to be
needed on further laptop models.

> diff --git a/drivers/hid/amd-sfh-hid/amd_sfh_client.c b/drivers/hid/amd-sfh-hid/amd_sfh_client.c
> index 96ae792beeb6c..8c03279646023 100644
> --- a/drivers/hid/amd-sfh-hid/amd_sfh_client.c
> +++ b/drivers/hid/amd-sfh-hid/amd_sfh_client.c
[ ... ]
> @@ -327,12 +314,6 @@ int amd_sfh_hid_client_init(struct amd_mp2_dev *privdata)
>  
>  	for (i = 0; i < cl_data->num_hid_devices; i++) {
>  		cl_data->cur_hid_dev = i;
> -		if (cl_data->sensor_idx[i] == op_idx) {
> -			dev_dbg(dev, "sid 0x%x (%s) status 0x%x\n",
> -				cl_data->sensor_idx[i], get_sensor_name(cl_data->sensor_idx[i]),
> -				cl_data->sensor_sts[i]);
> -			continue;
> -		}
>  
>  		if (cl_data->sensor_sts[i] == SENSOR_ENABLED) {
>  			rc = amdtp_hid_probe(i, cl_data);

[Severity: High]
This isn't a bug introduced by this patch, but is a pre-existing issue in the
driver's error-handling path.

If amdtp_hid_probe() fails (e.g., in hid_add_device()), it destroys the hid
device but does not clear the dangling pointer in
cli_data->hid_sensor_hubs[cur_hid_dev].

When this failure propagates up to amd_sfh_hid_client_init(), it jumps to the
cleanup label and invokes amd_sfh_hid_client_deinit() -> amdtp_hid_remove().

amdtp_hid_remove() will then iterate over the hid_sensor_hubs array and call
hid_destroy_device() again on the dangling pointer, which can cause a double
free.

Should the pointer be set to NULL in amdtp_hid_probe() on error?

> diff --git a/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c b/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c
> index 4b81cebdc3359..924329bfef8fe 100644
> --- a/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c
> +++ b/drivers/hid/amd-sfh-hid/amd_sfh_pcie.c
> @@ -39,6 +39,8 @@ module_param_named(sensor_mask, sensor_mask_override, int, 0444);
>  MODULE_PARM_DESC(sensor_mask, "override the detected sensors mask");
>  
>  static bool intr_disable = true;
> +module_param_named(intr_disable, intr_disable, bool, 0444);
> +MODULE_PARM_DESC(intr_disable, "override the interrupt disable sensor bit");

[Severity: Low]
Because intr_disable is implemented as a simple boolean, the DMI match quirk
for Asus VivoBook unconditionally invokes mp2_disable_intr(), which
overwrites intr_disable to false.

Does this prevent the driver from differentiating between the default state
and an explicit user override on the command line (e.g.,
amd_sfh.intr_disable=1)?

> diff --git a/drivers/hid/amd-sfh-hid/hid_descriptor/amd_sfh_hid_desc.c b/drivers/hid/amd-sfh-hid/hid_descriptor/amd_sfh_hid_desc.c
> index ef1f9be8b8938..7e807b92d2960 100644
> --- a/drivers/hid/amd-sfh-hid/hid_descriptor/amd_sfh_hid_desc.c
> +++ b/drivers/hid/amd-sfh-hid/hid_descriptor/amd_sfh_hid_desc.c
[ ... ]
> @@ -164,6 +180,11 @@ static u8 get_feature_report(int sensor_idx, int report_id, u8 *feature_report)
>  		memcpy(feature_report, &gyro_feature, sizeof(gyro_feature));
>  		report_size = sizeof(gyro_feature);
>  		break;
> +	case op_idx: /* op */
> +		get_common_features(&tablet_mode_feature.common_property, report_id);
> +		memcpy(feature_report, &tablet_mode_feature, sizeof(tablet_mode_feature));
> +		report_size = sizeof(tablet_mode_feature);
> +		break;

[Severity: Low]
The driver allocates and populates a Feature report
(tablet_mode_feature_report) here, but the corresponding
tablet_mode_report_descriptor lacks any Feature items (0xB1).

Since the HID core strictly relies on the parsed descriptor to create reports,
will a feature report with ID 0x11 ever be requested, or is this unused
dead code?

> @@ -238,6 +260,15 @@ static u8 get_input_report(u8 current_index, int sensor_idx, int report_id,
>  		memcpy(input_report, &gyro_input, sizeof(gyro_input));
>  		report_size = sizeof(gyro_input);
>  		break;
> +	case op_idx: /* op */
> +		tablet_mode_input.report_id = 0x11;
> +		if (sensor_virt_addr[0] == 3)
> +			tablet_mode_input.tablet_state = 1;
> +		else
> +			tablet_mode_input.tablet_state = 0;

[Severity: Low]
This code relies on a hardcoded magic number 3. Should this evaluate the
sensor payload using the newly defined AMD_SFH_OP_IDX_MODE_TABLET macro
instead?

> diff --git a/drivers/hid/amd-sfh-hid/hid_descriptor/amd_sfh_hid_desc.h b/drivers/hid/amd-sfh-hid/hid_descriptor/amd_sfh_hid_desc.h
> index 882434b1501ff..ff82e47eb3ca4 100644
> --- a/drivers/hid/amd-sfh-hid/hid_descriptor/amd_sfh_hid_desc.h
> +++ b/drivers/hid/amd-sfh-hid/hid_descriptor/amd_sfh_hid_desc.h
> @@ -114,4 +114,13 @@ struct hpd_input_report {
>  	u8 human_presence;
>  } __packed;
>  
> +struct tablet_mode_feature_report {
> +	struct common_feature_property common_property;
> +} __packed;
> +
> +struct tablet_mode_input_report {
> +	u8 report_id;    /* Muss 0x11 (17) sein */

[Severity: Low]
Is this German comment intentional? Kernel coding style typically requires all
comments to be written in English.

> diff --git a/drivers/hid/amd-sfh-hid/hid_descriptor/amd_sfh_hid_report_desc.h b/drivers/hid/amd-sfh-hid/hid_descriptor/amd_sfh_hid_report_desc.h
> index 67ec2d6a417de..f95368534fc60 100644
> --- a/drivers/hid/amd-sfh-hid/hid_descriptor/amd_sfh_hid_report_desc.h
> +++ b/drivers/hid/amd-sfh-hid/hid_descriptor/amd_sfh_hid_report_desc.h
> @@ -776,4 +776,22 @@ static const u8 hpd_report_descriptor[] = {
>  0X81, 0x02,		 /* HID Input (Data_Var_Abs) */
>  0xC0			 /* HID end collection */
>  };
> +
> +/* tablet mode switch */
> +static const u8 tablet_mode_report_descriptor[] = {
> +0x05, 0x0C,		/* HID usage consumer electronics */
> +0x09, 0x01,		/* HID usage */
> +0xA1, 0x00,		/* HID collection (Physical) */
> +0x85, 0x11,		/* HID report id */
> +0x0A, 0xFF, 0x02,	/* HID usage (unallocated -- (ab)used for tablet mode) */

[Severity: Medium]
Because the driver does not provide a custom .input_mapping callback to
intercept and map this unallocated Consumer usage to SW_TABLET_MODE, will the
standard hid-input core ignore it or map it to KEY_UNKNOWN (240)?

Could this result in physical changes in tablet mode emitting spurious unknown
key events instead of a standard EV_SW switch event?

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