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

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

[Shih-Yuan Lee]

This patch series simplifies the applespi driver by making touchpad
detection and registration synchronous during probe, and fixes a
use-after-free bug in applespi_remove().

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 | 175 ++++++++++++++++++++++++------
 1 file changed, 140 insertions(+), 35 deletions(-)

-- 
2.39.5

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

[Shih-Yuan Lee]

The driver currently maintains a dedicated wait queue 'drain_complete'
specifically to wait for outstanding write requests to complete.
Consolidate this with other wait events by renaming it to 'wait_queue' to
make resource management cleaner.

Furthermore, using wait_event_lock_irq() without a timeout risks blocking
the thread indefinitely during driver unbinding (remove) or PM transition
phases if the hardware fails to respond or interrupts are missed.

Replace wait_event_lock_irq() with wait_event_lock_irq_timeout() in
applespi_drain_writes() and applespi_drain_reads() with a 3-second timeout.
This ensures the driver can gracefully recover and avoid lockups under
unresponsive hardware conditions.

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

diff --git a/drivers/input/keyboard/applespi.c b/drivers/input/keyboard/applespi.c
index b5ff71cd5a70..64bbeba85ea9 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,33 @@ 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;
+
+	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);
+	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);
 }
 
 static void applespi_drain_reads(struct applespi_data *applespi)
 {
-	guard(spinlock_irqsave)(&applespi->cmd_msg_lock);
+	unsigned long flags;
+
+	spin_lock_irqsave(&applespi->cmd_msg_lock, flags);
 
-	wait_event_lock_irq(applespi->drain_complete, !applespi->read_active,
-			    applespi->cmd_msg_lock);
+	wait_event_lock_irq_timeout(applespi->wait_queue,
+				    !applespi->read_active,
+				    applespi->cmd_msg_lock,
+				    msecs_to_jiffies(3000));
 
 	applespi->suspended = true;
+
+	spin_unlock_irqrestore(&applespi->cmd_msg_lock, flags);
 }
 
 static void applespi_remove(struct spi_device *spi)
-- 
2.39.5

[PATCH v2 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.

Modify applespi_async() to allocate a completion slot and assert that the
caller holds the required cmd_msg_lock. This ensures robust and lock-safe
tracking of all asynchronous SPI transactions.

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

diff --git a/drivers/input/keyboard/applespi.c b/drivers/input/keyboard/applespi.c
index 64bbeba85ea9..a8f8d5370e95 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,71 @@ 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;
+	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);
+}
+
 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;
+
+	message->complete = applespi_async_complete;
+	message->context = info;
+
+	sts = spi_async(applespi->spi, message);
+	if (sts)
+		info->complete = NULL;
 
-	return spi_async(applespi->spi, message);
+	return sts;
 }
 
 static inline bool applespi_check_write_status(struct applespi_data *applespi,
-- 
2.39.5

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

[Shih-Yuan Lee]

Touchpad registration is currently deferred to an asynchronous worker
applespi_worker(). This asynchronous registration introduces race
conditions if debugfs or other properties are accessed before the worker
completes, or if the driver is unbound while the worker is active.

Remove the workqueue and the asynchronous worker. Perform touchpad
registration synchronously during driver probe.

Wait up to 3 seconds for the touchpad information command packet response
using wait_event_timeout(). If the response times out, log a warning and
fallback to keyboard-only mode. If registration fails, gracefully unwind GPE
handlers and wait for outstanding SPI transactions to complete.

To prevent data races between the interrupt handler and the probe thread,
protect the 'have_tp_info' flag and 'rcvd_tp_info' structure under the
cmd_msg_lock in applespi_handle_cmd_response().

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

diff --git a/drivers/input/keyboard/applespi.c b/drivers/input/keyboard/applespi.c
index a8f8d5370e95..42b7f87ef2cd 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;
@@ -1388,26 +1388,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;
 	}
 
@@ -1675,6 +1669,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 */
@@ -1692,8 +1687,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);
 
@@ -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");
+	} else {
+		sts = applespi_register_touchpad_device(applespi,
+							&applespi->rcvd_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
@@ -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);
+
+	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 v2 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 42b7f87ef2cd..95a8f790eaff 100644
--- a/drivers/input/keyboard/applespi.c
+++ b/drivers/input/keyboard/applespi.c
@@ -1027,12 +1027,13 @@ 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;
 
 	file->private_data = applespi;
 
 	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);
@@ -2015,6 +2016,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 v2 5/5] Input: applespi - fix use-after-free in applespi_remove()

[Shih-Yuan Lee]

applespi_remove() called applespi_drain_writes() to wait for in-flight
write transfers, then immediately called acpi_disable_gpe() and
acpi_remove_gpe_handler().  However it then called applespi_drain_reads()
*after* the GPE handler was removed, which races with any read SPI
completion callback that could still reference the applespi struct
already being torn down.

Moreover, the two drain helpers use separate wait paths that can miss
each other: a read completion arriving just after drain_writes() returns
but before drain_reads() is called will set read_active, and the
subsequent drain_reads() will then wait on a wait_queue that nobody will
ever wake because the GPE is already gone.

Fix by replacing the two separate drain calls with a single barrier
using the existing cancel_spi + wait_event_lock_irq mechanism:

  - Set cancel_spi = true under the spinlock so that applespi_async()
    immediately rejects new SPI submissions and wakes the wait queue
    once all outstanding operations have drained.
  - Wait for !applespi_async_outstanding() before proceeding with
    teardown.
  - Disable the GPE and remove its handler only after all in-flight SPI
    transfers have completed, eliminating the use-after-free window.

Fixes: 0b7a8ac72fc1 ("Input: applespi - add driver for Apple SPI keyboard and touchpad")
Signed-off-by: Shih-Yuan Lee <fourdollars@debian.org>
---
 drivers/input/keyboard/applespi.c | 13 ++++++++++---
 1 file changed, 10 insertions(+), 3 deletions(-)

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