[PATCH] Input: appletouch - fix offset caused by smoothing

[PATCH] Input: appletouch - fix offset caused by smoothing

[Icenowy Zheng]

The smoothing code introduces 4 sensors of margin on each side of the
input, which is not compensated and leads to a offset of 4 * factor added
to the calculated coordinate values.

However, the maximum value reported as ABS axis parameters are
calculated with the sensor count multiplied by the factor, which leads
to the coordinate values going beyond the maximum value and get ignored
by libinput.

Fix this by subtracting the index by 4 when accumlating the smoothed
values. This makes the reported coordinates in-range again, and libinput
stops to drop touch events in the right and down side of the trackpad.

Fixes: 739204bc9577 ("Input: appletouch - implement sensor data smoothing")
Cc: stable@vger.kernel.org
Signed-off-by: Icenowy Zheng <zhengxingda@iscas.ac.cn>
---
 drivers/input/mouse/appletouch.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/input/mouse/appletouch.c b/drivers/input/mouse/appletouch.c
index eebeb57515e1c..174762d59f87b 100644
--- a/drivers/input/mouse/appletouch.c
+++ b/drivers/input/mouse/appletouch.c
@@ -415,7 +415,7 @@ static int atp_calculate_abs(struct atp *dev, int offset, int nb_sensors,
 		 * by scale. Mostly noise.
 		 */
 		if ((dev->smooth[i] >> ATP_SCALE) > 0) {
-			pcum += dev->smooth[i] * i;
+			pcum += dev->smooth[i] * (i - 4);
 			psum += dev->smooth[i];
 		}
 	}
-- 
2.52.0

Re: [PATCH] Input: appletouch - fix offset caused by smoothing

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:

New issues:
- [High] Removing the 4-sensor offset causes valid edge touches to evaluate to 0, which the caller treats as dropped events.

Pre-existing issues:
- [High] Race condition between `atp_close()` and `atp_reinit()` causes use-after-free.
- [High] A 32-bit signed integer overflow in absolute coordinate calculation causes negative cursor jumps.
--

commit 39d19e6e224959faeb0b8190e5d07e558b5470fe
Author: Icenowy Zheng <zhengxingda@iscas.ac.cn>

Input: appletouch - fix offset caused by smoothing

This commit subtracts 4 from the index when accumulating smoothed values
to compensate for the 4-sensor margin introduced by smoothing. This aims
to keep the calculated coordinates within the maximum reported range.

> diff --git a/drivers/input/mouse/appletouch.c b/drivers/input/mouse/appletouch.c
> index eebeb57515e1c..174762d59f87b 100644
> --- a/drivers/input/mouse/appletouch.c
> +++ b/drivers/input/mouse/appletouch.c
> @@ -415,7 +415,7 @@ static int atp_calculate_abs(struct atp *dev, int offset, int nb_sensors,
>  		 * by scale. Mostly noise.
>  		 */
>  		if ((dev->smooth[i] >> ATP_SCALE) > 0) {
> -			pcum += dev->smooth[i] * i;
> +			pcum += dev->smooth[i] * (i - 4);

[Severity: High]
Does this change cause valid touches on the first sensor to be dropped?

If a touch is centered exactly on the first sensor (index 4 in the smooth
array), the smoothed values will be perfectly symmetric around index 4. The
new (i - 4) multiplier will cause the left and right sides to cancel out
exactly, making pcum equal 0.

The callers use 0 as a sentinel for no touch:

drivers/input/mouse/appletouch.c:atp_complete_geyser_3_4() {
    ...
    if (x && y && fingers == dev->fingers_old) {
    ...
}

Will this cause valid edge touches to be mistakenly treated as finger
lift-offs and create dead zones on the trackpad edges?

>  			psum += dev->smooth[i];
>  		}
>  	}
> 
>  	if (psum > 0) {
>  		*z = psum >> ATP_SCALE;        /* Scale down pressure output. */
>  		return pcum * fact / psum;

[Severity: High]
This is a pre-existing issue, but does this expression overflow a 32-bit
signed integer?

In atp_calculate_abs(), the accumulator pcum can reach large values for
multi-finger touches at the far edges of the trackpad. If pcum reaches
around 100,000,000, multiplying it by fact (which is 64 for xfact) yields
over 6 billion.

Does this silently wrap to a massive negative number, causing the cursor to
violently jerk to the top or left edge when reported to userspace?

[ ... ]

[Severity: High]
This is a pre-existing issue, but can a race condition between atp_close()
and atp_reinit() cause a use-after-free?

In atp_close(), usb_kill_urb() is called before cancel_work_sync():

drivers/input/mouse/appletouch.c:atp_close() {
    ...
    usb_kill_urb(dev->urb);
    cancel_work_sync(&dev->work);
    ...
}

If the worker atp_reinit() is executing concurrently, can it submit the URB
after usb_kill_urb() has finished? The cancel_work_sync() simply waits for
the worker to finish and returns, leaving the URB actively running after
the device is closed.

Furthermore, atp_disconnect() does not call cancel_work_sync() if the
device was already closed:

drivers/input/mouse/appletouch.c:atp_disconnect() {
    ...
    usb_kill_urb(dev->urb);
    input_unregister_device(dev->input);
    usb_free_coherent(...);
    usb_free_urb(dev->urb);
    kfree(dev);
}

Since a running URB can schedule the work again, could atp_disconnect()
free the dev structure while the worker or URB completion handler is still
running?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260607160311.2192061-1-zhengxingda@iscas.ac.cn?part=1