Re: [PATCH 2/2] Input: synaptics_tcm_i2c - add Synaptics TouchComm I2C touchscreen driver

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] Flawed chunking logic in `syna_recv_message()` fails to consume the entire I2C message for perfectly aligned payloads and uses incorrect read lengths for non-last chunks.
- [High] Missing upper bound on active object count enables a CPU-spinning infinite loop DoS and an out-of-bounds heap read via integer overflow.
- [Medium] Unbalanced regulator disable during device teardown if hardware recovery fails in resume.
--

commit 2e193cb965d6a8f43ae428d7f72355547d149239
Author: Vsevolod Nevorotov <sevanevorotov29@gmail.com>

Input: synaptics_tcm_i2c - add Synaptics TouchComm I2C touchscreen driver
    
This commit adds an input touchscreen driver for Synaptics TouchComm 
generation-1 controllers connected over I2C. It supports multi-touch, 
configurable touch report layouts, low-power wakeup gestures, and deep sleep 
mode during system suspend.

> diff --git a/drivers/input/touchscreen/synaptics_tcm_i2c.c b/drivers/input/touchscreen/synaptics_tcm_i2c.c
> new file mode 100644
> index 0000000000000..2553dabd39258
> --- /dev/null
> +++ b/drivers/input/touchscreen/synaptics_tcm_i2c.c
[ ... ]
> @@ -0,0 +1,1033 @@
[ ... ]
> +static int syna_recv_message(struct syna_tcm *ts)
> +{
[ ... ]
> +	/*
> +	 * Continued read: the rest arrives in chunks that repeat the marker and
> +	 * carry code 0x03, so only SYNA_RD_CHUNK_SIZE - 2 bytes of each one are
> +	 * payload. The trailing padding byte (0x5a) is read and verified in the
> +	 * final chunk.
> +	 */
> +	remaining = ts->payload_len - copied;
> +	while (remaining) {

[Severity: High]
If ts->payload_len is exactly 60 bytes, copied is 60 and remaining evaluates
to 0. Does this cause the loop to terminate prematurely and fail to read the
final chunk containing the required 0x5a padding byte? 

This could leave unread bytes in the device's TX buffer, desynchronizing the
I2C protocol and causing subsequent reads to retrieve stale bytes.

> +		bool last = (remaining <= SYNA_RD_CHUNK_SIZE - 3);
> +		unsigned int chunk = last ? remaining :
> +					    min(remaining - 1,
> +						(unsigned int)(SYNA_RD_CHUNK_SIZE - 3));
> +		unsigned int read_len = chunk + 2 + (last ? 1 : 0);

[Severity: High]
Does this logic compute incorrect read lengths for non-last chunks? It appears
to cap chunk at 61 bytes (SYNA_RD_CHUNK_SIZE - 3). This results in a read_len
of 63 bytes (61 + 2 + 0) rather than the expected 64-byte chunk limit.

[ ... ]
> +/* Extract a big field of @bits from the report's bit stream at @offset. */
> +static u32 syna_get_bits(const u8 *buf, unsigned int buf_len,
> +			 unsigned int offset, unsigned int bits)
> +{
> +	unsigned int byte = offset / 8;
> +	unsigned int bit = offset % 8;
> +	unsigned int done = 0;
> +	u32 out = 0;
> +
> +	if (!bits || bits > 32 || offset + bits > buf_len * 8)
> +		return 0;

[Severity: High]
Is it possible for offset + bits to overflow a 32-bit integer here? If offset
grows large enough, the overflow could bypass this bounds check and cause an
out-of-bounds heap read using the calculated byte index.

[ ... ]
> +static int syna_parse_touch_report(struct syna_tcm *ts)
> +{
[ ... ]
> +		case SYNA_TOUCH_FOREACH_END:
> +			if (!active_only) {
> +				if (++obj < ts->max_objects)
> +					idx = loop_start;
> +			} else if (have_active_count) {
> +				if (++seen < active)
> +					idx = loop_start;
> +			} else if (++seen < ts->max_objects &&

[Severity: High]
Since active acts as the loop bound here, a massive unvalidated value could
lock up the CPU in an IRQ handler, continuously incrementing offset until the
integer overflow in syna_get_bits() triggers the out-of-bounds memory read.

[ ... ]
> +		case SYNA_TOUCH_NUM_OF_ACTIVE_OBJECTS:
> +			active = data;
> +			have_active_count = true;
> +			if (!active)
> +				done = true;
> +			break;

[Severity: High]
The active variable receives a value directly from the hardware payload without
validation. If a faulty or malicious device provides a huge value, could this
lead to an unbounded loop in syna_parse_touch_report()?

[ ... ]
> +static int syna_reinit_hw(struct syna_tcm *ts)
> +{
> +	struct device *dev = &ts->client->dev;
> +	u8 report = SYNA_REPORT_TOUCH;
> +	int ret;
> +
> +	syna_power_down(ts);
> +	ret = syna_power_up(ts);
> +	if (ret)
> +		return ret;

[Severity: Medium]
If syna_power_up() fails, the regulators are left disabled.

[ ... ]
> +static int syna_resume(struct device *dev)
> +{
[ ... ]
> +		/* Wake up controller from deep sleep */
> +		ret = syna_exec_command(ts, SYNA_CMD_EXIT_DEEP_SLEEP, NULL, 0);
> +		if (ret) {
> +			dev_warn(dev, "failed to exit deep sleep (%d), recovering...\n", ret);
> +			ret = syna_reinit_hw(ts);
> +			if (ret)
> +				dev_err(dev, "failed to recover controller: %d\n", ret);
> +		}
> +
> +		ts->suspended = false;
> +		enable_irq(client->irq);

[Severity: Medium]
Does this swallow the error if syna_reinit_hw() fails? The error is logged,
but the function returns 0 and leaves the device bound. 

Since syna_power_up() failed earlier in the recovery path, the regulators are
left disabled. When the device is eventually unbound and devres triggers
syna_power_down(), will this result in an unbalanced regulator disable and a
kernel warning?

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260910070723.347768-1-sevanevorotov29@gmail.com?part=2