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

[sashiko-bot@kernel.org]

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