Re: [PATCH 1/3] Input: mms114 - fix multi-touch slot corruption

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

Pre-existing issues:
- [High] If `mms114_start()` fails during system resume, subsequent device close or suspend causes unbalanced `disable_irq()` and `regulator_disable()` calls, leaving the touchscreen permanently broken.
- [High] The suspend callback releases all touches before disabling the device interrupt, creating a race window where touches can get permanently stuck.
- [High] Stack-allocated buffers are passed directly to I2C transfer functions, which can cause DMA mapping failures or memory corruption.
--

commit 6a55987ddd51d1330fd1636cd3e72b4d910ea42b
Author: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Input: mms114 - fix multi-touch slot corruption

This commit fixes a multi-touch slot corruption bug in the mms114 driver.
When the touchscreen controller reports a touch ID of 0, the driver
calculates the slot ID as touch->id - 1, which underflows to UINT_MAX. The
driver fixes this by rejecting touch reports with ID 0.

> diff --git a/drivers/input/touchscreen/mms114.c b/drivers/input/touchscreen/mms114.c
> index 006dded17eb80..23e0283bc6b8b 100644
> --- a/drivers/input/touchscreen/mms114.c
> +++ b/drivers/input/touchscreen/mms114.c
> @@ -248,7 +248,7 @@ static void mms114_process_mt(struct mms114_data *data, struct mms114_touch *tou
>  	unsigned int x;
>  	unsigned int y;
>  
> -	if (touch->id > MMS114_MAX_TOUCH) {
> +	if (touch->id == 0 || touch->id > MMS114_MAX_TOUCH) {
>  		dev_err(&client->dev, "Wrong touch id (%d)\n", touch->id);
>  		return;
>  	}

[Severity: High]
This isn't a bug introduced by this patch, but I have a question about the
error handling in mms114_resume().

If mms114_start() fails (e.g. mms114_setup_regs() returns an error) it cleans
up the regulators but doesn't call enable_irq():

static int mms114_start(struct mms114_data *data)
{
...
	error = mms114_setup_regs(data);
	if (error < 0) {
		regulator_disable(data->io_reg);
		regulator_disable(data->core_reg);
		return error;
	}

	enable_irq(client->irq);
	return 0;
}

In mms114_resume(), the error is passed back up but the input device remains
logically open:

static int mms114_resume(struct device *dev)
{
...
	if (input_device_enabled(input_dev)) {
		error = mms114_start(data);
		if (error)
			return error;
	}
...
}

Because the input device is still logically enabled, won't a subsequent
suspend or input_close call mms114_stop(), which unconditionally calls
disable_irq() and regulator_disable()? Does this cause an unbalanced IRQ
disable depth that permanently masks the interrupt?

[Severity: High]
This is a pre-existing issue, but I noticed a potential race condition in
mms114_suspend().

The suspend callback releases all active touch slots before stopping the
device and disabling the interrupt:

static int mms114_suspend(struct device *dev)
{
...
	/* Release all touch */
	for (id = 0; id < MMS114_MAX_TOUCH; id++) {
		input_mt_slot(input_dev, id);
		input_mt_report_slot_inactive(input_dev);
	}

	input_mt_report_pointer_emulation(input_dev, true);
	input_sync(input_dev);

	guard(mutex)(&input_dev->mutex);

	if (input_device_enabled(input_dev))
		mms114_stop(data);
...
}

If a touch interrupt fires between the synthetic slot release and the call to
mms114_stop() (which disables the IRQ), could the physical touches be reported
as active again just as the device is suspended, leaving the touches
permanently stuck on resume?

[Severity: High]
This isn't a bug introduced by this patch, but there seem to be multiple
places where stack-allocated buffers are used for I2C transfers.

For example, in __mms114_read_reg(), a single byte stack variable is passed:

static int __mms114_read_reg(struct mms114_data *data, u8 reg,
			     unsigned int len, void *val)
{
...
	u8 buf = reg;
...
	xfer[0].buf = &buf;
...
	error = i2c_transfer(client->adapter, xfer, 2);
...
}

Similarly in mms114_write_reg():

static int mms114_write_reg(struct mms114_data *data, u8 reg, u8 val)
{
...
	u8 buf[2];

	buf[0] = reg;
	buf[1] = val;

	error = i2c_master_send(client, buf, 2);
...
}

And in mms114_interrupt():

	struct mms114_touch touch[MMS114_MAX_TOUCH];
...
	error = __mms114_read_reg(data, MMS114_INFORMATION, packet_size, touch);
...

Since many I2C controllers use DMA, doesn't passing stack memory to
i2c_transfer() risk DMA mapping failures or memory corruption when
CONFIG_VMAP_STACK is enabled?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260704060115.353049-1-dmitry.torokhov@gmail.com?part=1