Re: [PATCH 1/6] Input: mms114 - fix touch indexing for MMS134S and MMS136

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing validation for `touch->id == 0` causes multi-touch slot state corruption.
- [High] The devres allocation order in probe registers the input device's unregistration action before the regulators and IRQ are allocated, leading to a Use-After-Free.
- [Medium] Hardware I2C packet layout is mapped using compiler-dependent C bitfields.
--

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

Input: mms114 - fix touch indexing for MMS134S and MMS136

This commit fixes an issue where the driver iterates through touch events
using standard array indexing, assuming an 8-byte size. It explicitly
calculates the byte offset for each event based on the device's specific
event size (6 bytes for MMS134S and MMS136).

> diff --git a/drivers/input/touchscreen/mms114.c b/drivers/input/touchscreen/mms114.c
> index 4d748a13408d1..53ad35d61d476 100644
> --- a/drivers/input/touchscreen/mms114.c
> +++ b/drivers/input/touchscreen/mms114.c
> @@ -217,7 +217,9 @@ static irqreturn_t mms114_interrupt(int irq, void *dev_id)
>  	struct mms114_data *data = dev_id;
>  	struct i2c_client *client = data->client;
>  	struct mms114_touch touch[MMS114_MAX_TOUCH];
> +	struct mms114_touch *t;

[ ... ]

>  	for (index = 0; index < touch_size; index++) {
> -		switch (touch[index].type) {
> +		t = (struct mms114_touch *)((u8 *)touch + index * event_size);
> +

[Severity: Medium]
This is a pre-existing issue, but I noticed the hardware I2C packet layout
is mapped using C bitfields:

struct mms114_touch {
        u8 id:4, reserved_bit4:1, type:2, pressed:1;
        u8 x_hi:4, y_hi:4;
...

Because the layout of bitfields within a byte is compiler and endianness
dependent, will this read garbage for the id, type, and pressed fields
if the driver is compiled for a Big Endian system?

> +		switch (t->type) {
>  		case MMS114_TYPE_TOUCHSCREEN:
> -			mms114_process_mt(data, touch + index);
> +			mms114_process_mt(data, t);

[Severity: High]
This isn't a bug introduced by this patch, but does the multi-touch slot
processing handle an empty or invalid touch ID safely?

Looking at mms114_process_mt(), the slot index is calculated without a
check for zero:

mms114_process_mt() {
        ...
        id = touch->id - 1;
        ...
        input_mt_slot(input_dev, id);

If the device reports a touch event with touch->id == 0, id will underflow
to -1. Since the input core ignores negative slot values and leaves the
active slot unchanged, would subsequent coordinate reporting functions act
on this unchanged slot and silently corrupt the multi-touch tracking?

>  			break;
>  
>  		case MMS114_TYPE_TOUCHKEY:

[ ... ]

[Severity: High]
This is a pre-existing issue, but I have a question about the device cleanup
sequence during unbind.

In mms114_probe(), devm_input_allocate_device() is called before the
regulators and IRQ are allocated:

mms114_probe() {
        ...
        input_dev = devm_input_allocate_device(&client->dev);
        ...
        data->core_reg = devm_regulator_get(&client->dev, "avdd");
}

Because devres executes cleanup actions in reverse order (LIFO), during
unbind the IRQ and regulators are freed before input_unregister_device()
is called.

If the input device is held open by userspace during unbind, does
input_unregister_device() trigger mms114_input_close() and then
mms114_stop()?

mms114_stop() {
        ...
        disable_irq(client->irq);
        error = regulator_disable(data->io_reg);
        ...
}

Could this attempt to disable a freed IRQ and access freed regulators,
causing a use-after-free?

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