Re: [PATCH v2 5/5] Input: applespi - fix use-after-free in applespi_remove()

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] applespi_async_complete() triggers a use-after-free by signaling completion before actually executing the callback.
- [High] Ignored timeout in `wait_event_lock_irq_timeout` leads to a Use-After-Free on outstanding SPI transfers.
- [High] Setting `cancel_spi = true` before disabling the level-triggered GPE causes an interrupt storm.
--

--- Patch [5]: [PATCH v2 5/5] Input: applespi - fix use-after-free in applespi_remove() ---
commit 8828f1f25e357bbbc7a4945659f2ddf6d10cda5c
Author: Shih-Yuan Lee <fourdollars@debian.org>

Input: applespi - fix use-after-free in applespi_remove()

This commit replaces the separate read and write drain helpers with a single
barrier using the existing cancel_spi and wait_event_lock_irq mechanism. It
waits for outstanding asynchronous operations to complete before disabling
the GPE and tearing down the device.

> diff --git a/drivers/input/keyboard/applespi.c b/drivers/input/keyboard/applespi.c
> index 95a8f790eaffc..088337f060b24 100644
> --- a/drivers/input/keyboard/applespi.c
> +++ b/drivers/input/keyboard/applespi.c
> @@ -1910,15 +1910,22 @@ static void applespi_drain_reads(struct applespi_data *applespi)
>  static void applespi_remove(struct spi_device *spi)
>  {
>  	struct applespi_data *applespi = spi_get_drvdata(spi);
> +	unsigned long flags;
>  
> -	applespi_drain_writes(applespi);
> +	/* Prevent any new SPI transfers and wait for outstanding ones */
> +	spin_lock_irqsave(&applespi->cmd_msg_lock, flags);
> +	applespi->cancel_spi = true;

[Severity: High]
If we set cancel_spi to true while the level-triggered GPE is still enabled,
could this cause an interrupt storm?

When the GPE fires, applespi_notify() attempts to queue a read via
applespi_async(), which immediately rejects it with -ESHUTDOWN. Because the
read is rejected, the trackpad hardware never deasserts the interrupt line.

Would it be safer to disable the GPE before setting this flag?

> +	wait_event_lock_irq_timeout(applespi->wait_queue,
> +				    !applespi_async_outstanding(applespi),
> +				    applespi->cmd_msg_lock,
> +				    msecs_to_jiffies(3000));

[Severity: High]
If the wait times out (returning 0), the driver continues tearing down and
returns, allowing devm to free the memory. If the delayed SPI transfer
completes shortly after, won't that result in a use-after-free in the SPI
core or applespi_async_complete() when it accesses the freed driver data?

[Severity: High]
Does this wait condition race with the inner completion callback?

Looking at applespi_async_complete(), it signals completion before executing
the inner callback:

drivers/input/keyboard/applespi.c:applespi_async_complete() {
    ...
    info->complete = NULL;

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

If the teardown thread wakes up as soon as applespi_async_outstanding() is
false and frees the device structure, will complete(applespi) dereference
freed memory?

> +	spin_unlock_irqrestore(&applespi->cmd_msg_lock, flags);
>  
> +	/* Disable GPE and remove handler */
>  	acpi_disable_gpe(NULL, applespi->gpe);
>  	acpi_remove_gpe_handler(NULL, applespi->gpe, applespi_notify);
>  	device_wakeup_disable(&spi->dev);
>  
> -	applespi_drain_reads(applespi);
> -
>  	debugfs_remove_recursive(applespi->debugfs_root);
>  }

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