Re: [PATCH v2 3/5] Input: applespi - register touchpad synchronously in probe

[sashiko-bot@kernel.org]

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

New issues:
- [Critical] A race condition in the SPI completion handler causes a Use-After-Free of `applespi` when the probe thread fails.
- [Critical] Ignoring the timeout in `applespi_drain_writes` and `applespi_drain_reads` allows the driver to unbind while SPI DMA transfers are active.
- [High] Reading the `tp_dim` debugfs file triggers a NULL pointer dereference if the touchpad info packet times out during probe.
- [Medium] Data Race when reading `rcvd_tp_info` without `cmd_msg_lock` in probe.

Pre-existing issues:
- [High] Failure to call `acpi_finish_gpe()` if `applespi_async()` fails in the GPE interrupt handler.
--

--- Patch [3]: [PATCH v2 3/5] Input: applespi - register touchpad synchronously in probe ---
commit d8377d333c1aa1ce257aedd6f29080dfb8f5320b
Author: Shih-Yuan Lee <fourdollars@debian.org>

Input: applespi - register touchpad synchronously in probe

This commit removes the asynchronous worker for touchpad registration.
It replaces it with synchronous registration during driver probe, waiting up
to 3 seconds for the device info packet and falling back to keyboard-only
mode on timeout.

> diff --git a/drivers/input/keyboard/applespi.c b/drivers/input/keyboard/applespi.c
> --- a/drivers/input/keyboard/applespi.c
> +++ b/drivers/input/keyboard/applespi.c

[ ... ]

> @@ -1821,6 +1814,20 @@ static int applespi_probe(struct spi_device *spi)
>  	/* trigger touchpad setup */
>  	applespi_init(applespi, false);
>  
> +	/* set up the touchpad as a separate input device if info is received */
> +	sts = wait_event_timeout(applespi->wait_queue,
> +				 READ_ONCE(applespi->have_tp_info),
> +				 msecs_to_jiffies(3000));
> +	if (!sts) {
> +		dev_warn(&applespi->spi->dev,
> +			 "Timed out waiting for touchpad info, continuing keyboard-only\n");

[Severity: High]
If this timeout occurs, applespi->touchpad_input_dev remains null. 
Will this cause a kernel panic later?

When a user reads the debugfs file, applespi_tp_dim_open() unconditionally
dereferences it:

    snprintf(applespi->tp_dim_val, sizeof(applespi->tp_dim_val),
             "0x%.4x %dx%d+%u+%u\n",
             applespi->touchpad_input_dev->id.product,
             ...

> +	} else {
> +		sts = applespi_register_touchpad_device(applespi,
> +							&applespi->rcvd_tp_info);

[Severity: Medium]
Is there a data race when reading rcvd_tp_info here?

The commit message mentions protecting rcvd_tp_info under cmd_msg_lock,
but applespi_register_touchpad_device() extracts the model info from it
without holding the lock. If the device sends another unexpected touchpad
info packet, could the SPI callback overwrite rcvd_tp_info concurrently?

> +		if (sts)
> +			goto cancel_spi;
> +	}
> +

[ ... ]

> @@ -1853,6 +1860,19 @@ static int applespi_probe(struct spi_device *spi)
>  			    &applespi_tp_dim_fops);
>  
>  	return 0;
> +
> +cancel_spi:
> +	acpi_disable_gpe(NULL, applespi->gpe);
> +	acpi_remove_gpe_handler(NULL, applespi->gpe, applespi_notify);

[Severity: High]
This is a pre-existing issue, but does applespi_notify() have a bug in its
error handling path that could cause a permanent GPE stall?

If applespi_async() fails in applespi_notify() (e.g. transient out of
memory), it returns ACPI_INTERRUPT_HANDLED but misses queuing the async
operation:

    sts = applespi_async(applespi, &applespi->rd_m,
                         applespi_async_read_complete);
    if (sts)
        dev_warn(&applespi->spi->dev,
                 "Error queueing async read to device: %d\n", sts);

Because the async task was not queued, applespi_async_read_complete() is
never called. This means acpi_finish_gpe() is never invoked, permanently
leaving the level-triggered GPE unfinished and breaking all input.

> +
> +	spin_lock_irqsave(&applespi->cmd_msg_lock, flags);
> +	applespi->cancel_spi = true;
> +	wait_event_lock_irq(applespi->wait_queue,
> +			    !applespi_async_outstanding(applespi),
> +			    applespi->cmd_msg_lock);
> +	spin_unlock_irqrestore(&applespi->cmd_msg_lock, flags);
> +
> +	return sts;

[Severity: Critical]
Does this error path introduce a use-after-free for applespi?

When wait_event_lock_irq completes, this probe thread returns an error
and devres frees the applespi structure. However, in
applespi_async_complete():

    if (applespi->cancel_spi && !applespi_async_outstanding(applespi))
        wake_up_all(&applespi->wait_queue);

    spin_unlock_irqrestore(&applespi->cmd_msg_lock, flags);

    if (complete)
        complete(applespi);

The completion thread wakes the wait queue and drops the lock. This allows
the probe thread to resume, return, and trigger the devres free operation
before the completion thread executes complete(applespi), leading to a
use-after-free.

>  }
>  
>  static void applespi_drain_writes(struct applespi_data *applespi)

[Severity: Critical]
I noticed a regression here. Does applespi_drain_writes() allow the driver
to unbind while SPI DMA transfers are still active?

It uses a timeout when waiting for writes to finish:

    wait_event_lock_irq_timeout(applespi->wait_queue,
                                !applespi->write_active,
                                applespi->cmd_msg_lock,
                                msecs_to_jiffies(3000));
                                
    spin_unlock_irqrestore(&applespi->cmd_msg_lock, flags);

Since the return value of wait_event_lock_irq_timeout() is ignored, if the
SPI hardware is delayed and the timeout expires, this function silently
returns as if the drain succeeded. When applespi_remove() completes,
devres frees the memory. Once the hardware finally processes the transfer,
it will DMA to/from freed memory and execute the completion callback.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260720101435.13612-1-fourdollars@debian.org?part=3