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

[sashiko-bot@kernel.org]

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

New issues:
- [Medium] Improper mixing of `spin_lock_irqsave` with `wait_event_lock_irq` variants.

Pre-existing issues:
- [High] The driver fails to clear the `cancel_spi` flag during resume, rendering the keyboard and touchpad permanently dead after the first suspend cycle.
--

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

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

Replace the separate read and write drain helpers with a single barrier using
cancel_spi and wait_event_lock_irq_timeout(). Wait for outstanding
asynchronous SPI operations to complete before tearing down the driver
context.

> diff --git a/drivers/input/keyboard/applespi.c b/drivers/input/keyboard/applespi.c
> index a21e89f303879..6027808421247 100644
> --- a/drivers/input/keyboard/applespi.c
> +++ b/drivers/input/keyboard/applespi.c
> @@ -1939,14 +1939,29 @@ 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;
> +	long ret;
>  
> -	applespi_drain_writes(applespi);
> -
> +	/* Disable GPE and remove handler first to prevent interrupt storm */
>  	acpi_disable_gpe(NULL, applespi->gpe);
>  	acpi_remove_gpe_handler(NULL, applespi->gpe, applespi_notify);
>  	device_wakeup_disable(&spi->dev);
>  
> -	applespi_drain_reads(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]
This is a pre-existing issue, but does the driver fail to clear the
cancel_spi flag during resume?

When the system suspends, applespi_drain_writes() sets
applespi->cancel_spi = true. Upon waking up, applespi_resume() executes a
state restoration block but appears to omit resetting this flag:

applespi_resume() {
	...
	scoped_guard(spinlock_irqsave, &applespi->cmd_msg_lock) {
		applespi->drain = false;
		applespi->have_cl_led_on = false;
		applespi->have_bl_level = 0;
		applespi->cmd_msg_queued = 0;
		applespi->read_active = false;
		applespi->write_active = false;

		applespi->suspended = false;
	}
	...
}

Because of this, subsequent calls to applespi_async() will reject all new
SPI transfers and return -ESHUTDOWN, leaving the keyboard and touchpad
permanently unresponsive after the first suspend cycle.

Could cancel_spi be reset to false in applespi_resume() to avoid this?

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

[Severity: Medium]
Is there an API mismatch between spin_lock_irqsave() and the lock variants
used inside the wait macros?

applespi_remove() acquires the lock using spin_lock_irqsave(). Inside this
critical section, it calls wait_event_lock_irq_timeout().

This macro internally drops the lock using spin_unlock_irq(), which
unconditionally enables interrupts, and then reacquires it with
spin_lock_irq(). This completely overrides the interrupt state saved in the
flags variable.

Should this code just use spin_lock_irq() directly to avoid the mismatch?

> +	if (!ret && applespi_async_outstanding(applespi)) {
> +		dev_warn(&applespi->spi->dev,
> +			 "Timed out waiting for SPI transfers to drain, waiting unconditionally\n");
> +		wait_event_lock_irq(applespi->wait_queue,
> +				    !applespi_async_outstanding(applespi),
> +				    applespi->cmd_msg_lock);
> +	}
> +	spin_unlock_irqrestore(&applespi->cmd_msg_lock, flags);
>  
>  	debugfs_remove_recursive(applespi->debugfs_root);
>  }

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