Re: [PATCH] Input: apple_z2 - bound the device-reported packet length

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Missing bounds check on the number of fingers (`nfingers`) reported by the device in `apple_z2_parse_touches` leads to a kernel heap out-of-bounds read and potential information leak.
- [High] Concurrent execution of `apple_z2_read_packet()` from the probe/resume thread and the IRQ handler causes a data race and shared SPI buffer corruption.
- [Medium] If `apple_z2_boot()` times out waiting for the boot IRQ, it returns directly without disabling the IRQ or reverting the hardware reset state, leading to state inconsistencies.
- [Low] The input device is registered and exposed to userspace before the hardware is initialized and firmware is uploaded, violating input subsystem readiness rules.
--

commit 0ef574cc6b6df1efb30f7e523d3cf505e551ae3c
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. This bounds the device-reported packet length to reject
any packet whose length exceeds the receive buffer.

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

[Severity: High]
This isn't a bug introduced by this patch, but does apple_z2_parse_touches()
lack a bounds check on the number of fingers?

It appears that apple_z2_parse_touches() reads the unvalidated nfingers count
from msg[APPLE_Z2_NUM_FINGERS_OFFSET] and blindly iterates over it:

drivers/input/touchscreen/apple_z2.c:apple_z2_parse_touches() {
    ...
	nfingers = msg[APPLE_Z2_NUM_FINGERS_OFFSET];
	fingers = (struct apple_z2_finger *)(msg + APPLE_Z2_FINGERS_OFFSET);
	for (i = 0; i < nfingers; i++) {
    ...
}

Could a spoofed device set nfingers to 255, causing the loop to read
out-of-bounds past the rx_buf allocation, and potentially leading to an
information leak when those bytes are parsed as touch coordinates?

[Severity: High]
This is a pre-existing issue, but is there a data race when
apple_z2_read_packet() is executed concurrently?

In apple_z2_boot(), the device interrupt is unmasked via enable_irq() before
firmware is uploaded. Then in apple_z2_upload_firmware(), the driver sets
z2->booted = true and manually calls apple_z2_read_packet().

If the device asserts the interrupt concurrently, the threaded IRQ handler
apple_z2_irq() will wake up, observe z2->booted == true, and also invoke
apple_z2_read_packet().

Since there is no lock protecting apple_z2_read_packet(), could this
concurrently overwrite z2->tx_buf, mutate z2->index_parity, and corrupt the
shared SPI buffers?

[ ... ]

> @@ -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;

[Severity: Medium]
This is a pre-existing issue, but does apple_z2_boot() miss cleanup when
wait_for_completion_timeout() fails?

In apple_z2_boot(), the IRQ enable depth is incremented and the device is
taken out of reset. If it times out waiting for the boot_irq completion, it
directly returns -ETIMEDOUT:

drivers/input/touchscreen/apple_z2.c:apple_z2_boot() {
    ...
	if (!wait_for_completion_timeout(&z2->boot_irq, msecs_to_jiffies(20)))
		return -ETIMEDOUT;
    ...
}

If this happens during apple_z2_resume(), devm teardown does not occur. Does
this leave the IRQ permanently enabled and the reset GPIO active, leading to
state inconsistency?

[Severity: Low]
This is a pre-existing issue, but is the input device registered before the
hardware is fully initialized?

In apple_z2_probe(), input_register_device() is called before apple_z2_boot()
has completed:

drivers/input/touchscreen/apple_z2.c:apple_z2_probe() {
    ...
	error = input_register_device(z2->input_dev);
	if (error)
		return dev_err_probe(dev, error, "unable to register input device\n");

	/* Wait for device reset to finish */
	usleep_range(5000, 10000);
	error = apple_z2_boot(z2);
    ...
}

If apple_z2_boot() fails, the momentarily registered device is removed. Does
this expose an uninitialized device to userspace prematurely?

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