Re: [PATCH 3/9] HID: haptic, multitouch: Check haptic prerequisite properly

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [Medium] HID: haptic: hid_haptic_check_pressure_unit() misses non-force unit mismatch
- [Medium] HID: multitouch: one-way is_haptic_touchpad leaks state to other MT inputs
--

commit 8758bcd8ba263569bfbee87a3d749e9ff7ea4489
Author: Rong Zhang <i@rong.moe>

HID: haptic, multitouch: Check haptic prerequisite properly

This commit stores the HID_DG_TOUCHPAD input device and validates pressure
unit annotations across usages in hid_haptic_check_pressure_unit(), ensuring
hid_haptic_input_configured() only initializes haptics for the matching input
device. It also converts td->is_haptic_touchpad into a one-way flag and guards
probe-time hid_haptic_*() calls with td->haptic.

> diff --git a/drivers/hid/hid-haptic.c b/drivers/hid/hid-haptic.c
> index 9cbacc827dc9c..27cc8a2d9b595 100644
> --- a/drivers/hid/hid-haptic.c
> +++ b/drivers/hid/hid-haptic.c
> @@ -48,17 +48,40 @@ void hid_haptic_feature_mapping(struct hid_device *hdev,
>  }
>  EXPORT_SYMBOL_GPL(hid_haptic_feature_mapping);
>  
> -bool hid_haptic_check_pressure_unit(struct hid_haptic_device *haptic,
> +void hid_haptic_check_pressure_unit(struct hid_haptic_device *haptic,
>  				    struct hid_input *hi, struct hid_field *field)
>  {
> -	if (field->unit == HID_UNIT_GRAM || field->unit == HID_UNIT_NEWTON) {
> -		haptic->force_logical_minimum = field->logical_minimum;
> -		haptic->force_physical_minimum = field->physical_minimum;
> -		haptic->force_resolution = input_abs_get_res(hi->input,
> -							     ABS_MT_PRESSURE);
> -		return true;
> +	if (hi->application != HID_DG_TOUCHPAD)
> +		return;
> +
> +	/* Don't care about input devices with no pressure sensor. */
> +	if (field->unit != HID_UNIT_GRAM && field->unit != HID_UNIT_NEWTON)
> +		return;
> +
> +	if (IS_ERR(haptic->input_dev))
> +		return;
> +
> +	if (!haptic->input_dev) {
> +		haptic->input_dev = hi->input;
> +		haptic->pressure_field = field;
> +		haptic->force_resolution = input_abs_get_res(hi->input, ABS_MT_PRESSURE);
> +		return;
>  	}
> -	return false;
> +
> +	if (haptic->input_dev == hi->input &&
> +	    haptic->pressure_field->unit == field->unit &&
> +	    haptic->pressure_field->logical_minimum == field->logical_minimum &&
> +	    haptic->pressure_field->physical_minimum == field->physical_minimum &&
> +	    haptic->force_resolution == input_abs_get_res(hi->input, ABS_MT_PRESSURE))
> +		return;
> +
> +	/*
> +	 * Reject if:
> +	 * - there are multiple input devices with pressure sensors
> +	 * - the pressure annotation mismatches
> +	 */
> +	haptic->input_dev = ERR_PTR(-EPROTO);
> +	return;
>  }

[Severity: Medium]
What happens if a HID_DG_TOUCHPAD input device has multiple
HID_DG_TIPPRESSURE usages where some specify HID_UNIT_GRAM or HID_UNIT_NEWTON
and others have a non-force unit (such as 0)?

In mt_input_mapping() -> mt_touch_input_mapping():

	case HID_DG_TIPPRESSURE:
		set_abs(hi->input, ABS_MT_PRESSURE, field,
			cls->sn_pressure);
		if (td->haptic)
			hid_haptic_check_pressure_unit(td->haptic, hi, field);

Because hid_haptic_check_pressure_unit() returns early when field->unit is
neither HID_UNIT_GRAM nor HID_UNIT_NEWTON before checking haptic->input_dev:

1. If an earlier HID_DG_TIPPRESSURE usage on hi->input has a valid force unit
and a later usage on the same hi->input has unit 0, set_abs() overwrites
ABS_MT_PRESSURE parameters and resets its resolution to 0 on hi->input, while
hid_haptic_check_pressure_unit() returns early instead of setting
haptic->input_dev = ERR_PTR(-EPROTO).

2. Conversely, if the first HID_DG_TIPPRESSURE usage on hi->input has unit 0
and a subsequent usage has HID_UNIT_GRAM or HID_UNIT_NEWTON, the first usage
is ignored and the subsequent usage sees !haptic->input_dev and accepts the
device despite the mismatched annotations across usages.

Should hid_haptic_check_pressure_unit() reject the device when a non-force
unit is encountered on the same hi->input as a force unit?

[ ... ]

> diff --git a/drivers/hid/hid-multitouch.c b/drivers/hid/hid-multitouch.c
> index 4e19a0c4d2381..d744ccc3e0b0b 100644
> --- a/drivers/hid/hid-multitouch.c
> +++ b/drivers/hid/hid-multitouch.c

[ ... ]

> @@ -1955,14 +1956,11 @@ static int mt_input_configured(struct hid_device *hdev, struct hid_input *hi)
>  	struct hid_report *report;
>  	int ret;
>  
> -	if (td->is_haptic_touchpad && (td->mtclass.name == MT_CLS_WIN_8 ||
> -	    td->mtclass.name == MT_CLS_WIN_8_FORCE_MULTI_INPUT ||
> -	    td->mtclass.name == MT_CLS_WIN_8_KEEP_LATENCY_ON_CLOSE)) {
> -		if (hid_haptic_input_configured(hdev, td->haptic, hi) == 0)
> -			td->is_haptic_touchpad = false;
> -	} else {
> -		td->is_haptic_touchpad = false;
> -	}
> +	if (td->haptic && !td->is_haptic_touchpad &&
> +	    (td->mtclass.name == MT_CLS_WIN_8 ||
> +	     td->mtclass.name == MT_CLS_WIN_8_FORCE_MULTI_INPUT ||
> +	     td->mtclass.name == MT_CLS_WIN_8_KEEP_LATENCY_ON_CLOSE))
> +		td->is_haptic_touchpad = hid_haptic_input_configured(hdev, td->haptic, hi);
>  
>  	list_for_each_entry(report, &hi->reports, hidinput_list) {
>  		rdata = mt_find_report_data(td, report);

[Severity: Medium]
When a composite HID device has a haptic HID_DG_TOUCHPAD collection alongside
another multitouch application collection (such as a HID_DG_TOUCHSCREEN or a
second non-haptic HID_DG_TOUCHPAD without a pressure sensor), does latching
td->is_haptic_touchpad to true here affect the other multitouch inputs on the
same hid_device?

Once mt_input_configured() configures the haptic touchpad hid_input and sets
td->is_haptic_touchpad = true, any subsequent hid_input processed by
mt_input_configured() -> mt_touch_input_configured() will see:

	if (td->is_haptic_touchpad)
		app->mt_flags |= INPUT_MT_TOTAL_FORCE;

and set INPUT_MT_TOTAL_FORCE on that non-haptic application as well.

Similarly, at runtime, mt_process_slot() and mt_sync_frame() only check
td->is_haptic_touchpad rather than verifying whether input matches
td->haptic->input_dev:

	if (td->is_haptic_touchpad)
		hid_haptic_pressure_increase(td->haptic, *slot->p);

and:

	if (td->is_haptic_touchpad)
		hid_haptic_pressure_reset(td->haptic);

Could touches on a non-haptic multitouch input on the same HID device feed
into and reset td->haptic->pressure_sum?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261102-hid-haptic-kmod-v1-0-a3de5626dade@rong.moe?part=3