Re: [PATCH v6 2/2] Input: isa1200 - new driver for Imagis ISA1200

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [Critical] The `ff-memless` timer and input device can outlive the devres-managed driver state upon device unbind, leading to a Use-After-Free (UAF).
- [High] The worker `isa1200_play_work` silently drops force feedback events if it fails to acquire `input->mutex`, potentially leaving the vibrator stuck.
- [Medium] Concurrent access to `isa->level` across different locking contexts causes a data race that can schedule work after the device is closed.
--

commit 2ea8a9b82f2921004386e7307e4e382941aa5611
Author: Linus Walleij <linusw@kernel.org>

Input: isa1200 - new driver for Imagis ISA1200

The ISA1200 is a haptic feedback unit from Imagis Technology using two
motors for haptic feedback in mobile phones. Used in many mobile devices
c. 2012 including Samsung Galxy S Advance GT-I9070 (Janice), Samsung Beam
GT-I8350 (Gavini), LG Optimus 4X P880 and LG Optimus Vu P895.

> diff --git a/drivers/input/misc/isa1200.c b/drivers/input/misc/isa1200.c
> new file mode 100644
> index 0000000000000..f8c4da4a0e088
> --- /dev/null
> +++ b/drivers/input/misc/isa1200.c

[ ... ]

> +static void isa1200_play_work(struct work_struct *work)
> +{
> +	struct isa1200 *isa = container_of(work, struct isa1200, play_work);
> +	struct input_dev *input = isa->input;
> +
> +	scoped_guard(mutex_try, &input->mutex) {

[Severity: High]
Can this cause force feedback events to be silently dropped?

The worker uses scoped_guard(mutex_try, &input->mutex) to acquire the lock.
However, input->mutex is frequently held by the input core for normal
operations like input_open_device, input_close_device, and ioctls
(e.g., EVIOCGABS).

If this worker runs concurrently with any of these operations, the mutex_try
will fail and the worker will exit without updating the hardware. Because
isa->level was already updated locklessly in isa1200_vibrator_play_effect(),
future identical effect updates will be skipped, which could leave the
vibrator stuck in its current state.

> +		if (!isa->suspended) {
> +			if (isa->level)
> +				isa1200_start(isa);
> +			else
> +				isa1200_stop(isa);
> +		}
> +	}
> +}

[ ... ]

> +static void isa1200_vibrator_close(struct input_dev *input)
> +{
> +	struct isa1200 *isa = input_get_drvdata(input);
> +
> +	cancel_work_sync(&isa->play_work);
> +	isa1200_stop(isa);
> +	isa->level = 0;

[Severity: Medium]
Does this concurrent modification of isa->level create a data race that
can restart the vibrator after the device is closed?

The state variable isa->level is updated in isa1200_vibrator_play_effect()
under the input core's event_lock (timer context). Concurrently, it is reset
to 0 here in isa1200_vibrator_close() under input->mutex, without acquiring
event_lock.

Because the ff-memless timer is not explicitly stopped during close, the
timer can fire concurrently with or immediately after close. If it fires
after cancel_work_sync(&isa->play_work) completes, it can overwrite
isa->level to a non-zero value and re-schedule isa->play_work. This could
re-enable the motor indefinitely.

> +}

[ ... ]

> +static int isa1200_probe(struct i2c_client *client)
> +{
> +	struct isa1200 *isa;
> +	struct device *dev = &client->dev;
> +	int err;
> +
> +	isa = devm_kzalloc(dev, sizeof(*isa), GFP_KERNEL);

[Severity: Critical]
Is there a use-after-free risk here during device unbind?

The private isa structure is allocated with devm_kzalloc(), meaning it is
freed immediately when the device unbinds. However, the input device itself
is managed by devm_input_allocate_device(), which keeps the memory alive
as long as userspace holds an open file descriptor.

Because the ff-memless timer (ml_effect_timer) is tied to the input_dev
lifecycle, the timer can continue to fire even after unbind if an effect
was playing. When it fires, isa1200_vibrator_play_effect() retrieves the
freed isa structure via input_get_drvdata() and accesses corrupted memory
to schedule the workqueue.

Additionally, there doesn't appear to be a devm action added to explicitly
cancel isa->play_work on unbind.

> +	if (!isa)
> +		return -ENOMEM;
> +
> +	isa->input = devm_input_allocate_device(dev);

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260617070528.35006-1-clamor95@gmail.com?part=2