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

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

[Bryam Vargas via B4 Relay]

From: Bryam Vargas <hexlabsecurity@proton.me>

apple_z2_parse_touches() takes the finger count from the touch
controller's report and loops over that many fixed-size finger records
without ever checking the count against the length of the report:

	nfingers = msg[APPLE_Z2_NUM_FINGERS_OFFSET];
	fingers = (struct apple_z2_finger *)(msg + APPLE_Z2_FINGERS_OFFSET);
	for (i = 0; i < nfingers; i++)
		/* read fingers[i] ... */

msg points into the fixed 4000-byte z2->rx_buf and nfingers is a single
device-supplied byte, so it can be as large as 255.  A malicious,
malfunctioning or counterfeit controller (or an interposer on the SPI
bus) can report a large finger count in a short packet, making the loop
read up to 255 * sizeof(struct apple_z2_finger) bytes starting 24 bytes
into msg -- far past the 4000-byte buffer.  This is a controller-driven
heap out-of-bounds read, and the finger fields that are read (position,
pressure, touch and tool dimensions) are forwarded to userspace as input
events, leaking adjacent kernel memory.

Bound the device-reported count to the number of finger records the
report actually carries.

Reported-by: sashiko-bot@kernel.org
Closes: https://lore.kernel.org/all/20260613215358.329921F000E9@smtp.kernel.org/
Fixes: 471a92f8a21a ("Input: apple_z2 - add a driver for Apple Z2 touchscreens")
Cc: stable@vger.kernel.org
Signed-off-by: Bryam Vargas <hexlabsecurity@proton.me>
---
Changes since v1 [1]:
- Keep the early-return at NUM_FINGERS_OFFSET instead of moving it to
  FINGERS_OFFSET, so a short zero-finger ("all lifted") report still
  reaches input_mt_sync_frame()/input_sync() and does not leave touches
  stuck on the screen (caught by the sashiko-bot review of v1 [2]).  A
  packet too short to hold even one finger record clamps nfingers to 0
  instead of being dropped.

[1] https://lore.kernel.org/all/20260613-b4-disp-f0148c89-v1-1-868a48b2a187@proton.me/
[2] https://lore.kernel.org/all/20260614000725.6B8D11F000E9@smtp.kernel.org/

Reachable on every touch interrupt once the controller is booted
(apple_z2_irq -> apple_z2_read_packet -> apple_z2_parse_touches).

nfingers is bounded here by the message length; the message length is in
turn bounded by the companion "Input: apple_z2 - bound the device-reported
packet length" change (in flight), which caps the device-reported pkt_len
to the 4000-byte receive buffer.  The two together close the device-driven
out-of-bounds accesses in apple_z2_parse_touches() / apple_z2_read_packet().

Verified with a faithful in-kernel KASAN litmus (the verbatim 4000-byte
buffer, the struct apple_z2_finger layout and the parse loop),
CONFIG_KASAN=y on x86_64:

  Arm A, nfingers = 255 in a short packet (msg_len 19):
    BUG: KASAN: slab-out-of-bounds in apple_z2_parse_touches
    Read of size 2 ... 1 bytes to the right of allocated 4000-byte region
    ... cache kmalloc-4k of size 4096
  Arm B, with this patch: a zero-finger report (msg_len 19) reaches the
    sync; a 255-finger claim is clamped to what the packet holds; clean.
  Arm C, benign device (3 fingers): clean

  AddressSanitizer (x86_64 and i386): heap-buffer-overflow READ, both ABIs.

Reproducer and full logs available on request.
---
 drivers/input/touchscreen/apple_z2.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/drivers/input/touchscreen/apple_z2.c b/drivers/input/touchscreen/apple_z2.c
index 271ababf0ad5..39ade83ef0de 100644
--- a/drivers/input/touchscreen/apple_z2.c
+++ b/drivers/input/touchscreen/apple_z2.c
@@ -92,6 +92,12 @@ static void apple_z2_parse_touches(struct apple_z2 *z2,
 		return;
 	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 */
+	if (msg_len < APPLE_Z2_FINGERS_OFFSET)
+		nfingers = 0;
+	else
+		nfingers = min_t(int, nfingers,
+				 (msg_len - APPLE_Z2_FINGERS_OFFSET) / sizeof(*fingers));
 	for (i = 0; i < nfingers; i++) {
 		slot = input_mt_get_slot_by_key(z2->input_dev, fingers[i].finger);
 		if (slot < 0) {

---
base-commit: 8e65320d91cdc3b241d4b94855c88459b91abf66
change-id: 20260613-b4-disp-4ebcbd68-ed8a28672ccc

Best regards,
-- 
Bryam Vargas <hexlabsecurity@proton.me>

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

[sashiko-bot]

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

Pre-existing issues:
- [Critical] Unvalidated device-reported packet length leads to a severe heap buffer overflow in `spi_read`. The patch fails to address this root cause.
- [High] Using the `__free(kfree)` attribute on a variable that can hold an `ERR_PTR` causes a kernel panic during error handling.
- [High] Concurrent execution of `apple_z2_read_packet` causes data races on driver state, SPI buffers, and lockless input-mt helper states.
--

commit 9a7b4f864d6502dba7c8e2bed5aa7ed3358d9e4a
Author: Bryam Vargas <hexlabsecurity@proton.me>

Input: apple_z2 - bound the device-reported finger count

The commit adds bounds checking for the device-reported finger count against
the SPI packet length to prevent an out-of-bounds read in
apple_z2_parse_touches().

Closes: 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..39ade83ef0de7 100644
> --- a/drivers/input/touchscreen/apple_z2.c
> +++ b/drivers/input/touchscreen/apple_z2.c
> @@ -92,6 +92,12 @@ static void apple_z2_parse_touches(struct apple_z2 *z2,
>  		return;
>  	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 */
> +	if (msg_len < APPLE_Z2_FINGERS_OFFSET)
> +		nfingers = 0;
> +	else
> +		nfingers = min_t(int, nfingers,
> +				 (msg_len - APPLE_Z2_FINGERS_OFFSET) / sizeof(*fingers));
>  	for (i = 0; i < nfingers; i++) {
>  		slot = input_mt_get_slot_by_key(z2->input_dev, fingers[i].finger);
>  		if (slot < 0) {

[Severity: High]
This is a pre-existing issue, but does this code overflow the heap allocation
for rx_buf?

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

Because rx_buf is allocated with a fixed size of 4000 bytes, couldn't a
malfunctioning device supply a large packet length and cause spi_read to
write past the end of the allocation?

[Severity: High]
This isn't a bug introduced by this patch, but will this cause a kernel panic
on error?

In apple_z2_upload_firmware():

    const u8 *data __free(kfree) =
        apple_z2_build_cal_blob(z2, address, &size);
    if (IS_ERR(data))
        return PTR_ERR(data);

Since kfree does not check for IS_ERR, doesn't returning here cause the
compiler-inserted kfree(data) to attempt freeing an invalid virtual address?

[Severity: High]
This is a pre-existing issue, but can this sequence lead to data races?

If a hardware interrupt arrives immediately after setting z2->booted to true,
could the threaded IRQ handler execute apple_z2_read_packet concurrently?

CPU1 in apple_z2_upload_firmware():
    z2->booted = true;
    apple_z2_read_packet(z2);

CPU2 in apple_z2_irq():
    ...
    else
        apple_z2_read_packet(z2);

Since apple_z2_read_packet modifies shared state without a lock:

apple_z2_read_packet():
    len_cmd->counter = z2->index_parity + 1;
    ...
    z2->index_parity = !z2->index_parity;

wouldn't this concurrent execution corrupt z2->tx_buf, z2->rx_buf, and
z2->index_parity?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260613-b4-disp-4ebcbd68-v2-1-0161acfbd688@proton.me?part=1