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

[sashiko-bot@kernel.org]

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