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 [off-list ref]

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.

[ ... ]

+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