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

[sashiko-bot@kernel.org]

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

New issues:
- [High] The I2C read/write wrapper does not properly check for short transfers, and this patch leaks the resulting uninitialized stack memory to dmesg.

Pre-existing issues:
- [High] The patch fails to prevent dmesg flooding and causes the interrupt handler to incorrectly signal a spurious interrupt, which can permanently disable the touchscreen.
- [High] Dynamically adding a devres action in a threaded IRQ handler creates an inverted teardown order during device unbind, leading to a memory leak and Use-After-Free.
- [High] If the I2C sleep command fails during suspend, the function returns an error without re-enabling the IRQ, rendering the touchscreen permanently unresponsive.
- [High] The driver passes stack-allocated arrays to i2c_transfer(), violating DMA mapping rules and risking memory corruption.
--

commit ec332d9cd1c7923c04e4eec0684513387a92348d
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. This commit uses dev_err_ratelimited() and includes the first 16
bytes of the raw packet for context.

> diff --git a/drivers/input/touchscreen/ilitek_ts_i2c.c b/drivers/input/touchscreen/ilitek_ts_i2c.c
> index f0721af02adf2..8886bd39bc0b6 100644
> --- a/drivers/input/touchscreen/ilitek_ts_i2c.c
> +++ b/drivers/input/touchscreen/ilitek_ts_i2c.c
> @@ -241,7 +241,9 @@ static int ilitek_process_and_report_v6(struct ilitek_ts_data *ts)

[Severity: High]
This is a pre-existing issue, but does ilitek_process_and_report_v6() pass a
stack-allocated array to i2c_transfer()?

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 the underlying I2C host controller driver maps the buffer for DMA, it might
attempt to map a stack address. On modern kernels where VMAP_STACK is enabled,
can this cause an immediate panic because stack memory is not physically
contiguous, or lead to cacheline sharing corruption otherwise?

>  		return ilitek_process_pen_report(ts, buf);

[Severity: High]
This is a pre-existing issue, but does dynamically adding a devres action
in the threaded IRQ handler create an inverted teardown order?

The IRQ is registered via devm_request_threaded_irq() in probe. Later, the
ISR dynamically calls devm_add_action_or_reset() to register the pen cleanup:

drivers/input/touchscreen/ilitek_ts_i2c.c:ilitek_process_pen_report() {
    ...
    if (!input) {
        error = ilitek_pen_input_dev_init(dev, ts);

which calls devm_add_action_or_reset(dev, ilitek_unregister_pen_input, ts).

Because devm is a LIFO stack, the pen cleanup is placed above the IRQ release
action. During device unbind, devres_release_all() will run the pen cleanup
first, freeing the device, while the IRQ remains active. If an interrupt fires
before the IRQ is finally freed, could the ISR access the freed device and
cause a use-after-free, or attempt to append a new devres action while the
stack is being torn down?

>  
>  	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);

[Severity: High]
Does this new logging leak uninitialized kernel stack memory to dmesg?

Looking at ilitek_i2c_write_and_read(), it checks if error < 0, but
i2c_transfer() returns the number of successfully transferred messages:

drivers/input/touchscreen/ilitek_ts_i2c.c:ilitek_i2c_write_and_read() {
    ...
    error = i2c_transfer(client->adapter, msgs, ARRAY_SIZE(msgs));
    if (error < 0)
        return error;

If a short transfer occurs, this evaluates to false, so the wrapper returns 0
(success) despite the read buffer not being populated. When that happens,
won't this new log message directly print 16 bytes of uninitialized kernel
stack memory to the dmesg buffer?

>  		return -EINVAL;

[Severity: High]
This is a pre-existing issue, but does returning -EINVAL here cause the
interrupt handler to incorrectly signal a spurious interrupt?

When an unknown packet is received, ilitek_process_and_report_v6() returns
-EINVAL to the ISR, which then handles the error like this:

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;
    }

First, the ISR unconditionally logs the error, which defeats the ratelimiting
added in this patch. Second, could repeatedly returning IRQ_NONE when the
hardware interrupt was actually serviced (cleared by the I2C read) cause the
kernel's spurious interrupt detection to eventually disable the IRQ line
permanently?

>  	}
>  

[ ... ]

[Severity: High]
This is a pre-existing issue, but does ilitek_suspend() return an error
without re-enabling the previously disabled IRQ?

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 the I2C sleep command fails during suspend, the function returns an error
while the device IRQ is disabled. The PM core will abort the suspend
transition and resume previously suspended devices, but it will not call the
resume callback for the device that returned the error. Could this render the
touchscreen permanently unresponsive until reboot or module reload?

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