Input: focaltech - use signed coordinates to prevent underflow
Subsystems (2)
Dmitry Torokhov
maintainerPATCH 1/1 Input: focaltech - use signed coordinates to prevent underflow
focaltech_finger_state stores finger coordinates x and y as unsigned int. When processing relative packets, negative deltas can cause unsigned integer underflow if the finger moves past the left or bottom boundary of the touchpad, wrapping the coordinates to values near UINT_MAX.
When clamping the coordinates in focaltech_report_state(), these underflowed values are clamped against priv->x_max / priv->y_max instead of 0, causing the cursor to jump erratically to the opposite edge of the touchpad.
Change the coordinate variables and limits to signed int so that negative values resulting from relative movements clamp correctly to 0.
--- drivers/input/mouse/focaltech.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)
drivers/input/mouse/focaltech.c +6 -6
@@ -78,8 +78,8 @@ struct focaltech_finger_state {* Absolute position (from the bottom left corner) of the * finger. */ -unsigned int x; -unsigned int y; +int x; +int y; };/*
@@ -108,7 +108,7 @@ struct focaltech_hw_state { };struct focaltech_data { -unsigned int x_max, y_max; +int x_max, y_max;struct focaltech_hw_state state; };
@@ -126,14 +126,14 @@ static void focaltech_report_state(struct psmouse *psmouse)input_mt_slot (dev, i);input_mt_report_slot_state (dev,MT_TOOL_FINGER , active);if (active) { -unsigned int clamped_x, clamped_y; +int clamped_x, clamped_y;/* * The touchpad might report invalid data, so we clamp * the resulting values so that we do not confuse * userspace. */ - clamped_x =clamp (finger->x,0U , priv->x_max); - clamped_y =clamp (finger->y,0U , priv->y_max); + clamped_x =clamp (finger->x,0 , priv->x_max); + clamped_y =clamp (finger->y,0 , priv->y_max);input_report_abs (dev,ABS_MT_POSITION_X , clamped_x);input_report_abs (dev,ABS_MT_POSITION_Y , priv->y_max - clamped_y);
Signature · 5 lines
2.55.0.508.g3f0d502094-goog -- Dmitry
[email protected]
AI reviewThank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
New issues:
- [High] The patch changes internal coordinates to signed integers to prevent underflow jumps, but fails to write the clamped coordinates back to the internal state, creating a wind-up bug at the screen edges.
Pre-existing issues:
- [Critical] A signedness bug in focaltech_process_rel_packet allows an unused finger index to evaluate to -1 and bypass bounds checks, causing an out-of-bounds array write that silently corrupts the touchpad's physical bounds limits. --
--- Patch [1]: [PATCH] Input: focaltech - use signed coordinates to prevent underflow --- commit e1624dd1b0cb6c1665dcbaeda6430fe12e1a62aa
Author: Dmitry Torokhov <[email protected]>
Input: focaltech - use signed coordinates to prevent underflow
This commit changes the internal finger coordinates from unsigned to signed integers to prevent underflow when processing relative packets. It updates the variables in focaltech_finger_state and the clamping limits in focaltech_data to avoid erratic cursor jumps when moving past the touchpad boundaries.
Quoted 4 lines
drivers/input/mouse/focaltech.c +0
-0
[ ... ]
Quoted 5 lines
drivers/input/mouse/focaltech.c +0
-1@@ -126,14 +126,14 @@ static void focaltech_report_state(struct psmouse *psmouse)input_mt_slot (dev, i);input_mt_report_slot_state (dev,MT_TOOL_FINGER , active);if (active) { -unsigned int clamped_x, clamped_y;
drivers/input/mouse/focaltech.c static void focaltech_report_state(struct psmouse *psmouse) · line 136
Quoted 10 lines
drivers/input/mouse/focaltech.c +3
-2+int clamped_x, clamped_y;/* * The touchpad might report invalid data, so we clamp * the resulting values so that we do not confuse * userspace. */ - clamped_x =clamp (finger->x,0U , priv->x_max); - clamped_y =clamp (finger->y,0U , priv->y_max); + clamped_x =clamp (finger->x,0 , priv->x_max); + clamped_y =clamp (finger->y,0 , priv->y_max);
[Severity: High]
Does changing these coordinates to signed integers without updating the underlying state create a wind-up effect at the screen edges?
When swiping past the left or bottom edge, finger->x or finger->y will now be driven into negative values. Because the code clamps these values for reporting but does not write the bounded coordinates back to finger->x or finger->y, the out-of-bounds coordinates can accumulate wind-up.
Would the user then have to swipe a significant distance in the opposite direction just to offset this accumulated negative value before the cursor visibly moves away from the edge?
drivers/input/mouse/focaltech.c static void focaltech_report_state(struct psmouse *psmouse) · line 138
Quoted 2 lines
drivers/input/mouse/focaltech.c +0
-0input_report_abs (dev,ABS_MT_POSITION_X , clamped_x); input_report_abs(dev, ABS_MT_POSITION_Y,
[Severity: Critical]
This is a pre-existing issue, but does this out-of-bounds array write in focaltech_process_rel_packet() explain the original erratic jumping behavior the patch aims to fix?
When an odd number of fingers touches the pad, the hardware reports unused fingers with an index of 0. In focaltech_process_rel_packet(), the index is computed and checked like this:
drivers/input/mouse/focaltech.c:focaltech_process_rel_packet() {
...
finger2 = ((packet[3] >> 4) & 0x7) - 1;
if (finger2 < FOC_MAX_FINGERS) {
state->fingers[finger2].x += (s8)packet[4];
...
}Because finger2 is a signed int, an unused index of 0 evaluates to -1. The bounds check -1 < FOC_MAX_FINGERS incorrectly evaluates to true, bypassing the safety check. The exact same signedness flaw exists for finger1.
Due to the struct layout in focaltech_data, accessing state->fingers[-1] offsets backwards by exactly 12 bytes. This perfectly aliases state->fingers[-1].x to priv->x_max and state->fingers[-1].y to priv->y_max.
Could this out-of-bounds write be silently corrupting the touchpad physical bounds limits, continuously adding relative movement deltas to the device's maximum coordinate limits and causing the cursor to jump erratically?
Signature · 1 line
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1
Quoted 10 lines · Dmitry Torokhov
focaltech_finger_statestores finger coordinates x and y as unsigned int. When processing relative packets, negative deltas can cause unsigned integer underflow if the finger moves past the left or bottom boundary of the touchpad, wrapping the coordinates to values nearUINT_MAX.When clamping the coordinates in
focaltech_report_state(), these underflowed values are clamped againstpriv->x_max/priv->y_maxinstead of 0, causing the cursor to jump erratically to the opposite edge of the touchpad.
I agree this is theoretically possible. For what it's worth, I haven't actually experienced the cursor jumping to the opposite edge of the screen on my laptop. That might mean the relative packets are actually safe, e.g. generated internally from an absolute position.
Quoted 43 lines
Change the coordinate variables and limits to signed int so that negative values resulting from relative movements clamp correctly to 0.
Fixes 05be1d079ec0 ("Input: psmouse - support for the FocalTech PS/2 protocol extensions")Reported-by [email protected]Assisted-by Antigravity:gemini-3.6-flashSigned-off-by Dmitry Torokhov <[email protected]>---
drivers/input/mouse/focaltech.c| 12 ++++++------1 file changed, 6 insertions(+), 6 deletions(-)drivers/input/mouse/focaltech.c +4
-4@@ -78,8 +78,8 @@ struct focaltech_finger_state {* Absolute position (from the bottom left corner) of the * finger. */ -unsigned int x; -unsigned int y; +int x; +int y; };/* @@ -108,7 +108,7 @@ struct focaltech_hw_state { };struct focaltech_data { -unsigned int x_max, y_max; +int x_max, y_max;struct focaltech_hw_state state; };@@ -126,14 +126,14 @@ static void focaltech_report_state(struct psmouse *psmouse)input_mt_slot (dev, i);input_mt_report_slot_state (dev,MT_TOOL_FINGER , active);if (active) { -unsigned int clamped_x, clamped_y; +int clamped_x, clamped_y;/* * The touchpad might report invalid data, so we clamp
drivers/input/mouse/focaltech.c static void focaltech_report_state(struct psmouse *psmouse) · line 139
Quoted 10 lines
drivers/input/mouse/focaltech.c +2
-2* the resulting values so that we do not confuse * userspace. */ - clamped_x =clamp (finger->x,0U , priv->x_max); - clamped_y =clamp (finger->y,0U , priv->y_max); + clamped_x =clamp (finger->x,0 , priv->x_max); + clamped_y =clamp (finger->y,0 , priv->y_max);input_report_abs (dev,ABS_MT_POSITION_X , clamped_x);input_report_abs (dev,ABS_MT_POSITION_Y , priv->y_max - clamped_y);
If it's allowed and race-free to change finger->{x,y} in focaltech_report_state, then you might fix the Sashiko [High] issue on this patch by just doing:
finger->x = clamp(finger->x, 0, priv->x_max);finger->y = clamp(finger->y, 0, priv->y_max);
not having the clamped_{x,y} variables at all, and updating the two input_report_abs lines below to use finger->{x,y}
The Sashiko [Critical] pre-existing issue is the same one fixed by my previous patch - thanks for applying that.
Richard.
Dmitry Torokhov
maintainerQuoted 75 lines · Richard Davies
Quoted 10 lines · Dmitry Torokhov
focaltech_finger_statestores finger coordinates x and y as unsigned int. When processing relative packets, negative deltas can cause unsigned integer underflow if the finger moves past the left or bottom boundary of the touchpad, wrapping the coordinates to values nearUINT_MAX.When clamping the coordinates in
focaltech_report_state(), these underflowed values are clamped againstpriv->x_max/priv->y_maxinstead of 0, causing the cursor to jump erratically to the opposite edge of the touchpad.I agree this is theoretically possible. For what it's worth, I haven't actually experienced the cursor jumping to the opposite edge of the screen on my laptop. That might mean the relative packets are actually safe, e.g. generated internally from an absolute position.
Quoted 43 lines
Change the coordinate variables and limits to signed int so that negative values resulting from relative movements clamp correctly to 0.
Fixes 05be1d079ec0 ("Input: psmouse - support for the FocalTech PS/2 protocol extensions")Reported-by [email protected]Assisted-by Antigravity:gemini-3.6-flashSigned-off-by Dmitry Torokhov <[email protected]>---
drivers/input/mouse/focaltech.c| 12 ++++++------1 file changed, 6 insertions(+), 6 deletions(-)drivers/input/mouse/focaltech.c +4
-4@@ -78,8 +78,8 @@ struct focaltech_finger_state {* Absolute position (from the bottom left corner) of the * finger. */ -unsigned int x; -unsigned int y; +int x; +int y; };/* @@ -108,7 +108,7 @@ struct focaltech_hw_state { };struct focaltech_data { -unsigned int x_max, y_max; +int x_max, y_max;struct focaltech_hw_state state; };@@ -126,14 +126,14 @@ static void focaltech_report_state(struct psmouse *psmouse)input_mt_slot (dev, i);input_mt_report_slot_state (dev,MT_TOOL_FINGER , active);if (active) { -unsigned int clamped_x, clamped_y; +int clamped_x, clamped_y;/* * The touchpad might report invalid data, so we clamp drivers/input/mouse/focaltech.c static void focaltech_report_state(struct psmouse *psmouse) · line 139
Quoted 10 lines
drivers/input/mouse/focaltech.c +2
-2* the resulting values so that we do not confuse * userspace. */ - clamped_x =clamp (finger->x,0U , priv->x_max); - clamped_y =clamp (finger->y,0U , priv->y_max); + clamped_x =clamp (finger->x,0 , priv->x_max); + clamped_y =clamp (finger->y,0 , priv->y_max);input_report_abs (dev,ABS_MT_POSITION_X , clamped_x);input_report_abs (dev,ABS_MT_POSITION_Y , priv->y_max - clamped_y);If it's allowed and race-free to change finger->{x,y} in
focaltech_report_state, then you might fix the Sashiko [High] issue on this patch by just doing:
Quoted 5 lines
finger->x=clamp(finger->x, 0, priv->x_max);finger->y=clamp(finger->y, 0, priv->y_max);not having the clamped_{x,y} variables at all, and updating the two
input_report_abslines below to use finger->{x,y}
Good idea, thank you. I'll update and apply.
Thanks.
Signature · 1 line
Dmitry
Hi,
Quoted 11 lines · Dmitry Torokhov
focaltech_finger_statestores finger coordinates x and y as unsigned int. When processing relative packets, negative deltas can cause unsigned integer underflow if the finger moves past the left or bottom boundary of the touchpad, wrapping the coordinates to values nearUINT_MAX.When clamping the coordinates in
focaltech_report_state(), these underflowed values are clamped againstpriv->x_max/priv->y_maxinstead of 0, causing the cursor to jump erratically to the opposite edge of the touchpad.
Quoted 7 lines
Change the coordinate variables and limits to signed int so that negative values resulting from relative movements clamp correctly to 0.
Fixes 05be1d079ec0 ("Input: psmouse - support for the FocalTech PS/2 protocol extensions")Reported-by [email protected]Assisted-by Antigravity:gemini-3.6-flashSigned-off-by Dmitry Torokhov <[email protected]>
Thanks, patch looks good to me:
Regards,
Hans
Quoted 46 lines
--- drivers/input/mouse/focaltech.c | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-)drivers/input/mouse/focaltech.c +6
-6@@ -78,8 +78,8 @@ struct focaltech_finger_state {* Absolute position (from the bottom left corner) of the * finger. */ -unsigned int x; -unsigned int y; +int x; +int y; };/* @@ -108,7 +108,7 @@ struct focaltech_hw_state { };struct focaltech_data { -unsigned int x_max, y_max; +int x_max, y_max;struct focaltech_hw_state state; };@@ -126,14 +126,14 @@ static void focaltech_report_state(struct psmouse *psmouse)input_mt_slot (dev, i);input_mt_report_slot_state (dev,MT_TOOL_FINGER , active);if (active) { -unsigned int clamped_x, clamped_y; +int clamped_x, clamped_y;/* * The touchpad might report invalid data, so we clamp * the resulting values so that we do not confuse * userspace. */ - clamped_x =clamp (finger->x,0U , priv->x_max); - clamped_y =clamp (finger->y,0U , priv->y_max); + clamped_x =clamp (finger->x,0 , priv->x_max); + clamped_y =clamp (finger->y,0 , priv->y_max);input_report_abs (dev,ABS_MT_POSITION_X , clamped_x);input_report_abs (dev,ABS_MT_POSITION_Y , priv->y_max - clamped_y);