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

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [High] Flawed interaction between isa1200_stop, suspend, and isa->level leads to lost haptic state on resume and power leaks.
- [High] Missing workqueue cancellation and timer cleanup on driver unbind causes Use-After-Free.
- [High] Data races around hardware state transitions lead to resource leaks and I2C corruption.
- [Medium] Stack out-of-bounds read due to unvalidated GPIO descriptor count.
--

commit 87a2f648deb0c974007e802b082c1ec62d4fb097
Author: Linus Walleij <linusw@kernel.org>

Input: isa1200 - new driver for Imagis ISA1200

This commit introduces a new input driver for the Imagis ISA1200 haptic
feedback unit, adding support for controlling its motors over I2C and
PWM interfaces.

> diff --git a/drivers/input/misc/isa1200.c b/drivers/input/misc/isa1200.c
> new file mode 100644
> index 0000000000000..ff82252a08e1b
> --- /dev/null
> +++ b/drivers/input/misc/isa1200.c
[ ... ]
> +static void isa1200_start(struct isa1200 *isa)
> +{
> +	struct isa1200_config *config = &isa->config;
> +	struct device *dev = &isa->input->dev;
> +	struct pwm_state state;
> +	u8 hctrl0 = 0, hctrl1 = 0;
> +	DECLARE_BITMAP(values, ISA1200_EN_PINS_MAX);
> +	int err;
> +
> +	if (!isa->active) {
> +		err = regulator_bulk_enable(ARRAY_SIZE(isa1200_supplies),
> +					    isa->supplies);

Are isa1200_start() and isa1200_stop() vulnerable to data races on the
isa->active state?

The active flag is read and modified here without holding any locks.
If the workqueue and PM resume callbacks execute concurrently, could both
threads observe isa->active as false? This might lead to double
initialization of the clocks and regulators, leaking their reference
counts when isa1200_stop() only disables them once.

[ ... ]
> +static void isa1200_stop(struct isa1200 *isa)
> +{
[ ... ]
> +	isa->active = false;
> +	isa->level = 0;
> +}

[ ... ]
> +static int isa1200_of_probe(struct i2c_client *client)
> +{
[ ... ]
> +	isa->enable_gpios = devm_gpiod_get_array_optional(dev, "control",
> +							  GPIOD_OUT_LOW);

Should this validate the number of returned GPIO descriptors against
ISA1200_EN_PINS_MAX?

In isa1200_start() and isa1200_stop(), a local bitmap is declared on the
stack with DECLARE_BITMAP(values, ISA1200_EN_PINS_MAX). If a malformed
device tree provides more GPIOs than expected, could
gpiod_multi_set_value_cansleep() read past the stack bounds of the values
bitmap?

[ ... ]
> +static int isa1200_probe(struct i2c_client *client)
> +{
[ ... ]
> +	INIT_WORK(&isa->play_work, isa1200_play_work);

The isa structure is allocated via devm_kzalloc() and will be freed
automatically when the device is unbound. Because there is no remove
callback or devm action to cancel isa->play_work, could the workqueue
execute after unbind and cause a use-after-free if an effect is playing
during unbind?

Similarly, the ff_memless timer is tied to the input_dev, which can
outlive the I2C device if userspace holds an open file descriptor. Does
the timer also need to be cleaned up on driver unbind to prevent it from
accessing freed memory?

[ ... ]
> +static int isa1200_suspend(struct device *dev)
> +{
> +	struct isa1200 *isa = dev_get_drvdata(dev);
> +
> +	guard(mutex)(&isa->input->mutex);
> +
> +	if (input_device_enabled(isa->input)) {
> +		cancel_work_sync(&isa->play_work);
> +		if (isa->level)
> +			isa1200_stop(isa);
> +	}
> +
> +	return 0;
> +}

If the user has already requested the motor to stop (level is 0), this
skips calling isa1200_stop(). If the workqueue is cancelled before it
gets a chance to shut down the hardware, would this leave the device
active and leak power throughout suspend?

Additionally, looking at the resume function:

[ ... ]
> +static int isa1200_resume(struct device *dev)
> +{
> +	struct isa1200 *isa = dev_get_drvdata(dev);
> +
> +	guard(mutex)(&isa->input->mutex);
> +
> +	if (input_device_enabled(isa->input))
> +		if (isa->level)
> +			isa1200_start(isa);
> +
> +	return 0;
> +}

Since isa1200_stop() unconditionally sets isa->level to 0 during suspend,
will this check always evaluate to false, causing the driver to permanently
lose its haptic state after resuming?

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