[PATCH v3 0/5] Input: applespi - Fix probe timeout and use-after-free bugs

[PATCH v3 0/5] Input: applespi - Fix probe timeout and use-after-free bugs

[Shih-Yuan Lee]

This 5-patch series refactors the applespi driver for Apple SPI keyboards and touchpads,
eliminating use-after-free (UAF) race conditions during driver unbind, kernel panics
in debugfs, level-triggered ACPI GPE interrupt storms, and data races.

Specifically, this series:
  - Replaces separate drain helpers with a unified wait queue and 3-second timeout barrier,
    falling back to unconditional completion waiting if transfers remain active to prevent
    premature driver unbind and DMA memory corruption.
  - Tracks in-flight asynchronous SPI transfers using a completion tracking array and
    cancel_spi flag, ensuring inner completion callbacks execute prior to waking wait
    queue waiters to guarantee teardown safety.
  - Converts touchpad detection and registration from an asynchronous workqueue worker
    to a synchronous probe sequence, while protecting against NULL pointer dereferences in
    debugfs during keyboard-only fallback mode.
  - Corrects driver unbind sequence by disabling ACPI GPE handlers before marking the driver
    as shutting down, avoiding level-triggered interrupt storms on unhandled reads.

Changes in v3:
  - Addressed feedback from Sashiko review on the v2 patchset:
  - Fixed Use-After-Free in applespi_drain_writes() and applespi_drain_reads()
    on timeout by falling back to unconditional wait (wait_event_lock_irq()) if
    transfers are still active upon 3-second timeout expiry.
  - Activated cancel_spi tracking flag by setting cancel_spi = true in drain functions
    under cmd_msg_lock to reject new async requests (-ESHUTDOWN) and trigger wait
    queue wakeups.
  - Fixed completion callback race in applespi_async_complete() by invoking inner
    complete(applespi) callback BEFORE clearing tracking slot and waking waiters
    on wait_queue.
  - Fixed NULL pointer dereference in applespi_tp_dim_open() when touchpad registration
    fails or times out, returning -ENODEV if touchpad_input_dev is NULL.
  - Protected rcvd_tp_info from data races in probe by making a safe local copy under
    cmd_msg_lock before passing to applespi_register_touchpad_device().
  - Fixed GPE Interrupt Storm during unbind in applespi_remove() by disabling and
    removing GPE handler BEFORE setting cancel_spi = true and waiting for transfers to drain.

Changes since v1:
  - Split the large touchpad registration patch into 4 distinct, single-purpose
    commits (wait queue consolidation & timeouts, async queue slots tracking,
    synchronous registration, and async probe preference) for better readability.
  - Fixed a self-deadlock in applespi_async() where it attempted to acquire
    cmd_msg_lock while already held by applespi_notify() and other callers.
  - Updated applespi_async() to assert the lock is held by the caller.
  - Protected touchpad info flags under cmd_msg_lock in response handling.
  - Simplified applespi_tp_dim_open() by removing redundant NULL checks.
  - Overhauled GPE disabling and teardown order in applespi_remove() to prevent
    GPE interrupt storms and unbind deadlocks.
  - Addressed reviewer feedback from Sashiko.

Shih-Yuan Lee (5):
  Input: applespi - use unified wait queue with timeouts for drain
  Input: applespi - track asynchronous SPI transfers in flight
  Input: applespi - register touchpad synchronously in probe
  Input: applespi - prefer asynchronous driver probing
  Input: applespi - fix use-after-free in applespi_remove()

 drivers/input/keyboard/applespi.c | 208 +++++++++++++++++++++++++-----
 1 file changed, 175 insertions(+), 33 deletions(-)

-- 
2.39.5

[PATCH v3 1/5] Input: applespi - use unified wait queue with timeouts for drain

[Shih-Yuan Lee]

Consolidate wait queues by renaming drain_complete to wait_queue and use
wait_event_lock_irq_timeout() with a 3-second timeout during read and write
drains to prevent indefinite lockups during driver unbind or PM transitions
if the hardware becomes unresponsive.

If a timeout occurs while a read or write transfer is still active, issue
a warning and fallback to waiting unconditionally until the transfer completion
callback clears the active flag. This guarantees that applespi_drain_writes()
and applespi_drain_reads() never return prematurely while transfers are in flight,
preventing use-after-free (UAF) and DMA memory corruption when devres frees
the driver context and DMA buffers during unbind.

Signed-off-by: Shih-Yuan Lee <fourdollars@debian.org>
---
 drivers/input/keyboard/applespi.c | 48 ++++++++++++++++++++++++-------
 1 file changed, 38 insertions(+), 10 deletions(-)

diff --git a/drivers/input/keyboard/applespi.c b/drivers/input/keyboard/applespi.c
index b5ff71cd5a70..c501bf7e517e 100644
--- a/drivers/input/keyboard/applespi.c
+++ b/drivers/input/keyboard/applespi.c
@@ -417,7 +417,7 @@ struct applespi_data {
 
 	bool				suspended;
 	bool				drain;
-	wait_queue_head_t		drain_complete;
+	wait_queue_head_t		wait_queue;
 	bool				read_active;
 	bool				write_active;
 
@@ -677,7 +677,7 @@ static int applespi_setup_spi(struct applespi_data *applespi)
 		return sts;
 
 	spin_lock_init(&applespi->cmd_msg_lock);
-	init_waitqueue_head(&applespi->drain_complete);
+	init_waitqueue_head(&applespi->wait_queue);
 
 	return 0;
 }
@@ -725,7 +725,7 @@ static void applespi_msg_complete(struct applespi_data *applespi,
 		applespi->write_active = false;
 
 	if (applespi->drain && !applespi->write_active)
-		wake_up_all(&applespi->drain_complete);
+		wake_up_all(&applespi->wait_queue);
 
 	if (is_write_msg) {
 		applespi->cmd_msg_queued = 0;
@@ -1415,7 +1415,7 @@ static void applespi_got_data(struct applespi_data *applespi)
 			applespi->read_active = false;
 			applespi->write_active = false;
 
-			wake_up_all(&applespi->drain_complete);
+			wake_up_all(&applespi->wait_queue);
 		}
 
 		return;
@@ -1793,21 +1793,49 @@ static int applespi_probe(struct spi_device *spi)
 
 static void applespi_drain_writes(struct applespi_data *applespi)
 {
-	guard(spinlock_irqsave)(&applespi->cmd_msg_lock);
+	unsigned long flags;
+	long ret;
+
+	spin_lock_irqsave(&applespi->cmd_msg_lock, flags);
 
 	applespi->drain = true;
-	wait_event_lock_irq(applespi->drain_complete, !applespi->write_active,
-			    applespi->cmd_msg_lock);
+	ret = wait_event_lock_irq_timeout(applespi->wait_queue,
+					  !applespi->write_active,
+					  applespi->cmd_msg_lock,
+					  msecs_to_jiffies(3000));
+	if (!ret && applespi->write_active) {
+		dev_warn(&applespi->spi->dev,
+			 "Timed out waiting for write drain, waiting unconditionally\n");
+		wait_event_lock_irq(applespi->wait_queue,
+				    !applespi->write_active,
+				    applespi->cmd_msg_lock);
+	}
+
+	spin_unlock_irqrestore(&applespi->cmd_msg_lock, flags);
 }
 
 static void applespi_drain_reads(struct applespi_data *applespi)
 {
-	guard(spinlock_irqsave)(&applespi->cmd_msg_lock);
+	unsigned long flags;
+	long ret;
 
-	wait_event_lock_irq(applespi->drain_complete, !applespi->read_active,
-			    applespi->cmd_msg_lock);
+	spin_lock_irqsave(&applespi->cmd_msg_lock, flags);
+
+	ret = wait_event_lock_irq_timeout(applespi->wait_queue,
+					  !applespi->read_active,
+					  applespi->cmd_msg_lock,
+					  msecs_to_jiffies(3000));
+	if (!ret && applespi->read_active) {
+		dev_warn(&applespi->spi->dev,
+			 "Timed out waiting for read drain, waiting unconditionally\n");
+		wait_event_lock_irq(applespi->wait_queue,
+				    !applespi->read_active,
+				    applespi->cmd_msg_lock);
+	}
 
 	applespi->suspended = true;
+
+	spin_unlock_irqrestore(&applespi->cmd_msg_lock, flags);
 }
 
 static void applespi_remove(struct spi_device *spi)
-- 
2.39.5

[PATCH v3 2/5] Input: applespi - track asynchronous SPI transfers in flight

[Shih-Yuan Lee]

The driver queues read and write packets asynchronously. When shutting
down, removing, or suspending, the driver must guarantee that no
asynchronous transfers remain in flight to prevent memory corruption or
use-after-free conditions.

Introduce a 'spi_complete' slot tracking array in struct applespi_data
to represent the two concurrent transfers (one for reads, one for
writes). Implement applespi_async_outstanding() and
applespi_async_complete() to track transfers under cmd_msg_lock.

Set applespi->cancel_spi = true during drain operations to reject new
asynchronous transfer requests with -ESHUTDOWN and activate completion
tracking.

In applespi_async_complete(), invoke the inner completion callback BEFORE
clearing the tracking slot and waking waiters on wait_queue. This ensures
that teardown threads waiting for in-flight transfers to drain cannot wake up
and free the driver structure before the completion callback finishes execution,
preventing use-after-free conditions.

Signed-off-by: Shih-Yuan Lee <fourdollars@debian.org>
---
 drivers/input/keyboard/applespi.c | 75 +++++++++++++++++++++++++++++--
 1 file changed, 72 insertions(+), 3 deletions(-)

diff --git a/drivers/input/keyboard/applespi.c b/drivers/input/keyboard/applespi.c
index c501bf7e517e..c9bbceaf0671 100644
--- a/drivers/input/keyboard/applespi.c
+++ b/drivers/input/keyboard/applespi.c
@@ -421,6 +421,12 @@ struct applespi_data {
 	bool				read_active;
 	bool				write_active;
 
+	struct applespi_complete_info {
+		void				(*complete)(void *context);
+		struct applespi_data		*applespi;
+	}				spi_complete[2];
+	bool				cancel_spi;
+
 	struct work_struct		work;
 	struct touchpad_info_protocol	rcvd_tp_info;
 
@@ -607,13 +613,73 @@ static void applespi_setup_write_txfrs(struct applespi_data *applespi)
 	spi_message_add_tail(st_t, msg);
 }
 
+static bool applespi_async_outstanding(struct applespi_data *applespi)
+{
+	return applespi->spi_complete[0].complete ||
+	       applespi->spi_complete[1].complete;
+}
+
+static void applespi_async_complete(void *context)
+{
+	struct applespi_complete_info *info = context;
+	struct applespi_data *applespi = info->applespi;
+	void (*complete)(void *context);
+	unsigned long flags;
+
+	spin_lock_irqsave(&applespi->cmd_msg_lock, flags);
+	complete = info->complete;
+	spin_unlock_irqrestore(&applespi->cmd_msg_lock, flags);
+
+	if (complete)
+		complete(applespi);
+
+	spin_lock_irqsave(&applespi->cmd_msg_lock, flags);
+	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);
+}
+
 static int applespi_async(struct applespi_data *applespi,
 			  struct spi_message *message, void (*complete)(void *))
 {
-	message->complete = complete;
-	message->context = applespi;
+	struct applespi_complete_info *info;
+	int sts;
+
+	assert_spin_locked(&applespi->cmd_msg_lock);
+
+	if (applespi->cancel_spi) {
+		if (!applespi_async_outstanding(applespi))
+			wake_up_all(&applespi->wait_queue);
+		return -ESHUTDOWN;
+	}
+
+	/*
+	 * There can only be at most 2 spi requests in flight, one for "reads"
+	 * and one for "writes".
+	 */
+	if (!applespi->spi_complete[0].complete)
+		info = &applespi->spi_complete[0];
+	else if (!applespi->spi_complete[1].complete)
+		info = &applespi->spi_complete[1];
+	else {
+		dev_warn(&applespi->spi->dev, "Both SPI async slots in use\n");
+		return -EBUSY;
+	}
+
+	info->complete = complete;
+	info->applespi = applespi;
 
-	return spi_async(applespi->spi, message);
+	message->complete = applespi_async_complete;
+	message->context = info;
+
+	sts = spi_async(applespi->spi, message);
+	if (sts)
+		info->complete = NULL;
+
+	return sts;
 }
 
 static inline bool applespi_check_write_status(struct applespi_data *applespi,
@@ -1799,6 +1865,7 @@ static void applespi_drain_writes(struct applespi_data *applespi)
 	spin_lock_irqsave(&applespi->cmd_msg_lock, flags);
 
 	applespi->drain = true;
+	applespi->cancel_spi = true;
 	ret = wait_event_lock_irq_timeout(applespi->wait_queue,
 					  !applespi->write_active,
 					  applespi->cmd_msg_lock,
@@ -1821,6 +1888,8 @@ static void applespi_drain_reads(struct applespi_data *applespi)
 
 	spin_lock_irqsave(&applespi->cmd_msg_lock, flags);
 
+	applespi->cancel_spi = true;
+
 	ret = wait_event_lock_irq_timeout(applespi->wait_queue,
 					  !applespi->read_active,
 					  applespi->cmd_msg_lock,
-- 
2.39.5

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

[Shih-Yuan Lee]

Replace the asynchronous worker for touchpad registration with synchronous
registration during driver probe. Wait up to 3 seconds for the device info
packet and fallback to keyboard-only mode if a timeout occurs.

Prevent a kernel panic in debugfs by checking for a NULL touchpad_input_dev
in applespi_tp_dim_open() before dereferencing it, returning -ENODEV if the
device is operating in keyboard-only mode.

Protect rcvd_tp_info from data races by creating a local copy under
cmd_msg_lock before passing it to applespi_register_touchpad_device().

Signed-off-by: Shih-Yuan Lee <fourdollars@debian.org>
---
 drivers/input/keyboard/applespi.c | 60 ++++++++++++++++++++++---------
 1 file changed, 44 insertions(+), 16 deletions(-)

diff --git a/drivers/input/keyboard/applespi.c b/drivers/input/keyboard/applespi.c
index c9bbceaf0671..6db4eeca3060 100644
--- a/drivers/input/keyboard/applespi.c
+++ b/drivers/input/keyboard/applespi.c
@@ -427,7 +427,7 @@ struct applespi_data {
 	}				spi_complete[2];
 	bool				cancel_spi;
 
-	struct work_struct		work;
+	bool				have_tp_info;
 	struct touchpad_info_protocol	rcvd_tp_info;
 
 	struct dentry			*debugfs_root;
@@ -1030,6 +1030,9 @@ static int applespi_tp_dim_open(struct inode *inode, struct file *file)
 {
 	struct applespi_data *applespi = inode->i_private;
 
+	if (!applespi->touchpad_input_dev)
+		return -ENODEV;
+
 	file->private_data = applespi;
 
 	snprintf(applespi->tp_dim_val, sizeof(applespi->tp_dim_val),
@@ -1390,26 +1393,20 @@ applespi_register_touchpad_device(struct applespi_data *applespi,
 	return 0;
 }
 
-static void applespi_worker(struct work_struct *work)
-{
-	struct applespi_data *applespi =
-		container_of(work, struct applespi_data, work);
-
-	applespi_register_touchpad_device(applespi, &applespi->rcvd_tp_info);
-}
-
 static void applespi_handle_cmd_response(struct applespi_data *applespi,
 					 struct spi_packet *packet,
 					 struct message *message)
 {
+	unsigned long flags;
+
 	if (packet->device == PACKET_DEV_INFO &&
 	    le16_to_cpu(message->type) == 0x1020) {
-		/*
-		 * We're not allowed to sleep here, but registering an input
-		 * device can sleep.
-		 */
+		spin_lock_irqsave(&applespi->cmd_msg_lock, flags);
 		applespi->rcvd_tp_info = message->tp_info;
-		schedule_work(&applespi->work);
+		applespi->have_tp_info = true;
+		spin_unlock_irqrestore(&applespi->cmd_msg_lock, flags);
+
+		wake_up_all(&applespi->wait_queue);
 		return;
 	}
 
@@ -1677,6 +1674,7 @@ static int applespi_probe(struct spi_device *spi)
 	acpi_handle spi_handle = ACPI_HANDLE(&spi->dev);
 	acpi_status acpi_sts;
 	int sts, i;
+	unsigned long flags;
 	unsigned long long gpe, usb_status;
 
 	/* check if the USB interface is present and enabled already */
@@ -1694,8 +1692,6 @@ static int applespi_probe(struct spi_device *spi)
 
 	applespi->spi = spi;
 
-	INIT_WORK(&applespi->work, applespi_worker);
-
 	/* store the driver data */
 	spi_set_drvdata(spi, applespi);
 
@@ -1823,6 +1819,25 @@ 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");
+	} else {
+		struct touchpad_info_protocol tp_info;
+
+		spin_lock_irqsave(&applespi->cmd_msg_lock, flags);
+		tp_info = applespi->rcvd_tp_info;
+		spin_unlock_irqrestore(&applespi->cmd_msg_lock, flags);
+
+		sts = applespi_register_touchpad_device(applespi, &tp_info);
+		if (sts)
+			goto cancel_spi;
+	}
+
 	/*
 	 * By default this device is not enabled for wakeup; but USB keyboards
 	 * generally are, so the expectation is that by default the keyboard
@@ -1855,6 +1870,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);
+
+	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;
 }
 
 static void applespi_drain_writes(struct applespi_data *applespi)
-- 
2.39.5

[PATCH v3 4/5] Input: applespi - prefer asynchronous driver probing

[Shih-Yuan Lee]

Set probe_type to PROBE_PREFER_ASYNCHRONOUS to allow the driver core
to run applespi_probe() asynchronously. This improves system boot speeds by
avoiding blocking the main kernel thread during the 3-second touchpad
detection wait.

Additionally, clean up applespi_tp_dim_open() by simplifying product ID
retrieval and avoiding dereferencing touchpad_input_dev without a helper
variable.

Signed-off-by: Shih-Yuan Lee <fourdollars@debian.org>
---
 drivers/input/keyboard/applespi.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/input/keyboard/applespi.c b/drivers/input/keyboard/applespi.c
index 6db4eeca3060..a21e89f30387 100644
--- a/drivers/input/keyboard/applespi.c
+++ b/drivers/input/keyboard/applespi.c
@@ -1029,6 +1029,7 @@ static void applespi_debug_update_dimensions(struct applespi_data *applespi,
 static int applespi_tp_dim_open(struct inode *inode, struct file *file)
 {
 	struct applespi_data *applespi = inode->i_private;
+	struct input_dev *touchpad = applespi->touchpad_input_dev;
 
 	if (!applespi->touchpad_input_dev)
 		return -ENODEV;
@@ -1037,7 +1038,7 @@ static int applespi_tp_dim_open(struct inode *inode, struct file *file)
 
 	snprintf(applespi->tp_dim_val, sizeof(applespi->tp_dim_val),
 		 "0x%.4x %dx%d+%u+%u\n",
-		 applespi->touchpad_input_dev->id.product,
+		 touchpad->id.product,
 		 applespi->tp_dim_min_x, applespi->tp_dim_min_y,
 		 applespi->tp_dim_max_x - applespi->tp_dim_min_x,
 		 applespi->tp_dim_max_y - applespi->tp_dim_min_y);
@@ -2044,6 +2045,7 @@ static struct spi_driver applespi_driver = {
 		.name			= "applespi",
 		.acpi_match_table	= applespi_acpi_match,
 		.pm			= pm_sleep_ptr(&applespi_pm_ops),
+		.probe_type		= PROBE_PREFER_ASYNCHRONOUS,
 	},
 	.probe		= applespi_probe,
 	.remove		= applespi_remove,
-- 
2.39.5

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

[Shih-Yuan Lee]

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.

Disable GPE and remove the GPE handler prior to setting cancel_spi = true.
This prevents level-triggered GPE interrupt storms where applespi_notify()
repeatedly attempts applespi_async() and gets rejected with -ESHUTDOWN while
the hardware interrupt line remains asserted.

If the 3-second wait times out while SPI transfers are still outstanding, issue
a warning and fallback to waiting unconditionally until all transfers complete,
guaranteeing that applespi_remove() never returns prematurely while transfers
are active.

Signed-off-by: Shih-Yuan Lee <fourdollars@debian.org>
---
 drivers/input/keyboard/applespi.c | 21 ++++++++++++++++++---
 1 file changed, 18 insertions(+), 3 deletions(-)

diff --git a/drivers/input/keyboard/applespi.c b/drivers/input/keyboard/applespi.c
index a21e89f30387..602780842124 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;
+	ret = wait_event_lock_irq_timeout(applespi->wait_queue,
+					  !applespi_async_outstanding(applespi),
+					  applespi->cmd_msg_lock,
+					  msecs_to_jiffies(3000));
+	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);
 }
-- 
2.39.5

Re: [PATCH v3 2/5] Input: applespi - track asynchronous SPI transfers in flight

[sashiko-bot]

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

New issues:
- [High] Teardown threads wake up prematurely and free `applespi` while `applespi_async_complete` is still running, causing a Use-After-Free.
- [High] Consecutive SPI transfers fail with -EBUSY due to occupied tracking slots, dropping commands and stalling the driver.
- [High] The driver becomes completely broken after a suspend/resume cycle because `cancel_spi` is never reset.

Pre-existing issues:
- [High] Missing cancellation of `applespi->work` in `applespi_remove()` leads to a Use-After-Free.
--

--- Patch [2]: [PATCH v3 2/5] Input: applespi - track asynchronous SPI transfers in flight ---
commit 461be9b81e79a5a61b76018d2f876c28cc39167f
Author: Shih-Yuan Lee <fourdollars@debian.org>

Input: applespi - track asynchronous SPI transfers in flight

The driver queues read and write packets asynchronously. When shutting
down, removing, or suspending, the driver must guarantee that no
asynchronous transfers remain in flight to prevent memory corruption or
use-after-free conditions. This commit introduces a tracking mechanism
for these async transfers using a spi_complete slot array.

> diff --git a/drivers/input/keyboard/applespi.c b/drivers/input/keyboard/applespi.c
> index c501bf7e517e4..c9bbceaf06718 100644
> --- a/drivers/input/keyboard/applespi.c
> +++ b/drivers/input/keyboard/applespi.c

[ ... ]

> @@ -607,13 +613,73 @@ static void applespi_setup_write_txfrs(struct applespi_data *applespi)
>  	spi_message_add_tail(st_t, msg);
>  }
>  
> +static bool applespi_async_outstanding(struct applespi_data *applespi)
> +{
> +	return applespi->spi_complete[0].complete ||
> +	       applespi->spi_complete[1].complete;
> +}
> +
> +static void applespi_async_complete(void *context)
> +{
> +	struct applespi_complete_info *info = context;
> +	struct applespi_data *applespi = info->applespi;
> +	void (*complete)(void *context);
> +	unsigned long flags;
> +
> +	spin_lock_irqsave(&applespi->cmd_msg_lock, flags);
> +	complete = info->complete;
> +	spin_unlock_irqrestore(&applespi->cmd_msg_lock, flags);
> +
> +	if (complete)
> +		complete(applespi);

[Severity: High]
If the inner complete callback queues a new write command, will
applespi_async() see the first slot as still occupied because info->complete
has not been cleared yet?

If a read transfer happens to be active in the other slot, could this cause
applespi_async() to return -EBUSY and drop the new command, permanently
stalling driver initialization?

> +
> +	spin_lock_irqsave(&applespi->cmd_msg_lock, flags);
> +	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);
> +}

[Severity: High]
Can this sequence cause a use-after-free regression?

If applespi_drain_writes() is waiting for write_active to become false:

CPU1 running applespi_async_complete():
    complete(applespi);

The inner callback sets write_active = false and wakes wait_queue.

CPU2 running applespi_drain_writes():
    wakes up, finishes applespi_remove(), and frees the applespi structure

Could CPU1 then try to acquire the spinlock and clear info->complete on the
freed applespi structure?

[ ... ]

> @@ -1799,6 +1865,7 @@ static void applespi_drain_writes(struct applespi_data *applespi)
>  	spin_lock_irqsave(&applespi->cmd_msg_lock, flags);
>  
>  	applespi->drain = true;
> +	applespi->cancel_spi = true;
>  	ret = wait_event_lock_irq_timeout(applespi->wait_queue,
>  					  !applespi->write_active,
>  					  applespi->cmd_msg_lock,

[Severity: High]
If applespi_drain_writes() sets cancel_spi to true during applespi_suspend(),
is it ever reset to false in applespi_resume()?

Could this cause applespi_async() to permanently reject new transfers with
-ESHUTDOWN after the system resumes?

[Severity: High]
This isn't a bug introduced by this patch, but while looking at the driver
teardown path, does applespi_remove() need to call
cancel_work_sync(&applespi->work)?

If applespi->work is queued to register the touchpad and the module is
unbound, could devres free the applespi structure before applespi_worker()
executes, resulting in a use-after-free regression?

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

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

[sashiko-bot]

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

Re: [PATCH v3 4/5] Input: applespi - prefer asynchronous driver probing

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] Concurrent data race on the global `touchpad_dimensions` array due to asynchronous probing.
--

--- Patch [4]: [PATCH v3 4/5] Input: applespi - prefer asynchronous driver probing ---
commit b9f1c9a61a0fcf093f49319a208df622cfeba00e
Author: Shih-Yuan Lee <fourdollars@debian.org>
Input: applespi - prefer asynchronous driver probing

This commit enables asynchronous probing for the applespi driver to avoid
blocking the main kernel thread. It also cleans up product ID retrieval
in applespi_tp_dim_open().

> 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
[ ... ]
> @@ -2044,6 +2045,7 @@ static struct spi_driver applespi_driver = {
>  		.name			= "applespi",
>  		.acpi_match_table	= applespi_acpi_match,
>  		.pm			= pm_sleep_ptr(&applespi_pm_ops),
> +		.probe_type		= PROBE_PREFER_ASYNCHRONOUS,

[Severity: Medium]
Could enabling asynchronous probing here cause a data race on the global
touchpad_dimensions array?

Looking at applespi_register_touchpad_device(), it reads from and modifies
the global touchpad_dimensions module parameter without locking:

drivers/input/keyboard/applespi.c:applespi_register_touchpad_device()
        sts = sscanf(touchpad_dimensions, "%dx%d+%u+%u", &x, &y, &w, &h);
        ...
        touchpad_dimensions[0] = '\0';
        ...
        snprintf(touchpad_dimensions, sizeof(touchpad_dimensions), ...

If a system has multiple instances of the APP000D ACPI device, the driver
core could probe them concurrently, potentially corrupting this string or
causing incorrect dimensions to be read and parsed.

>  	},
>  	.probe		= applespi_probe,
>  	.remove		= applespi_remove,

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

Re: [PATCH v3 1/5] Input: applespi - use unified wait queue with timeouts for drain

[sashiko-bot]

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

New issues:
- [Medium] The patch attempts to prevent indefinite lockups during driver unbind or PM transitions by adding a timeout to the drain waits. However, upon timeout, it explicitly falls back to waiting unconditionally on the exact same condition, neutralizing the protection and still causing an indefinite lockup.
- [Low] Unnecessary removal of scope-based cleanup `guard()`, reverting to manual spinlock management.

Pre-existing issues:
- [Critical] Missing cancel_work_sync() in applespi_remove() causes Use-After-Free of applespi_data when the workqueue executes.
--

--- Patch [1]: [PATCH v3 1/5] Input: applespi - use unified wait queue with timeouts for drain ---
commit 08fd2aeb0171b9531113cd115a28c72f63462dc0
Author: Shih-Yuan Lee <fourdollars@debian.org>

Input: applespi - use unified wait queue with timeouts for drain
    
Consolidate wait queues by renaming drain_complete to wait_queue and use
wait_event_lock_irq_timeout() with a 3-second timeout during read and write
drains to prevent indefinite lockups during driver unbind or PM transitions
if the hardware becomes unresponsive.

> diff --git a/drivers/input/keyboard/applespi.c b/drivers/input/keyboard/applespi.c
> index b5ff71cd5a701..c501bf7e517e4 100644
> --- a/drivers/input/keyboard/applespi.c
> +++ b/drivers/input/keyboard/applespi.c

[ ... ]

> @@ -1793,21 +1793,49 @@ static int applespi_probe(struct spi_device *spi)
>  
>  static void applespi_drain_writes(struct applespi_data *applespi)
>  {
> -	guard(spinlock_irqsave)(&applespi->cmd_msg_lock);
> +	unsigned long flags;
> +	long ret;
> +
> +	spin_lock_irqsave(&applespi->cmd_msg_lock, flags);

[Severity: Low]
This isn't a bug, but is it necessary to revert from guard() to manual
spin_lock_irqsave() here?

The input subsystem guidelines prefer using modern cleanup primitives like
guard() in new or refactored code. wait_event_lock_irq_timeout() should
work correctly with the hidden flags variable established by guard().

>  
>  	applespi->drain = true;
> -	wait_event_lock_irq(applespi->drain_complete, !applespi->write_active,
> -			    applespi->cmd_msg_lock);
> +	ret = wait_event_lock_irq_timeout(applespi->wait_queue,
> +					  !applespi->write_active,
> +					  applespi->cmd_msg_lock,
> +					  msecs_to_jiffies(3000));
> +	if (!ret && applespi->write_active) {
> +		dev_warn(&applespi->spi->dev,
> +			 "Timed out waiting for write drain, waiting unconditionally\n");
> +		wait_event_lock_irq(applespi->wait_queue,
> +				    !applespi->write_active,
> +				    applespi->cmd_msg_lock);

[Severity: Medium]
Does this fallback logic defeat the purpose of the timeout?

The commit message states the timeout is meant to prevent indefinite lockups,
but if a timeout occurs here, wait_event_lock_irq() waits unconditionally
on the exact same condition. 

If the hardware is genuinely unresponsive, won't this just delay the indefinite
lockup by 3 seconds rather than preventing it?

> +	}
> +
> +	spin_unlock_irqrestore(&applespi->cmd_msg_lock, flags);
>  }
>  
>  static void applespi_drain_reads(struct applespi_data *applespi)
>  {
> -	guard(spinlock_irqsave)(&applespi->cmd_msg_lock);
> +	unsigned long flags;
> +	long ret;
>  
> -	wait_event_lock_irq(applespi->drain_complete, !applespi->read_active,
> -			    applespi->cmd_msg_lock);
> +	spin_lock_irqsave(&applespi->cmd_msg_lock, flags);
> +
> +	ret = wait_event_lock_irq_timeout(applespi->wait_queue,
> +					  !applespi->read_active,
> +					  applespi->cmd_msg_lock,
> +					  msecs_to_jiffies(3000));
> +	if (!ret && applespi->read_active) {
> +		dev_warn(&applespi->spi->dev,
> +			 "Timed out waiting for read drain, waiting unconditionally\n");
> +		wait_event_lock_irq(applespi->wait_queue,
> +				    !applespi->read_active,
> +				    applespi->cmd_msg_lock);

[Severity: Medium]
Does this read drain fallback have the same indefinite lockup issue as the 
write drain when the hardware fails?

> +	}
>  
>  	applespi->suspended = true;
> +
> +	spin_unlock_irqrestore(&applespi->cmd_msg_lock, flags);
>  }
>  
>  static void applespi_remove(struct spi_device *spi)

[Severity: Critical]
This is a pre-existing issue, but does applespi_remove() miss a call to
cancel_work_sync(&applespi->work) before returning?

If the driver unbinds while a touchpad info SPI transaction is in flight,
applespi_drain_reads() will block until the read finishes. 

When the SPI read completes, its handler schedules applespi->work, clears
read_active, and wakes up the drain. 

If applespi_remove() finishes without cancelling the work, devres will
free applespi_data, which could lead to a use-after-free when the scheduled
applespi_worker() executes.

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