[PATCH v3 0/2] Input: apple_z2 - bound device-reported packet length and finger count

[PATCH v3 0/2] Input: apple_z2 - bound device-reported packet length and finger count

[Bryam Vargas via B4 Relay]

The Apple Z2 touch controller is across an SPI bus and its reports are not
trusted: a malicious, malfunctioning or counterfeit controller (or an
interposer on the bus) can put arbitrary values in the report header.  Two
such device-supplied fields are used as sizes without being bounded against
the fixed 4000-byte receive buffer, giving the device a heap out-of-bounds
write and a heap out-of-bounds read.  This series bounds both.

Patch 1 bounds the 16-bit packet length before the second SPI transfer
(spi_read() into the 4000-byte rx_buf) -- the OOB write.

Patch 2 bounds the device-reported finger count in apple_z2_parse_touches()
-- the OOB read that is then forwarded to userspace as input events.

The two are paired because, as Dmitry noted on v2 of patch 2, the finger
count alone is not enough: msg_len is itself derived from the device length,
so patch 1 is what makes the patch 2 bound meaningful.  Patch 1 was posted
standalone first and carries Sasha Finkelstein's Reviewed-by; it is unchanged
here.

This v3 series supersedes the two separate postings:
  - packet length (standalone): https://lore.kernel.org/all/20260613-b4-disp-b1926f1a-v1-1-3a277b7c0cfa@proton.me/
  - finger count v1:            https://lore.kernel.org/all/20260613-b4-disp-f0148c89-v1-1-868a48b2a187@proton.me/
  - finger count v2:            https://lore.kernel.org/all/20260613-b4-disp-4ebcbd68-v2-1-0161acfbd688@proton.me/

Changes since the v2 finger-count patch (Dmitry Torokhov review,
https://lore.kernel.org/all/ai8U0QiwpBH8qNS3@google.com/):
- Patch 2: instead of clamping the finger count to what the packet holds
  (v2 used min_t and reported the fingers that fit), drop the whole packet
  when the device claims more fingers than it can hold, with a
  dev_warn_ratelimited(), per Dmitry's "I'd drop such packet" suggestion.
  A legitimate zero-finger ("all lifted") report (nfingers == 0) is still
  passed through to input_mt_sync_frame()/input_sync(), so lifting all
  fingers is not lost.  (Dropped Joshua Peisach's v2 Reviewed-by, as the
  fix shape changed materially from clamp to drop.)
- Patch 2 bound uses the multiplication form
  (msg_len < FINGERS_OFFSET + nfingers * sizeof(*fingers)) rather than a
  (msg_len - FINGERS_OFFSET) subtraction, so there is no unsigned underflow
  when msg_len < FINGERS_OFFSET (which would otherwise re-open the read).
- Patch 1 folded into the series unchanged (Reviewed-by Sasha Finkelstein).

Note on patch 1 (Sasha Finkelstein, on the standalone thread): rejecting an
over-long packet desyncs the SPI datastream and the device then needs a reset
before it operates again.  That is the intended outcome for a controller that
is already misbehaving or hostile -- a recoverable desync is preferable to a
~61 KiB heap overflow -- so the reject is kept.

Verification (in-kernel KASAN, CONFIG_KASAN=y kasan.fault=panic, plus userspace
ASan models of the exact parse arithmetic on x86_64 and i386):
  Patch 1 (packet length): reported length -> pkt_len 65540:
    Arm A (no patch): KASAN slab-out-of-bounds / ASan heap-buffer-overflow WRITE,
      both ABIs.
    Arm B (patch): rejected, clean.   Arm C (benign): clean.
  Patch 2 (finger count, drop shape): nfingers 255 in a short packet (msg_len 19):
    Arm A (no patch): BUG: KASAN: slab-out-of-bounds, Read of size 2, 1 byte to
      the right of the allocated 4000-byte region (cache kmalloc-4k) -> panic;
      ASan heap-buffer-overflow READ, both ABIs.
    Arm B (patch): packet dropped, zero reads, clean, not synced.
    Arm C (benign 3 fingers, msg_len 114): parsed in-bounds, clean, synced.
    Arm D (zero-finger all-lifted, nfingers 0, msg_len 19): clean, synced.
Reproducers and full logs available on request.

---
Bryam Vargas (2):
      Input: apple_z2 - bound the device-reported packet length
      Input: apple_z2 - bound the device-reported finger count

 drivers/input/touchscreen/apple_z2.c | 16 +++++++++++++++-
 1 file changed, 15 insertions(+), 1 deletion(-)
---
base-commit: 8e65320d91cdc3b241d4b94855c88459b91abf66
change-id: 20260615-b4-disp-05943ee1-9dc5cce670fe

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

[PATCH v3 1/2] Input: apple_z2 - bound the device-reported packet length

[Bryam Vargas via B4 Relay]

From: Bryam Vargas <hexlabsecurity@proton.me>

apple_z2_read_packet() takes a 16-bit length from the touch controller's
interrupt-data reply (rx_buf[1..2]) and, only rounded down to a multiple of
four, uses it as the size of the second SPI transfer into the fixed-size
rx_buf:

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

rx_buf is a fixed 4000-byte buffer, but pkt_len is fully controlled by the
device and is never checked against it, so a malicious, malfunctioning or
counterfeit controller (or an interposer on the SPI bus) that reports a
large length makes spi_read() write up to 65540 device-supplied bytes into
the 4000-byte buffer -- a controller-driven heap out-of-bounds write of up
to about 61 KiB.  The recently added reply-type check only validates
rx_buf[0], not the length.

Reject any packet whose length exceeds the receive buffer.

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>
Reviewed-by: Sasha Finkelstein <k@chaosmail.tech>
---
 drivers/input/touchscreen/apple_z2.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/drivers/input/touchscreen/apple_z2.c b/drivers/input/touchscreen/apple_z2.c
index 271ababf0ad5..ff9ff97be185 100644
--- a/drivers/input/touchscreen/apple_z2.c
+++ b/drivers/input/touchscreen/apple_z2.c
@@ -22,6 +22,7 @@
 #define APPLE_Z2_TOUCH_MOVED             4
 #define APPLE_Z2_CMD_READ_INTERRUPT_DATA 0xEB
 #define APPLE_Z2_REPLY_INTERRUPT_DATA    0xE1
+#define APPLE_Z2_MAX_PACKET              4000
 #define APPLE_Z2_HBPP_CMD_BLOB           0x3001
 #define APPLE_Z2_FW_MAGIC                0x5746325A
 #define LOAD_COMMAND_INIT_PAYLOAD        0
@@ -147,6 +148,8 @@ static int apple_z2_read_packet(struct apple_z2 *z2)
 		return 0;
 
 	pkt_len = (get_unaligned_le16(z2->rx_buf + 1) + 8) & 0xfffffffc;
+	if (pkt_len > APPLE_Z2_MAX_PACKET)
+		return -EMSGSIZE;
 
 	error = spi_read(z2->spidev, z2->rx_buf, pkt_len);
 	if (error)
@@ -363,7 +366,7 @@ static int apple_z2_probe(struct spi_device *spi)
 	if (!z2->tx_buf)
 		return -ENOMEM;
 	/* 4096 will end up being rounded up to 8192 due to devres header */
-	z2->rx_buf = devm_kzalloc(dev, 4000, GFP_KERNEL);
+	z2->rx_buf = devm_kzalloc(dev, APPLE_Z2_MAX_PACKET, GFP_KERNEL);
 	if (!z2->rx_buf)
 		return -ENOMEM;
 

-- 
2.43.0

[PATCH v3 2/2] 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 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 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 -- past the end of the receive 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.

Drop any packet that claims more fingers than it can hold, warning once
per rate-limit interval, rather than reporting a truncated set of
fingers from an inconsistent report.  A short zero-finger ("all lifted")
report is still passed through so the lifted state reaches userspace.
The 4000-byte receive buffer holds at most 132 finger records, so this
only ever drops counts that could not have been parsed in bounds.

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>
---
 drivers/input/touchscreen/apple_z2.c | 11 +++++++++++
 1 file changed, 11 insertions(+)

diff --git a/drivers/input/touchscreen/apple_z2.c b/drivers/input/touchscreen/apple_z2.c
index ff9ff97be185..4e2a6967436e 100644
--- a/drivers/input/touchscreen/apple_z2.c
+++ b/drivers/input/touchscreen/apple_z2.c
@@ -93,6 +93,17 @@ 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 or malfunctioning controller can report more fingers
+	 * than the packet actually carries; drop the packet rather than read
+	 * finger records past the end of the receive buffer.
+	 */
+	if (nfingers && msg_len < APPLE_Z2_FINGERS_OFFSET + nfingers * sizeof(*fingers)) {
+		dev_warn_ratelimited(&z2->spidev->dev,
+				     "discarding packet: %d fingers exceed packet length\n",
+				     nfingers);
+		return;
+	}
 	for (i = 0; i < nfingers; i++) {
 		slot = input_mt_get_slot_by_key(z2->input_dev, fingers[i].finger);
 		if (slot < 0) {

-- 
2.43.0

Re: [PATCH v3 1/2] Input: apple_z2 - bound the device-reported packet length

[sashiko-bot]

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

New issues:
- [High] Skipping `spi_read()` on oversized packets leaves the device's buffer undrained, causing an infinite interrupt storm.

Pre-existing issues:
- [High] Unprotected concurrent calls to `apple_z2_read_packet()` from both the probe/resume thread and IRQ context lead to SPI buffer corruption.
--

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

Input: apple_z2 - bound the device-reported packet length

apple_z2_read_packet() takes a 16-bit length from the touch controller's
interrupt-data reply and uses it as the size of the second SPI transfer into
the fixed-size rx_buf. Reject any packet whose length exceeds the receive
buffer to prevent out-of-bounds writes.

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>
Reviewed-by: Sasha Finkelstein <k@chaosmail.tech>

> diff --git a/drivers/input/touchscreen/apple_z2.c b/drivers/input/touchscreen/apple_z2.c
> index 271ababf0ad55..ff9ff97be185a 100644
> --- a/drivers/input/touchscreen/apple_z2.c
> +++ b/drivers/input/touchscreen/apple_z2.c

[ ... ]

> @@ -147,6 +148,8 @@ static int apple_z2_read_packet(struct apple_z2 *z2)
>  		return 0;

[Severity: High]
This is a pre-existing issue, but does this function need synchronization to
prevent SPI buffer corruption?

If apple_z2_upload_firmware() sets z2->booted = true and synchronously calls
apple_z2_read_packet(), and a concurrent interrupt triggers apple_z2_irq(),
they can both invoke apple_z2_read_packet() at the same time.

Won't this cause simultaneous unsynchronized modifications to the shared
z2->tx_buf, z2->rx_buf, and z2->index_parity variables?

>  
>  	pkt_len = (get_unaligned_le16(z2->rx_buf + 1) + 8) & 0xfffffffc;
> +	if (pkt_len > APPLE_Z2_MAX_PACKET)
> +		return -EMSGSIZE;

[Severity: High]
Will returning early here cause an interrupt storm?

By skipping spi_read(), the device's internal buffer is not drained and the
hardware interrupt line will likely remain asserted.

Since apple_z2_irq() ignores the error returned by apple_z2_read_packet() and
unilaterally returns IRQ_HANDLED:

drivers/input/touchscreen/apple_z2.c:apple_z2_irq() {
    ...
    else
        apple_z2_read_packet(z2);
        
    return IRQ_HANDLED;
}

Could this immediately unmask the interrupt and endlessly loop through this
same failure path?

>  
>  	error = spi_read(z2->spidev, z2->rx_buf, pkt_len);
>  	if (error)

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260615-b4-disp-05943ee1-v3-0-bbfe1b651613@proton.me?part=1

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

[sashiko-bot]

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

Pre-existing issues:
- [High] Concurrent execution of apple_z2_read_packet() from the probe thread and threaded IRQ handler causes buffer corruption.
- [High] Missing length check for `fw->size` before accessing the firmware header in `apple_z2_upload_firmware()`.
- [Medium] The timeout error path in `apple_z2_boot()` returns early without disabling the IRQ or asserting the reset GPIO.
- [Low] Missing input_set_drvdata() before device registration.
--

commit 647d8f6a84132a3fd0648807a009baeb1d458863
Author: Bryam Vargas <hexlabsecurity@proton.me>

Input: apple_z2 - bound the device-reported finger count

This commit adds bounds checking to apple_z2_parse_touches() to ensure the
reported finger count does not exceed the receive buffer. It drops packets
that claim more fingers than can fit, preventing an out-of-bounds read that
could leak adjacent kernel memory.

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 ff9ff97be185a..4e2a6967436e8 100644
> --- a/drivers/input/touchscreen/apple_z2.c
> +++ b/drivers/input/touchscreen/apple_z2.c

[ ... ]

[Severity: High]
This isn't a bug introduced by this patch, but does concurrent execution of
apple_z2_read_packet() from the probe thread and threaded IRQ handler cause
buffer corruption?

In apple_z2_upload_firmware():

    z2->booted = true;
    apple_z2_read_packet(z2);

If the device asserts the IRQ line here, the unmasked IRQ handler
apple_z2_irq() wakes up, observes booted is true, and also calls
apple_z2_read_packet():

    else
            apple_z2_read_packet(z2);

Could this result in a data race where SPI DMA transfers corrupt the receive
buffer while apple_z2_parse_touches() is executing?

[Severity: High]
This is a pre-existing issue, but does apple_z2_upload_firmware() miss a
length check for fw->size before accessing the firmware header?

    fw_hdr = (const struct apple_z2_fw_hdr *)fw->data;
    if (le32_to_cpu(fw_hdr->magic) != APPLE_Z2_FW_MAGIC ||
        le32_to_cpu(fw_hdr->version) != 1) {

If the firmware file is truncated or empty, will this cause an out-of-bounds
read when the header fields are accessed?

[Severity: Medium]
This isn't a bug introduced by this patch, but I noticed the timeout error
path in apple_z2_boot() returns early without cleaning up:

    if (!wait_for_completion_timeout(&z2->boot_irq, msecs_to_jiffies(20)))
            return -ETIMEDOUT;

Does this leave the hardware out of reset and create an unbalanced IRQ enable
state? The subsequent error path for apple_z2_upload_firmware() correctly
performs these cleanup steps by disabling the IRQ and asserting the reset
GPIO.

[Severity: Low]
This is a pre-existing issue, but the input device is registered in
apple_z2_probe() without prior initialization of driver data:

    error = input_register_device(z2->input_dev);

Is it intentional to omit input_set_drvdata() before registration here? The
subsystem guidelines state that all private driver data must be initialized
before registration to avoid visibility issues.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260615-b4-disp-05943ee1-v3-0-bbfe1b651613@proton.me?part=2