Re: [PATCH v3 2/3] Input: ilitek_ts: handle short I2C transfers

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

Pre-existing issues:
- [High] Missing IRQ re-enable on error paths in suspend/resume handlers.
- [High] Unsafe DMA Buffer Allocation for I2C transfers (stack variables passed to i2c_transfer).
- [High] Data parsing error for multi-touch reporting when more than 10 contacts are present.
--

--- Patch [2]: [PATCH v3 2/3] Input: ilitek_ts: handle short I2C transfers ---

commit 642c55e3cb3ec68f3ba0fdc9a106d372a6e3c257
Author: Kristian Mide [off-list ref]

Input: ilitek_ts: handle short I2C transfers

Check that i2c_transfer() returns the expected number of messages in
ilitek_i2c_write_and_read(). Short transfers were previously treated as
success, allowing callers to continue with partially filled buffers.

[Severity: High]
This isn't a bug introduced by this patch, but will passing stack buffers to
i2c_transfer() cause memory corruption or kernel panics if the I2C adapter
driver uses DMA?

For example, ilitek_process_and_report_v6() allocates a 512-byte buffer on the
stack and passes it down the call chain:

ilitek_process_and_report_v6()
  u8 buf[512];
  ...
  error = ilitek_i2c_write_and_read(ts, NULL, 0, 0, buf, 64);

And api_protocol_get_mcu_ver() does something similar:

api_protocol_get_mcu_ver()
  u8 buf[64];
  ...
  error = ilitek_i2c_write_and_read(ts, buf, 1, 5, outbuf, 32);

On systems with CONFIG_VMAP_STACK=y, stack pages are not physically
contiguous. Is there a risk of dma_map_single() silently failing or
corrupting memory when mapping these buffers?

 		if (error < 0)
 			return error;
+		if (error != ARRAY_SIZE(msgs))
+			return -EIO;

[Severity: High]
This is a pre-existing issue, but returning explicitly on short transfers
makes error paths more likely to be triggered. Are we leaking IRQ states in
the suspend and resume routines?

If api_protocol_set_cmd() fails due to this new -EIO return during
ilitek_suspend(), we return early:

ilitek_suspend()
  ...
  if (error)
      return error;

The function returns without re-enabling the interrupt that was disabled at
the start of the function. The PM core aborts the suspend transition and
won't call the resume callback, leaving the device interrupt permanently
disabled.

A similar situation happens in ilitek_resume():

ilitek_resume()
  ...
  if (error)
      return error;

This skips the enable_irq(client->irq) call at the end of the function.
Could this leave the touchscreen permanently unresponsive?

 	} else {
 		if (write_len > 0) {
 			error = i2c_transfer(client->adapter, msgs, 1);
 			if (error < 0)
 				return error;
+			if (error != 1)
+				return -EIO;
 		}

[ ... ]

[Severity: High]
This is also a pre-existing issue, but does the data parsing loop in
ilitek_process_and_report_v6() calculate offsets correctly when there are
more than 10 contacts?

When report_max_point > 10, the driver fetches subsequent data in 64-byte
chunks. However, the parsing loop calculates offsets linearly assuming a
continuous array:

ilitek_process_and_report_v6()
  for (i = 0; i < report_max_point; i++) {
      status = buf[i * packet_len + 1] & 0x40;
  ...

For the 11th point (where i = 10 and packet_len = 5), the offset is 51, which
falls inside the 14-byte padding at the end of the first 64-byte chunk. The
actual data for this point was written to the second chunk starting at
index 64.

Will this cause garbage touch events to be sent to userspace for the 11th
and subsequent fingers?

 		}
 	}

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