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

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 [off-list ref]

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.

[ ... ]

[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