Re: [PATCH] HID: haptic: don't write an uninitialized value to unhandled usages

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

Pre-existing issues:
- [High] This problem wasn't introduced by this patch, but there is a Use-After-Free of work_struct when destroying the haptic device.
- [High] This problem wasn't introduced by this patch, but there is an unbalanced put_device() and module_put() in the initialization error path.
- [High] This problem wasn't introduced by this patch, but there is a probe crash (Null Pointer Dereference) in `hid_haptic_init()` when a device omits `HID_HP_AUTOTRIGGER`.
--

--- Patch [1]: [PATCH] HID: haptic: don't write an uninitialized value to unhandled usages ---
commit 969510b84661436472ff272c9047594db1487c3c
Author: Karl Mehltretter <[email protected]>

    HID: haptic: don't write an uninitialized value to unhandled usages

    fill_effect_buf() initializes value only for the four haptic usages
    handled by its switch, but writes it to field->value[] for every usage.
    An unhandled usage can therefore receive either an uninitialized value
    or one left over from the previous usage. hid_output_report() then
    serializes that value into the effect's report buffer.

> diff --git a/drivers/hid/hid-haptic.c b/drivers/hid/hid-haptic.c
> index deadab28cdbef..66d90f0f78a3b 100644
> --- a/drivers/hid/hid-haptic.c
> +++ b/drivers/hid/hid-haptic.c
> @@ -187,7 +187,7 @@ static void fill_effect_buf(struct hid_haptic_device *haptic,
>  				value = waveform_ordinal;
>  				break;
>  			default:
> -				break;
> +				continue;
>  			}
>  
>  			field->value[j] = value;

[Severity: High]
This problem wasn't introduced by this patch, but there is a Use-After-Free
of work_struct when destroying the haptic device.

drivers/hid/hid-haptic.c:hid_haptic_destroy() {
...
	if (haptic->effect) {
		for (r = 0; r < ff->max_effects; r++)
			kfree(haptic->effect[r].report_buf);
		kfree(haptic->effect);
	}
	haptic->effect = NULL;

	destroy_workqueue(haptic->wq);
...

Will destroy_workqueue() attempt to drain the queue and access pending work
items that reside inside the dynamically allocated haptic->effect, which was
just freed by kfree()?


[Severity: High]
This problem wasn't introduced by this patch, but there is an unbalanced
put_device() and module_put() in the initialization error path.

drivers/hid/hid-haptic.c:hid_haptic_init() {
...
	if (!try_module_get(THIS_MODULE)) {
		dev_err(&hdev->dev, "Failed to increase module count.\n");
		goto input_free;
	}
	if (!get_device(&hdev->dev)) {
		dev_err(&hdev->dev, "Failed to get hdev device.\n");
		module_put(THIS_MODULE);
		goto input_free;
	}
...

Does jumping to input_free invoke input_ff_destroy(), which then
unconditionally triggers hid_haptic_destroy()?

drivers/hid/hid-haptic.c:hid_haptic_destroy() {
...
	if (hdev)
		put_device(&hdev->dev);
...
	module_put(THIS_MODULE);
...

Will this unconditionally call put_device() and module_put() despite failing
to acquire them, causing reference count underflows?


[Severity: High]
This problem wasn't introduced by this patch, but there is a probe crash
(Null Pointer Dereference) in hid_haptic_init() when a device omits
HID_HP_AUTOTRIGGER.

If a malicious HID device advertises a Windows 8 Multitouch class and
includes a HID_DG_TIPPRESSURE field with force units, it appears
td->is_haptic_touchpad will be set to true.

drivers/hid/hid-multitouch.c:mt_input_configured() {
...
	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 {
...

Does mt_input_configured() leave is_haptic_touchpad set to true if
hid_haptic_input_configured() returns -1?

If this is bypassed, and mt_probe() subsequently calls hid_haptic_init():

drivers/hid/hid-haptic.c:hid_haptic_init() {
...
	mutex_init(&haptic->auto_trigger_mutex);
	for (r = 0; r < haptic->auto_trigger_report->maxfield; r++)
		parse_auto_trigger_field(haptic, haptic->auto_trigger_report->field[r]);
...

Will haptic->auto_trigger_report be NULL and cause an immediate kernel panic
when dereferenced if the device omitted HID_HP_AUTOTRIGGER?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1