Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Pre-existing issues:
- [High] Incomplete Buffer Overflow Fix (TOCTOU) / Out-of-bounds Read / Integer Underflow
- [High] Out-of-Bounds Read / Missing Length Validation for Touch Data
--
commit 0acfd851483a1efe80080d23464f2baa2b7b318e
Author: Linkai Gong [off-list ref]
Input: cyttsp5 - clamp the HID report size before memcpy
This commit attempts to fix a buffer overflow by clamping the size of the
HID report payload before copying it into the response buffer.
quoted hunk ↗ jump to hunk
diff --git a/drivers/input/touchscreen/cyttsp5.c b/drivers/input/touchscreen/cyttsp5.c
index 9266c07314bee..e878a02dc9b75 100644
--- a/drivers/input/touchscreen/cyttsp5.c
+++ b/drivers/input/touchscreen/cyttsp5.c
@@ -710,6 +710,7 @@ static irqreturn_t cyttsp5_handle_irq(int irq, void *handle)
size = 2;
} else {
report_id = ts->input_buf[2];
+ size = min(size, CY_MAX_INPUT);
}
switch (report_id) {
[Severity: High]
This isn't a bug introduced by this patch, but the patch provides an
incomplete fix. Although the local size variable is clamped here to protect
the memcpy, the unclamped size remains embedded within the copied payload in
ts->input_buf, which is then copied to ts->response_buf.
Later, cyttsp5_validate_cmd_response() reads the original unclamped size
directly from the buffer:
cyttsp5_validate_cmd_response() {
...
size = get_unaligned_le16(&ts->response_buf[0]);
...
if (ts->response_buf[size - 1] != HID_OUTPUT_BL_EOP) {
...
If the device provided a size greater than 512, does this result in an
out-of-bounds read?
Additionally, if the size is very small (e.g., 5), the expression size - 7
mathematically underflows when passed to crc_itu_t():
crc = crc_itu_t(0xFFFF, &ts->response_buf[4], size - 7);
Could we also validate the size value stored inside the buffer, or ensure
that the extracted size in cyttsp5_validate_cmd_response() falls within safe
bounds (>= 7 and <= CY_MAX_INPUT)?
[Severity: High]
This is also a pre-existing issue, but while we are looking at bounds checking
in this driver, there appears to be missing validation for the touch data
sizes as well.
In cyttsp5_get_sysinfo_regs(), scd->max_tch is set directly from the device
payload without capping it to fit within the static CY_MAX_INPUT buffer
bounds.
When an interrupt occurs, cyttsp5_mt_attention() extracts num_cur_tch from
the payload. There is no check to guarantee that num_cur_tch multiplied by
TOUCH_REPORT_SIZE fits within the statically allocated 512-byte buffer.
For large values of num_cur_tch, the loop in cyttsp5_get_mt_touches()
calculates pointers that exceed the 512-byte bound:
cyttsp5_get_mt_touches() {
...
for (i = 0; i < num_cur_tch; i++) {
tch_addr = ts->input_buf + offset + (i * TOUCH_REPORT_SIZE);
...
Could this lead to out-of-bounds reads of the ts->input_buf array,
potentially leaking adjacent kernel heap data into userspace via the input
events?
--
Sashiko AI review · https://sashiko.dev/#/patchset/20260901122649.1173066-1-gonglinkai@kylinos.cn?part=1