Re: [PATCH v2 3/3] Input: ilitek_ts: ratelimit unexpected report logging

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] The patch fails to prevent dmesg spam because the caller logs an un-ratelimited error for any negative return value.
- [Low] Direct bit manipulation is used to set single device capabilities instead of the standard helper function.

Pre-existing issues:
- [High] Unbalanced disable_irq() in suspend and resume error paths leaves the device interrupt permanently disabled.
- [High] Multi-touch data parsing accesses incorrect buffer offsets for reports with more than 10 active touch points, parsing garbage data.
- [High] Returning IRQ_NONE for a serviced hardware interrupt can cause the kernel to permanently disable the IRQ line.
- [High] I2C transfer buffers are allocated on the stack, which can cause arbitrary physical memory corruption when DMA is used with vmapped stacks.
--

--- Patch [3]: [PATCH v2 3/3] Input: ilitek_ts: ratelimit unexpected report logging ---
commit bf03da99321dcdb2c6e97a5d9bbe75d687cccec8
Author: Kristian Mide <kristian@mide.dk>

Input: ilitek_ts: ratelimit unexpected report logging
    
Unexpected report IDs can repeat quickly when the controller
emits unknown packets.
    
Use dev_err_ratelimited() and include the first 16 bytes of the
raw packet to keep dmesg usable while preserving enough context
for debugging.

> diff --git a/drivers/input/touchscreen/ilitek_ts_i2c.c b/drivers/input/touchscreen/ilitek_ts_i2c.c
> index 6146baa2b6062..e68bc052248ef 100644
> --- a/drivers/input/touchscreen/ilitek_ts_i2c.c
> +++ b/drivers/input/touchscreen/ilitek_ts_i2c.c
> @@ -238,7 +238,9 @@ static int ilitek_process_and_report_v6(struct ilitek_ts_data *ts)
>  		return ilitek_process_pen_report(ts, buf);
>  
>  	if (buf[0] != ILITEK_TP_I2C_REPORT_ID) {
> -		dev_err(dev, "get touch info failed. Wrong id: 0x%02X\n", buf[0]);
> +		dev_err_ratelimited(dev,
> +				    "get touch info failed. Wrong id: 0x%02X raw[0:16]=%*ph\n",
> +				    buf[0], 16, buf);
>  		return -EINVAL;
>  	}

[Severity: Medium]
Does this actually prevent dmesg spam?  The caller ilitek_i2c_isr() observes
the -EINVAL return value and unconditionally logs an un-ratelimited error
for any negative return value:

drivers/input/touchscreen/ilitek_ts_i2c.c:ilitek_i2c_isr() {
    ...
    error = ilitek_process_and_report_v6(ts);
    if (error < 0) {
        dev_err(&ts->client->dev, "[%s] err:%d\n", __func__, error);
        return IRQ_NONE;
    }
    ...
}

[Severity: High]
This is a pre-existing issue, but returning IRQ_NONE from ilitek_i2c_isr()
after successfully executing I2C reads and clearing the hardware interrupt
can trigger the kernel's spurious interrupt detector.

If the hardware emits a stream of unexpected report IDs, will repeatedly
returning IRQ_NONE permanently disable the IRQ line?

[Severity: High]
This is a pre-existing issue, but does ilitek_process_and_report_v6()
incorrectly parse multi-touch data for reports with more than 10 active
touch points?

For 11+ points, the data is split into multiple 64-byte reads, but the loop
calculates a linear byte offset that completely disregards the 64-byte chunk
boundaries:

drivers/input/touchscreen/ilitek_ts_i2c.c:ilitek_process_and_report_v6() {
    ...
    for (i = 0; i < report_max_point; i++) {
        status = buf[i * packet_len + 1] & 0x40;
    ...
}

For the 11th point (index 10), this offset calculation (10 * 5 + 1 = 51)
reads from the padding of the first 64-byte chunk instead of advancing to
the next chunk.  Does this result in parsing garbage data?

[Severity: High]
This is a pre-existing issue, but does allocating I2C transfer buffers on the
stack cause arbitrary physical memory corruption when DMA is used?

drivers/input/touchscreen/ilitek_ts_i2c.c:ilitek_process_and_report_v6() {
    ...
    u8 buf[512];
    ...
    error = ilitek_i2c_write_and_read(ts, NULL, 0, 0, buf, 64);
    ...
}

If CONFIG_VMAP_STACK is enabled, the stack may not be physically contiguous.
Can a DMA transfer crossing a physical page boundary silently overwrite
unrelated physical memory?  (This also happens in ilitek_read_tp_info()
with a 256-byte stack buffer).

[Severity: Low]
Does ilitek_pen_input_dev_init() use direct bit manipulation to set single
device capabilities instead of the standard input_set_capability() helper?

drivers/input/touchscreen/ilitek_ts_i2c.c:ilitek_pen_input_dev_init() {
    ...
    __set_bit(INPUT_PROP_DIRECT, pen_input->propbit);
    __set_bit(EV_KEY, pen_input->evbit);
    __set_bit(EV_ABS, pen_input->evbit);
    ...
}

[Severity: High]
This is a pre-existing issue, but do the suspend and resume error paths leave
the device interrupt permanently disabled?

drivers/input/touchscreen/ilitek_ts_i2c.c:ilitek_suspend() {
    ...
    disable_irq(client->irq);

    if (!device_may_wakeup(dev)) {
        error = api_protocol_set_cmd(ts, SET_IC_SLEEP, NULL, NULL);
        if (error)
            return error;
    }
    ...
}

If api_protocol_set_cmd() fails, it bails out before enable_irq() can be
called to balance the disable_irq().  A similar issue exists in
ilitek_resume().  Does a failure during suspend/resume permanently kill
touchscreen functionality?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260709182602.7308-1-kristian@mide.dk?part=3