Re: [PATCH v3 5/5] Input: applespi - fix use-after-free in applespi_remove()
From: sashiko-bot@kernel.org
Date: 2026-07-20 16:37:06
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 [off-list ref] 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.
quoted hunk ↗ jump to hunk
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