Re: [PATCH v4] HID: multitouch: fix use-after-free of haptic data on delayed input release

[bentiss]

Hi,

> mt_probe() allocates td->haptic with devm_kzalloc(), tying its
> lifetime to the HID device's driver unbind.  hid_haptic_init() then
> stores the pointer in ff->private and installs hid_haptic_destroy()
> as the force-feedback destroy callback.
> 
> When the HID device is removed while a process still holds an evdev
> fd, devres frees td->haptic at unbind time.  hid_haptic_destroy()
> runs later from input_dev_release() and dereferences freed memory.
> The input core then calls kfree(ff->private) on the same pointer,
> double-freeing it.
> 
> The existing get_device()/put_device() pair in init/destroy pins the
> struct hid_device but does not keep its devres allocations alive,
> since devres runs at driver unbind, not at the final device kref put.
> 
> Replace devm_kzalloc() with plain kzalloc() so the haptic struct
> survives driver unbind.  Track ownership explicitly:
> 
>  - Add an 'owner' back-pointer (to td->haptic) and an 'ff_owned'
>    flag to struct hid_haptic_device.
> 
>  - hid_haptic_destroy() NULLs *owner under a lock before the input
>    core frees ff->private, so td->haptic becomes NULL and later
>    kfree(td->haptic) calls are harmless.
> 
>  - hid_haptic_release() safely frees the struct only when the FF
>    subsystem does not own it.
> 
>  - In mt_remove(), hid_haptic_detach() disconnects the back-pointer
>    before hid_hw_stop() so a deferred destroy (from a still-open
>    evdev fd) does not write into the devres-freed mt_device.

Sorry, but even if you are fixing a valid issue, I really dislike the
approach. devm_* is introduced to make code more readable and somehow
safer (which failed that point here), but removing it feels like a
pretty big step back.

Can't we achieve the same result by adding a specific devm cleanup
action instead of just removing it entirely?

I'd be fine with a custom devm_add_action_or_reset() in
hid-multitouch.c, but of course a full conversion of the hid-haptic
functionality into a devm_* api with things like
devm_hid_haptic_allocate() would be even better (but not required).


> 
> Additionally, reorder hid_haptic_init() to call try_module_get() and
> get_device() before input_ff_create(), eliminating the input_free
> error path that called input_ff_destroy() without setting a return
> code -- which left td->haptic as a dangling pointer on those
> (theoretical) failures.

I think this should go into its own separate path.

Cheers,
Benjamin

-- 
Benjamin Tissoires <bentiss@xxxxxxxxxx>