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

[sashiko-bot@kernel.org]

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