[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

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] Ignoring the return value of `wait_event_lock_irq_timeout()` allows driver unbinding to complete while asynchronous SPI transfers are still in flight, leading to Use-After-Free and DMA memory corruption.
--

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

Input: applespi - use unified wait queue with timeouts for drain

This commit consolidates the wait queues by renaming drain_complete to
wait_queue and replaces wait_event_lock_irq() with
wait_event_lock_irq_timeout() using a 3-second timeout during read and
write drains. This aims to prevent indefinite lockups during driver unbind
or PM transitions if the hardware is unresponsive.

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

[ ... ]

> @@ -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));

[Severity: High]
If this timeout expires (returning 0), the function will return while
applespi->write_active is still true. 

During driver unbind, applespi_remove() calls applespi_drain_writes(). If
it returns early due to a timeout, applespi_remove() will complete, and
devres will free the devm-managed applespi_data structure and DMA buffers
like tx_buffer and rx_buffer.

Won't the SPI controller continue performing DMA into this freed memory?

Additionally, when the SPI transfer eventually completes, the SPI core
invokes the completion callback which dereferences the freed context:

applespi_async_write_complete(void *context) {
        struct applespi_data *applespi = context;
        enum applespi_evt_type evt_type = applespi->cmd_evt_type;
        ...
        applespi_get_trace_fun(evt_type)(evt_type, PT_WRITE,
                                         applespi->tx_buffer,
                                         APPLESPI_PACKET_SIZE);
        ...
}

Could this lead to a use-after-free and DMA memory corruption? Should the
timeout return value be checked and handled, perhaps by canceling in-flight
SPI messages before returning?

> +
> +	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));

[Severity: High]
Similar to the drain_writes case above, if this wait times out, the function
returns while applespi->read_active is still true.

Does this expose the same use-after-free and memory corruption risks during
unbind if devm-managed structures are freed while reads are still in flight?

>  
>  	applespi->suspended = true;
> +
> +	spin_unlock_irqrestore(&applespi->cmd_msg_lock, flags);
>  }

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

[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

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

[sashiko-bot]

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

New issues:
- [High] The newly introduced asynchronous SPI transfer tracking logic and `cancel_spi` flag are dead code, failing to prevent use-after-free during teardown.
- [High] The asynchronous tracking mechanism signals completion to potential waiters before the inner callback finishes executing, introducing a Use-After-Free window.

Pre-existing issues:
- [High] The driver schedules `applespi->work` but fails to cancel it during driver removal, leading to a use-after-free.
- [High] Re-queueing the statically allocated `wr_m` SPI message after a timeout corrupts the SPI subsystem's message queue.
- [High] A pre-existing use-after-free vulnerability exists in `applespi_async_read_complete()` during device removal.
--

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

Input: applespi - track asynchronous SPI transfers in flight

This patch aims to fix use-after-free conditions during driver teardown by
tracking asynchronous SPI requests. It introduces a completion tracking array
and a cancel_spi flag to represent concurrent read and write transfers.

> diff --git a/drivers/input/keyboard/applespi.c b/drivers/input/keyboard/applespi.c
> index 64bbeba85ea9e..a8f8d5370e95a 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;
> +

[Severity: High]
Is cancel_spi ever initialized or set to true during driver teardown?

Looking through the patch and the driver, it appears cancel_spi is declared
but never set to true. As a result, the checks for applespi->cancel_spi in
applespi_async() and applespi_async_complete() will always evaluate to false,
leaving the new tracking mechanism inactive.

Additionally, it doesn't look like applespi_remove() was updated to wait for
!applespi_async_outstanding(applespi) during driver removal. Does the
teardown path still proceed without waiting for the tracked transfers to
finish?

[ ... ]

> +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);
> +}

[Severity: High]
Could waking up the wait queue before invoking the inner completion callback
introduce a use-after-free?

If a teardown thread is waiting on the wait_queue, it will unblock as soon
as wake_up_all() is called. This thread might then finish driver removal and
allow the devm subsystem to free the applespi structure.

If that happens, the subsequent call to complete(applespi) will dereference
the freed applespi object. Should the inner completion be invoked before
waking up the wait queue?


[Severity: High]
This is a pre-existing issue, but does driver removal fail to cancel the
scheduled workqueue?

Looking at applespi_remove():

applespi_remove() {
        applespi_drain_writes(applespi);
        applespi_drain_reads(applespi);
        ...
}

Since applespi is managed by devm_kzalloc(), its memory is freed
automatically shortly after applespi_remove() returns. If the touchpad
initialization worker scheduled in applespi->work runs after driver removal,
applespi_worker() will dereference the freed applespi structure.

Should cancel_work_sync(&applespi->work) be called during teardown?


[Severity: High]
This is a pre-existing issue, but can the command timeout logic corrupt the
SPI message queue?

In applespi_send_cmd_msg():

applespi_send_cmd_msg() {
        if (applespi->cmd_msg_queued) {
                if (ktime_ms_delta(ktime_get(), applespi->cmd_msg_queued) < 1000)
                        return 0;
                applespi->cmd_msg_queued = 0;
                applespi->write_active = false;
        }
        ...
        sts = applespi_async(applespi, &applespi->wr_m, applespi_async_write_complete);
}

If a command times out, the driver assumes the transfer is no longer in
flight and clears its internal flags. However, the message might still be
pending in the SPI controller's active queue.

Re-queueing the statically allocated applespi->wr_m struct via spi_async()
while it is still linked in the SPI core's lists will invoke list_add_tail()
on an already linked node, which corrupts the list.


[Severity: High]
This is a pre-existing issue, but is there a use-after-free window in the
read completion handler during teardown?

During driver removal, applespi_drain_reads() waits for read_active to
become false:

applespi_drain_reads() {
        wait_event_lock_irq_timeout(applespi->wait_queue,
                                    !applespi->read_active, ...);
}

Inside applespi_async_read_complete():

applespi_async_read_complete() {
        ...
        applespi_got_data(applespi);
        ...
        acpi_finish_gpe(NULL, applespi->gpe);
}

The call to applespi_got_data() clears read_active, which wakes up
applespi_drain_reads(). This allows the driver removal thread to finish
and free the applespi object. When the completion handler resumes, it
calls acpi_finish_gpe(NULL, applespi->gpe), dereferencing the freed memory.

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

[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

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

[sashiko-bot]

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

New issues:
- [Critical] A race condition in the SPI completion handler causes a Use-After-Free of `applespi` when the probe thread fails.
- [Critical] Ignoring the timeout in `applespi_drain_writes` and `applespi_drain_reads` allows the driver to unbind while SPI DMA transfers are active.
- [High] Reading the `tp_dim` debugfs file triggers a NULL pointer dereference if the touchpad info packet times out during probe.
- [Medium] Data Race when reading `rcvd_tp_info` without `cmd_msg_lock` in probe.

Pre-existing issues:
- [High] Failure to call `acpi_finish_gpe()` if `applespi_async()` fails in the GPE interrupt handler.
--

--- Patch [3]: [PATCH v2 3/5] Input: applespi - register touchpad synchronously in probe ---
commit d8377d333c1aa1ce257aedd6f29080dfb8f5320b
Author: Shih-Yuan Lee <fourdollars@debian.org>

Input: applespi - register touchpad synchronously in probe

This commit removes the asynchronous worker for touchpad registration.
It replaces it with synchronous registration during driver probe, waiting up
to 3 seconds for the device info packet and falling back to keyboard-only
mode on timeout.

> 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

[ ... ]

> @@ -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");

[Severity: High]
If this timeout occurs, applespi->touchpad_input_dev remains null. 
Will this cause a kernel panic later?

When a user reads the debugfs file, applespi_tp_dim_open() unconditionally
dereferences it:

    snprintf(applespi->tp_dim_val, sizeof(applespi->tp_dim_val),
             "0x%.4x %dx%d+%u+%u\n",
             applespi->touchpad_input_dev->id.product,
             ...

> +	} else {
> +		sts = applespi_register_touchpad_device(applespi,
> +							&applespi->rcvd_tp_info);

[Severity: Medium]
Is there a data race when reading rcvd_tp_info here?

The commit message mentions protecting rcvd_tp_info under cmd_msg_lock,
but applespi_register_touchpad_device() extracts the model info from it
without holding the lock. If the device sends another unexpected touchpad
info packet, could the SPI callback overwrite rcvd_tp_info concurrently?

> +		if (sts)
> +			goto cancel_spi;
> +	}
> +

[ ... ]

> @@ -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);

[Severity: High]
This is a pre-existing issue, but does applespi_notify() have a bug in its
error handling path that could cause a permanent GPE stall?

If applespi_async() fails in applespi_notify() (e.g. transient out of
memory), it returns ACPI_INTERRUPT_HANDLED but misses queuing the async
operation:

    sts = applespi_async(applespi, &applespi->rd_m,
                         applespi_async_read_complete);
    if (sts)
        dev_warn(&applespi->spi->dev,
                 "Error queueing async read to device: %d\n", sts);

Because the async task was not queued, applespi_async_read_complete() is
never called. This means acpi_finish_gpe() is never invoked, permanently
leaving the level-triggered GPE unfinished and breaking all input.

> +
> +	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;

[Severity: Critical]
Does this error path introduce a use-after-free for applespi?

When wait_event_lock_irq completes, this probe thread returns an error
and devres frees the applespi structure. However, in
applespi_async_complete():

    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);

The completion thread wakes the wait queue and drops the lock. This allows
the probe thread to resume, return, and trigger the devres free operation
before the completion thread executes complete(applespi), leading to a
use-after-free.

>  }
>  
>  static void applespi_drain_writes(struct applespi_data *applespi)

[Severity: Critical]
I noticed a regression here. Does applespi_drain_writes() allow the driver
to unbind while SPI DMA transfers are still active?

It uses a timeout when waiting for writes to finish:

    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);

Since the return value of wait_event_lock_irq_timeout() is ignored, if the
SPI hardware is delayed and the timeout expires, this function silently
returns as if the drain succeeded. When applespi_remove() completes,
devres frees the memory. Once the hardware finally processes the transfer,
it will DMA to/from freed memory and execute the completion callback.

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

[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

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

[sashiko-bot]

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

New issues:
- [High] Failure to cancel in-flight SPI async transfers upon unbind timeout results in use-after-free in completion callbacks.

Pre-existing issues:
- [High] Kernel panic in `applespi_tp_dim_open` due to an unchecked NULL `touchpad_input_dev` when touchpad initialization times out.
--

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

Input: applespi - prefer asynchronous driver probing
    
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.

> 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
> @@ -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,

[Severity: High]
This isn't a bug introduced by this patch, but can this dereference a NULL
pointer if the touchpad fails to initialize?

If the 3-second wait in applespi_probe() times out:

applespi_probe() {
    ...
    sts = wait_event_timeout(applespi->wait_queue,
                             READ_ONCE(applespi->have_tp_info),
                             msecs_to_jiffies(3000));
    ...
}

Then applespi->touchpad_input_dev is never allocated. However, the
debugfs file is still created unconditionally later in applespi_probe():

debugfs_create_file("tp_dim", 0400, applespi->debugfs_root, applespi,
                    &applespi_tp_dim_fops);

If a user subsequently reads this file, applespi_tp_dim_open() is called
and will dereference the unconditionally NULL touchpad pointer when
retrieving the product ID.

>  		 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,

[Severity: High]
Does the timeout added in this series for unbinding lead to a use-after-free
if SPI transfers are still active?

In applespi_remove(), the driver attempts to drain in-flight operations:

applespi_remove() {
    applespi_drain_writes(applespi);
    acpi_disable_gpe(NULL, applespi->gpe);
    ...
    applespi_drain_reads(applespi);
    ...
}

These drain functions wait with a 3-second timeout:

applespi_drain_writes() {
    wait_event_lock_irq_timeout(applespi->wait_queue,
                                !applespi->write_active,
                                applespi->cmd_msg_lock,
                                msecs_to_jiffies(3000));
}

If this timeout expires because the hardware is unresponsive, the function
returns and applespi_remove() completes. The devres subsystem then frees
the applespi structure and its DMA-mapped buffers.

When the asynchronous SPI transfer eventually completes, the SPI core invokes
applespi_async_complete() with the freed context pointer:

applespi_async_complete() {
    struct applespi_complete_info *info = context;
    struct applespi_data *applespi = info->applespi;
    ...
    spin_lock_irqsave(&applespi->cmd_msg_lock, flags);
    ...
}

Will this dereference the freed applespi object and corrupt memory?

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

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

[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

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

[sashiko-bot]

Thank you for your contribution! Sashiko AI review found 3 potential issue(s) to consider:
- [High] applespi_async_complete() triggers a use-after-free by signaling completion before actually executing the callback.
- [High] Ignored timeout in `wait_event_lock_irq_timeout` leads to a Use-After-Free on outstanding SPI transfers.
- [High] Setting `cancel_spi = true` before disabling the level-triggered GPE causes an interrupt storm.
--

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

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

This commit replaces the separate read and write drain helpers with a single
barrier using the existing cancel_spi and wait_event_lock_irq mechanism. It
waits for outstanding asynchronous operations to complete before disabling
the GPE and tearing down the device.

> diff --git a/drivers/input/keyboard/applespi.c b/drivers/input/keyboard/applespi.c
> index 95a8f790eaffc..088337f060b24 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;

[Severity: High]
If we set cancel_spi to true while the level-triggered GPE is still enabled,
could this cause an interrupt storm?

When the GPE fires, applespi_notify() attempts to queue a read via
applespi_async(), which immediately rejects it with -ESHUTDOWN. Because the
read is rejected, the trackpad hardware never deasserts the interrupt line.

Would it be safer to disable the GPE before setting this flag?

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

[Severity: High]
If the wait times out (returning 0), the driver continues tearing down and
returns, allowing devm to free the memory. If the delayed SPI transfer
completes shortly after, won't that result in a use-after-free in the SPI
core or applespi_async_complete() when it accesses the freed driver data?

[Severity: High]
Does this wait condition race with the inner completion callback?

Looking at applespi_async_complete(), it signals completion before executing
the inner callback:

drivers/input/keyboard/applespi.c:applespi_async_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);
    ...
}

If the teardown thread wakes up as soon as applespi_async_outstanding() is
false and frees the device structure, will complete(applespi) dereference
freed memory?

> +	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);
>  }

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