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

[sashiko-bot@kernel.org]

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