[PATCH 0/2] Input: applespi - probe and remove fixes

[PATCH 0/2] Input: applespi - probe and remove fixes

[Shih-Yuan Lee]

This series fixes two bugs in the applespi driver found during work to
restore MacBook8,1 SPI keyboard/touchpad functionality across S3
suspend/resume cycles.

Patch 1 fixes a probe-time regression introduced when the touchpad
registration was made asynchronous: a deferred workqueue task could
complete after the spi_device was already removed, causing a NULL pointer
dereference.  The fix registers the touchpad input device synchronously
in probe and removes the deferred path.

Patch 2 fixes a use-after-free race in applespi_remove().  The original
code called applespi_drain_writes(), then removed the GPE handler, then
called applespi_drain_reads() -- but any read completion arriving after
GPE removal would reference the already-torn-down applespi struct.  The
fix uses the existing cancel_spi + wait_event_lock_irq mechanism to
drain all in-flight SPI transfers atomically before proceeding with
teardown.

These patches are independent of the companion spi-pxa2xx series also
submitted today (MacBook8,1 DMA quirk and LPSS S3 resume fix).

Shih-Yuan Lee (2):
  Input: applespi - register touchpad device synchronously in probe
  Input: applespi - fix use-after-free in applespi_remove()

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

-- 
2.39.5

[PATCH 1/2] Input: applespi - register touchpad device synchronously in probe

[Shih-Yuan Lee]

Touchpad registration is currently done asynchronously from a worker
thread because it can sleep. This leads to high-severity unbind/remove
use-after-free vulnerabilities and debugfs race conditions (userspace
opening tp_dim debugfs file before touchpad_input_dev is initialized).

Simplify the driver and make it robust by making the touchpad detection and
registration synchronous in the probe function. Introduce a timeout of
3 seconds when waiting for the touchpad info response packet, and add
3 second timeouts to the drain functions to prevent suspend/remove hangs.
Define probe_type as PROBE_PREFER_ASYNCHRONOUS so that the probe is run
asynchronously by the driver core, preventing boot delays.

This obsoletes the need for async work cancellation on driver remove
and debugfs NULL pointer checks.

Suggested-by: Dmitry Torokhov <dmitry.torokhov@gmail.com>
Based-on-patch-by: Ronald Tschalär <ronald@innovation.ch>
Signed-off-by: Shih-Yuan Lee <fourdollars@debian.org>
---
 drivers/input/keyboard/applespi.c | 148 ++++++++++++++++++++++++------
 1 file changed, 118 insertions(+), 30 deletions(-)

diff --git a/drivers/input/keyboard/applespi.c b/drivers/input/keyboard/applespi.c
index b5ff71cd5a70..c1065a6ba96c 100644
--- a/drivers/input/keyboard/applespi.c
+++ b/drivers/input/keyboard/applespi.c
@@ -417,11 +417,16 @@ 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;
 
-	struct work_struct		work;
+	struct applespi_complete_info {
+		void				(*complete)(void *context);
+		struct applespi_data		*applespi;
+	}				spi_complete[2];
+	bool				cancel_spi;
+
 	struct touchpad_info_protocol	rcvd_tp_info;
 
 	struct dentry			*debugfs_root;
@@ -607,13 +612,61 @@ 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;
+	unsigned long flags;
+
+	info->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;
 
-	return spi_async(applespi->spi, message);
+	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
+		info = &applespi->spi_complete[1];
+	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 sts;
 }
 
 static inline bool applespi_check_write_status(struct applespi_data *applespi,
@@ -677,7 +730,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 +778,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;
@@ -963,12 +1016,18 @@ 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;
 
 	file->private_data = applespi;
 
+	/* Pairs with smp_store_release in applespi_register_touchpad_device() */
+	touchpad = smp_load_acquire(&applespi->touchpad_input_dev);
+	if (!touchpad)
+		return -ENODEV;
+
 	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);
@@ -1324,26 +1383,14 @@ 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)
 {
 	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.
-		 */
 		applespi->rcvd_tp_info = message->tp_info;
-		schedule_work(&applespi->work);
+		wake_up_all(&applespi->wait_queue);
 		return;
 	}
 
@@ -1415,7 +1462,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;
@@ -1610,6 +1657,7 @@ static int applespi_probe(struct spi_device *spi)
 	struct applespi_data *applespi;
 	acpi_handle spi_handle = ACPI_HANDLE(&spi->dev);
 	acpi_status acpi_sts;
+	unsigned long flags;
 	int sts, i;
 	unsigned long long gpe, usb_status;
 
@@ -1628,8 +1676,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);
 
@@ -1757,6 +1803,22 @@ static int applespi_probe(struct spi_device *spi)
 	/* trigger touchpad setup */
 	applespi_init(applespi, false);
 
+	/* set up the touchpad as a separate input device */
+	sts = wait_event_timeout(applespi->wait_queue,
+				 applespi->rcvd_tp_info.model_no,
+				 msecs_to_jiffies(3000));
+	if (!sts) {
+		dev_err(&applespi->spi->dev,
+			"Timed out waiting for device info\n");
+		sts = -ETIMEDOUT;
+		goto cancel_spi;
+	}
+
+	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
@@ -1789,25 +1851,50 @@ 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)
 {
-	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;
 
-	wait_event_lock_irq(applespi->drain_complete, !applespi->read_active,
-			    applespi->cmd_msg_lock);
+	spin_lock_irqsave(&applespi->cmd_msg_lock, flags);
+
+	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)
@@ -1919,6 +2006,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 2/2] 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 | 12 +++++++++---
 1 file changed, 9 insertions(+), 3 deletions(-)

diff --git a/drivers/input/keyboard/applespi.c b/drivers/input/keyboard/applespi.c
index c1065a6ba96c..1f5cc2b4dc30 100644
--- a/drivers/input/keyboard/applespi.c
+++ b/drivers/input/keyboard/applespi.c
@@ -1900,15 +1900,21 @@ 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(applespi->wait_queue,
+			    !applespi_async_outstanding(applespi),
+			    applespi->cmd_msg_lock);
+	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

Re: [PATCH 1/2] Input: applespi - register touchpad device synchronously in probe

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [Critical] Re-entrancy in async SPI completion leads to callback slot overwrite and NULL pointer dereference.
- [High] Missing timeouts in probe error path and remove path cause indefinite system hang if the SPI controller wedges.
- [High] Synchronous touchpad detection failure forces a complete driver teardown, disabling the functional keyboard.
- [Medium] Lockless copy and evaluation of touchpad info struct causes a data race.
- [Medium] Timeout condition fails if the touchpad's model number is 0.
- [Low] Redundant and contradictory debugfs NULL pointer check.
--

--- Patch [1]: [PATCH 1/2] Input: applespi - register touchpad device synchronously in probe ---
commit 4961cde9c37a41e959c60a2aa6d8bde99a5e96b5
Author: Shih-Yuan Lee <fourdollars@debian.org>

Input: applespi - register touchpad device synchronously in probe

This patch simplifies the applespi driver by making touchpad detection and
registration synchronous during probe rather than using an asynchronous worker.
It also introduces timeouts for wait events and changes the probe type to prefer
asynchronous execution by the driver core.

> diff --git a/drivers/input/keyboard/applespi.c b/drivers/input/keyboard/applespi.c
> index b5ff71cd5a701..c1065a6ba96c8 100644
> --- a/drivers/input/keyboard/applespi.c
> +++ b/drivers/input/keyboard/applespi.c
[ ... ]
> @@ -607,13 +612,61 @@ 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;
> +	unsigned long flags;
> +
> +	info->complete(applespi);
> +
> +	spin_lock_irqsave(&applespi->cmd_msg_lock, flags);
> +
> +	info->complete = NULL;

[Severity: Critical]
Can this execution order cause a nested applespi_async() call to mistakenly
view the slot as occupied?

Because info->complete(applespi) executes before info->complete = NULL, if the
callback queues a new SPI message, it might see this slot as still in use.

> +
> +	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;
>  
> -	return spi_async(applespi->spi, message);
> +	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
> +		info = &applespi->spi_complete[1];
> +	info->complete = complete;

[Severity: Critical]
Does this blindly overwrite the second slot if the first is occupied, even if
the second slot is also in use?

If a re-entrant call happens before info->complete is cleared in
applespi_async_complete(), this might overwrite the callback of a concurrent
in-flight transfer. This could lead to a NULL pointer dereference or
use-after-free later.

> +	info->applespi = applespi;
> +
> +	message->complete = applespi_async_complete;
> +	message->context = info;
> +
> +	sts = spi_async(applespi->spi, message);
[ ... ]
> @@ -963,12 +1016,18 @@ 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;
>  
>  	file->private_data = applespi;
>  
> +	/* Pairs with smp_store_release in applespi_register_touchpad_device() */
> +	touchpad = smp_load_acquire(&applespi->touchpad_input_dev);
> +	if (!touchpad)
> +		return -ENODEV;
> +

[Severity: Low]
Is this NULL pointer check redundant?

The commit message states that the patch "obsoletes the need for async work
cancellation on driver remove and debugfs NULL pointer checks." However, this
adds a new NULL pointer check.

Since the probe is now synchronous, the debugfs file isn't created until the
touchpad is fully registered, making it impossible for touchpad_input_dev to be
NULL here.

>  	snprintf(applespi->tp_dim_val, sizeof(applespi->tp_dim_val),
>  		 "0x%.4x %dx%d+%u+%u\n",
[ ... ]
> @@ -1324,26 +1383,14 @@ applespi_register_touchpad_device(struct applespi_data *applespi,
>  	return 0;
>  }
>  
> -static void applespi_worker(struct work_struct *work)
> -{
[ ... ]
> -}
> -
>  static void applespi_handle_cmd_response(struct applespi_data *applespi,
>  					 struct spi_packet *packet,
>  					 struct message *message)
>  {
>  	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.
> -		 */
>  		applespi->rcvd_tp_info = message->tp_info;
> -		schedule_work(&applespi->work);
> +		wake_up_all(&applespi->wait_queue);

[Severity: Medium]
Is there a data race here since the 112-byte message->tp_info struct is copied
locklessly before waking the wait queue?

>  		return;
>  	}
[ ... ]
> @@ -1757,6 +1803,22 @@ static int applespi_probe(struct spi_device *spi)
>  	/* trigger touchpad setup */
>  	applespi_init(applespi, false);
>  
> +	/* set up the touchpad as a separate input device */
> +	sts = wait_event_timeout(applespi->wait_queue,
> +				 applespi->rcvd_tp_info.model_no,
> +				 msecs_to_jiffies(3000));

[Severity: Medium]
Because applespi->rcvd_tp_info.model_no is evaluated locklessly without an
acquire memory barrier, could the waiting thread observe a non-zero model_no
and wake up before preceding fields are fully committed to memory?

[Severity: Medium]
What happens if a legitimate touchpad reports a model_no of 0?

Since applespi->rcvd_tp_info.model_no is used as a boolean flag, a value of 0
would cause the wait condition to fail and trigger the 3-second timeout, even
though the hardware responded successfully.

> +	if (!sts) {
> +		dev_err(&applespi->spi->dev,
> +			"Timed out waiting for device info\n");
> +		sts = -ETIMEDOUT;
> +		goto cancel_spi;
> +	}

[Severity: High]
Does a touchpad detection timeout here completely abort the driver probe and
disable a fully functional keyboard?

In the previous asynchronous design, a missing info packet safely skipped
touchpad registration while leaving the keyboard intact.

> +
> +	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
> @@ -1789,25 +1851,50 @@ 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);

[Severity: High]
Does this probe error path lack a timeout?

The commit message mentions adding 3-second timeouts to prevent suspend/remove
hangs, but if the SPI controller wedges, this wait_event_lock_irq() could hang
indefinitely. This same issue appears to be present in applespi_remove().

> +	spin_unlock_irqrestore(&applespi->cmd_msg_lock, flags);
> +
> +	return sts;
>  }

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