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

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.

> diff --git a/drivers/input/mouse/focaltech.c b/drivers/input/mouse/focaltech.c
> index 43f9939b7c63c..4c909c4ca76c9 100644
> --- a/drivers/input/mouse/focaltech.c
> +++ b/drivers/input/mouse/focaltech.c

[ ... ]

> @@ -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);

[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?

>  			input_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?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/[email protected]?part=1