Re: [PATCH 2/5] Input: imagis - do not report coordinates of released contacts

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

Pre-existing issues:
- [High] DMA mapping of stack variables in I2C transfer.
- [High] Missing I2C client data initialization leads to a guaranteed NULL pointer dereference during system suspend.
- [High] Unconditionally returning IRQ_HANDLED on I2C failure can cause an interrupt storm.
--

--- Patch [2]: [PATCH 2/5] Input: imagis - do not report coordinates of released contacts ---
commit e577f0168e2b3c0834df658e8cd2f5b4436f2fcb
Author: Markuss Broks <[email protected]>
Subject: Input: imagis - do not report coordinates of released contacts

This commit modifies the imagis driver to only report the position and
contact area for active contacts. It also prepares the driver for the
IST40xx family, which carries an event type in the contact area bits for
release records.

> diff --git a/drivers/input/touchscreen/imagis.c b/drivers/input/touchscreen/imagis.c
> index 7bbb00beec3be..7cc91f97c06e0 100644
> --- a/drivers/input/touchscreen/imagis.c
> +++ b/drivers/input/touchscreen/imagis.c

[ ... ]

> @@ -127,6 +127,8 @@ static irqreturn_t imagis_interrupt(int irq, void *dev_id)
>  	finger_pressed = FIELD_GET(IST3038C_FINGER_STATUS_MASK, intr_message);
>  
>  	for (i = 0; i < finger_count; i++) {
> +		bool pressed;
> +
>  		if (ts->tdata->protocol_b)
>  			error = imagis_i2c_read_reg(ts,

[Severity: High]
This isn't a bug introduced by this patch, but could this I2C read cause DMA
mapping issues?

In imagis_i2c_read_reg(), the variables reg_be and ret_be are allocated
on the stack:

    __be32 ret_be;
    __be32 reg_be = cpu_to_be32(reg);
    struct i2c_msg msg[] = {
        {
            .addr = ts->client->addr,
            .flags = 0,
            .buf = (unsigned char *)&reg_be,
            .len = sizeof(reg_be),
        },
        ...

Their addresses are passed directly to i2c_transfer(). If the underlying I2C
adapter relies on DMA and CONFIG_VMAP_STACK is enabled, these stack
addresses reside in vmalloc space and are not DMA-mappable, which can lead
to dma_map_single() failures or memory corruption.

>  						    ts->tdata->touch_coord_cmd + (i * 4),
> @@ -141,9 +143,13 @@ static irqreturn_t imagis_interrupt(int irq, void *dev_id)
>  			goto out;
>  		}

[Severity: High]
This is a pre-existing issue, but does this error path bypass the kernel's
spurious interrupt detector?

If imagis_i2c_read_reg() fails, the code jumps to the out label which
unconditionally returns IRQ_HANDLED.

If the controller fails to respond, the physical IRQ line might remain
asserted because the device status registers aren't cleared. By returning
IRQ_HANDLED instead of IRQ_NONE, the IRQ core will unmask the line and it
will immediately fire again, potentially creating an interrupt storm that
locks up the CPU core.

>  
> +		pressed = finger_pressed & BIT(i);
> +
>  		input_mt_slot(ts->input_dev, i);
> -		input_mt_report_slot_state(ts->input_dev, MT_TOOL_FINGER,
> -					   finger_pressed & BIT(i));
> +		input_mt_report_slot_state(ts->input_dev, MT_TOOL_FINGER, pressed);
> +		if (!pressed)
> +			continue;
> +
>  		touchscreen_report_pos(ts->input_dev, &ts->prop,
>  				       FIELD_GET(IST3038C_X_MASK, finger_status),
>  				       FIELD_GET(IST3038C_Y_MASK, finger_status),

[Severity: High]
This is a pre-existing issue, but could system suspend trigger a NULL pointer
dereference?

Looking at imagis_suspend(), it calls i2c_get_clientdata(client) to get the
driver data, but imagis_probe() never sets it via i2c_set_clientdata() or
dev_set_drvdata().

This means i2c_get_clientdata() will return NULL, and the very next line
will dereference it:

    struct imagis_ts *ts = i2c_get_clientdata(client);
    int error;

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

Would it be possible to initialize the client data in imagis_probe() to
prevent this?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=2