Re: [PATCH] Input: apple_z2 - bound the device-reported finger count

[sashiko-bot@kernel.org]

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

New issues:
- [High] The patch incorrectly discards valid zero-finger reports, resulting in stuck touches.

Pre-existing issues:
- [Critical] Unbounded device-reported packet length leads to a massive heap buffer overflow during `spi_read()` and completely bypasses the patch's intended fix for out-of-bounds reads.
--

commit 3a1e80d03b931df0a0db172ea0ca1a4ea3349e47
Author: Bryam Vargas <hexlabsecurity@proton.me>

Input: apple_z2 - bound the device-reported finger count

This commit attempts to fix a heap out-of-bounds read by bounding the number
of reported fingers against the message length.

Link: https://lore.kernel.org/all/20260613215358.329921F000E9@smtp.kernel.org/

> diff --git a/drivers/input/touchscreen/apple_z2.c b/drivers/input/touchscreen/apple_z2.c
> index 271ababf0ad55..61f353553e7c5 100644
> --- a/drivers/input/touchscreen/apple_z2.c
> +++ b/drivers/input/touchscreen/apple_z2.c
> @@ -88,10 +88,13 @@ static void apple_z2_parse_touches(struct apple_z2 *z2,
>  	int slot_valid;
>  	struct apple_z2_finger *fingers;
>  
> -	if (msg_len <= APPLE_Z2_NUM_FINGERS_OFFSET)
> +	if (msg_len <= APPLE_Z2_FINGERS_OFFSET)
>  		return;

[Severity: High]
Will this new check discard valid zero-finger reports?

With SPI length rounding, an empty report indicating all fingers have been
lifted usually lacks the finger array, meaning its payload size can fall
between 17 and 24 bytes.

If a 0-finger packet is dropped here, the input subsystem won't be notified
via input_mt_sync_frame() and input_sync(). Could this result in touches
remaining stuck on the screen?

>  	nfingers = msg[APPLE_Z2_NUM_FINGERS_OFFSET];
>  	fingers = (struct apple_z2_finger *)(msg + APPLE_Z2_FINGERS_OFFSET);
> +	/* a malicious controller can claim more fingers than the packet holds */
> +	nfingers = min_t(int, nfingers,
> +			 (msg_len - APPLE_Z2_FINGERS_OFFSET) / sizeof(*fingers));

[Severity: Critical]
This isn't a bug introduced by this patch, but does deriving msg_len from
the untrusted packet length bypass this new boundary check entirely?

In apple_z2_read_packet(), pkt_len is derived directly from an
unauthenticated SPI read:

apple_z2_read_packet() {
        ...
        pkt_len = (get_unaligned_le16(z2->rx_buf + 1) + 8) & 0xfffffffc;
        error = spi_read(z2->spidev, z2->rx_buf, pkt_len);
        ...
}

Since z2->rx_buf is a fixed 4000-byte allocation, a malicious controller
could send a large length field (up to 65540) to cause a heap buffer
overflow during spi_read().

Furthermore, because msg_len is derived directly from this untrusted
pkt_len, an artificially large pkt_len bypasses the new clamping logic
above. This allows nfingers to still evaluate to 255.

Does this render the out-of-bounds read fix ineffective against malicious
controllers?

>  	for (i = 0; i < nfingers; i++) {
>  		slot = input_mt_get_slot_by_key(z2->input_dev, fingers[i].finger);
>  		if (slot < 0) {

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260613-b4-disp-f0148c89-v1-1-868a48b2a187@proton.me?part=1