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

[sashiko-bot@kernel.org]

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