Input: focaltech - use signed coordinates to prevent underflow

[PATCH] Input: focaltech - use signed coordinates to prevent underflow

Posted by Dmitry Torokhov 2 months ago

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.

Fixes: 05be1d079ec0 ("Input: psmouse - support for the FocalTech PS/2 protocol extensions")
Reported-by: sashiko-bot@kernel.org
Assisted-by: Antigravity:gemini-3.6-flash
Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
---
 drivers/input/mouse/focaltech.c | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/drivers/input/mouse/focaltech.c b/drivers/input/mouse/focaltech.c
index d3ad4af5aa09..c6f6540e3e29 100644
--- a/drivers/input/mouse/focaltech.c
+++ b/drivers/input/mouse/focaltech.c
@@ -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);
-- 
2.55.0.508.g3f0d502094-goog


-- 
Dmitry

Re: [PATCH] Input: focaltech - use signed coordinates to prevent underflow

Posted by Hans de Goede 1 month, 3 weeks ago

Hi,

On 3-Aug-26 03:22, Dmitry Torokhov wrote:
> 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.
> 
> Fixes: 05be1d079ec0 ("Input: psmouse - support for the FocalTech PS/2 protocol extensions")
> Reported-by: sashiko-bot@kernel.org
> Assisted-by: Antigravity:gemini-3.6-flash
> Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>

Thanks, patch looks good to me:

Reviewed-by: Hans de Goede <johannes.goede@oss.qualcomm.com>

Regards,

Hans


> ---
>  drivers/input/mouse/focaltech.c | 12 ++++++------
>  1 file changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/drivers/input/mouse/focaltech.c b/drivers/input/mouse/focaltech.c
> index d3ad4af5aa09..c6f6540e3e29 100644
> --- a/drivers/input/mouse/focaltech.c
> +++ b/drivers/input/mouse/focaltech.c
> @@ -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);

Re: [PATCH] Input: focaltech - use signed coordinates to prevent underflow

Posted by Richard Davies 1 month, 4 weeks ago

Dmitry Torokhov wrote:
>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.

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.

>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: sashiko-bot@kernel.org
>Assisted-by: Antigravity:gemini-3.6-flash
>Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
>---
> drivers/input/mouse/focaltech.c | 12 ++++++------
> 1 file changed, 6 insertions(+), 6 deletions(-)
>
>diff --git a/drivers/input/mouse/focaltech.c b/drivers/input/mouse/focaltech.c
>index d3ad4af5aa09..c6f6540e3e29 100644
>--- a/drivers/input/mouse/focaltech.c
>+++ b/drivers/input/mouse/focaltech.c
>@@ -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);

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.

Re: [PATCH] Input: focaltech - use signed coordinates to prevent underflow

Posted by Dmitry Torokhov 1 month, 4 weeks ago

On Mon, Aug 03, 2026 at 01:29:32PM +0000, Richard Davies wrote:
> Dmitry Torokhov wrote:
> > 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.
> 
> 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.
> 
> > 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: sashiko-bot@kernel.org
> > Assisted-by: Antigravity:gemini-3.6-flash
> > Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
> > ---
> > drivers/input/mouse/focaltech.c | 12 ++++++------
> > 1 file changed, 6 insertions(+), 6 deletions(-)
> > 
> > diff --git a/drivers/input/mouse/focaltech.c b/drivers/input/mouse/focaltech.c
> > index d3ad4af5aa09..c6f6540e3e29 100644
> > --- a/drivers/input/mouse/focaltech.c
> > +++ b/drivers/input/mouse/focaltech.c
> > @@ -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);
> 
> 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}

Good idea, thank you. I'll update and apply.

Thanks.

-- 
Dmitry