Re: [PATCH v6 1/2] HID: haptic: reorder try_module_get/get_device before input_ff_create

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] input: NULL pointer dereference in evdev on EVIOCGEFFECTS when hid_haptic_init fails
--

commit 2755afe0061688f07b774dbe02464fb532ab3852
Author: Aldo Ariel Panzardo <qwe.aldo@gmail.com>

HID: haptic: reorder try_module_get/get_device before input_ff_create

This commit moves try_module_get() and get_device() calls before
input_ff_create() to fix a dangling pointer and uninitialized return value
in the error path of hid_haptic_init().

> diff --git a/drivers/hid/hid-haptic.c b/drivers/hid/hid-haptic.c
> index 8760eeb08b2cd..592b44e020d6b 100644
> --- a/drivers/hid/hid-haptic.c
> +++ b/drivers/hid/hid-haptic.c
> @@ -501,11 +499,23 @@ int hid_haptic_init(struct hid_device *hdev,
>  
>  	input_set_capability(dev, EV_FF, FF_HAPTIC);
>  
> -	flush = dev->flush;
> -	event = dev->event;
> +	if (!try_module_get(THIS_MODULE)) {
> +		dev_err(&hdev->dev, "Failed to increase module count.\n");
> +		ret = -ENODEV;
> +		goto stop_buffer_free;
> +	}
> +	if (!get_device(&hdev->dev)) {
> +		dev_err(&hdev->dev, "Failed to get hdev device.\n");
> +		ret = -ENODEV;
> +		module_put(THIS_MODULE);
> +		goto stop_buffer_free;
> +	}
> +
>  	ret = input_ff_create(dev, FF_MAX_EFFECTS);
>  	if (ret) {
>  		dev_err(&hdev->dev, "Failed to create ff device.\n");
> +		put_device(&hdev->dev);
> +		module_put(THIS_MODULE);
>  		goto stop_buffer_free;
>  	}

[Severity: High]
Can this cause a NULL pointer dereference in evdev if try_module_get() or
get_device() fails?

By setting EV_FF on the device unconditionally via input_set_capability()
before these checks, if we hit the stop_buffer_free error path, EV_FF
remains set but dev->ff is never initialized.

Because the caller hid_haptic_input_configured() masks the error:

drivers/hid/hid-haptic.c:hid_haptic_input_configured() {
    ...
	error = hid_haptic_init(hdev, haptic, hi->input);
	if (error) {
		dev_warn(&hdev->dev, "Cannot allocate haptic for %s\n",
			 hdev->name);
		return 0;
	}
    ...
}

The input device will still be successfully registered with the EV_FF
capability. When userspace subsequently issues an EVIOCGEFFECTS ioctl,
evdev_do_ioctl() checks EV_FF and blindly dereferences dev->ff:

drivers/input/evdev.c:evdev_do_ioctl() {
    ...
	case EVIOCGEFFECTS:
		i = test_bit(EV_FF, dev->evbit) ?
				dev->ff->max_effects : 0;
    ...
}

Since dev->ff is NULL, this will cause a kernel panic. Should the EV_FF
capability be cleared in the stop_buffer_free error path, or only set
after these checks succeed?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261016151343.3120300-1-qwe.aldo@gmail.com?part=1