Re: [PATCH 3/6] Input: mms114 - use appropriate register argument types

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Stack-allocated memory is passed to I2C transfer functions, which is unsafe as I2C controllers may use DMA.
- [High] The driver doesn't handle a touch ID of 0 for touchscreen events, which causes a slot underflow and corrupts the previously active slot's state.
- [High] Incorrect initialization of Y-axis resolution register.
--

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

Input: mms114 - use appropriate register argument types

This commit updates the register arguments in the mms114 touch controller
driver to use 8-bit unsigned integers, matching the hardware specifications.
It also changes the value buffer pointer to void* to prevent explicit casting
when reading from the device.

> diff --git a/drivers/input/touchscreen/mms114.c b/drivers/input/touchscreen/mms114.c
> index db23b51f4630d..c2e006ac11960 100644
> --- a/drivers/input/touchscreen/mms114.c
> +++ b/drivers/input/touchscreen/mms114.c
> @@ -87,12 +87,12 @@ struct mms114_touch {
>  	u8 reserved[2];
>  } __packed;
>  
> -static int __mms114_read_reg(struct mms114_data *data, unsigned int reg,
> -			     unsigned int len, u8 *val)
> +static int __mms114_read_reg(struct mms114_data *data, u8 reg,
> +			     unsigned int len, void *val)
>  {
>  	struct i2c_client *client = data->client;
>  	struct i2c_msg xfer[2];
> -	u8 buf = reg & 0xff;
> +	u8 buf = reg;

[Severity: High]
This is a pre-existing issue, but I noticed we are using a stack-allocated
variable here.

Could this cause issues with DMA? The buf variable is allocated on the stack
and its address is assigned to xfer[0].buf for the outgoing I2C transfer.
Does this violate the DMA API prohibition on using stack-allocated memory
for I2C controllers that use DMA?

[ ... ]

> -static int mms114_write_reg(struct mms114_data *data, unsigned int reg,
> -			    unsigned int val)
> +static int mms114_write_reg(struct mms114_data *data, u8 reg, u8 val)
>  {
>  	struct i2c_client *client = data->client;
>  	u8 buf[2];
>  	int error;
>  
> -	buf[0] = reg & 0xff;
> -	buf[1] = val & 0xff;
> +	buf[0] = reg;
> +	buf[1] = val;
>  
>  	error = i2c_master_send(client, buf, 2);

[Severity: High]
This isn't a bug introduced by this patch, but this function also uses a stack
array.

Is it safe to pass the local stack array buf[2] directly to
i2c_master_send()? Does this risk stack corruption if the I2C controller
uses DMA?

[ ... ]

> @@ -242,9 +241,8 @@ static irqreturn_t mms114_interrupt(int irq, void *dev_id)
>  
>  	touch_size = packet_size / event_size;
>  
> -	error = __mms114_read_reg(data, MMS114_INFORMATION, packet_size,
> -			(u8 *)touch);
> -	if (error < 0)
> +	error = __mms114_read_reg(data, MMS114_INFORMATION, packet_size, touch);
> +	if (error)
>  		goto out;

[Severity: High]
This isn't a bug introduced by this patch, but the touch array is allocated
on the stack in this interrupt handler.

Since the touch array is passed as the read buffer for incoming I2C
touch events, could this lead to stack corruption if the I2C controller
performs DMA?


While reviewing this file, I also noticed a couple of other pre-existing
issues outside of this diff:

[Severity: High]
This is a pre-existing issue, but I noticed there isn't a check for touch ID
0 in mms114_process_mt():

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

If the hardware sends a touch event with touch->id == 0, id would underflow
to -1. When passed to input_mt_slot(), would this cause the input core to
ignore the slot update and overwrite the state of the previously active slot?


[Severity: High]
This is a pre-existing issue, but it looks like we might be writing the
X-resolution into the Y-resolution register in mms114_setup_regs():

    val = props->max_x & 0xff;
    error = mms114_write_reg(data, MMS114_Y_RESOLUTION, val);

Does val here inadvertently use props->max_x instead of props->max_y
when writing to MMS114_Y_RESOLUTION?

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