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

[sashiko-bot@kernel.org]

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

New issues:
- [High] Teardown threads wake up prematurely and free `applespi` while `applespi_async_complete` is still running, causing a Use-After-Free.
- [High] Consecutive SPI transfers fail with -EBUSY due to occupied tracking slots, dropping commands and stalling the driver.
- [High] The driver becomes completely broken after a suspend/resume cycle because `cancel_spi` is never reset.

Pre-existing issues:
- [High] Missing cancellation of `applespi->work` in `applespi_remove()` leads to a Use-After-Free.
--

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

Input: applespi - track asynchronous SPI transfers in flight

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. This commit introduces a tracking mechanism
for these async transfers using a spi_complete slot array.

> diff --git a/drivers/input/keyboard/applespi.c b/drivers/input/keyboard/applespi.c
> index c501bf7e517e4..c9bbceaf06718 100644
> --- a/drivers/input/keyboard/applespi.c
> +++ b/drivers/input/keyboard/applespi.c

[ ... ]

> @@ -607,13 +613,73 @@ 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;
> +	spin_unlock_irqrestore(&applespi->cmd_msg_lock, flags);
> +
> +	if (complete)
> +		complete(applespi);

[Severity: High]
If the inner complete callback queues a new write command, will
applespi_async() see the first slot as still occupied because info->complete
has not been cleared yet?

If a read transfer happens to be active in the other slot, could this cause
applespi_async() to return -EBUSY and drop the new command, permanently
stalling driver initialization?

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

[Severity: High]
Can this sequence cause a use-after-free regression?

If applespi_drain_writes() is waiting for write_active to become false:

CPU1 running applespi_async_complete():
    complete(applespi);

The inner callback sets write_active = false and wakes wait_queue.

CPU2 running applespi_drain_writes():
    wakes up, finishes applespi_remove(), and frees the applespi structure

Could CPU1 then try to acquire the spinlock and clear info->complete on the
freed applespi structure?

[ ... ]

> @@ -1799,6 +1865,7 @@ static void applespi_drain_writes(struct applespi_data *applespi)
>  	spin_lock_irqsave(&applespi->cmd_msg_lock, flags);
>  
>  	applespi->drain = true;
> +	applespi->cancel_spi = true;
>  	ret = wait_event_lock_irq_timeout(applespi->wait_queue,
>  					  !applespi->write_active,
>  					  applespi->cmd_msg_lock,

[Severity: High]
If applespi_drain_writes() sets cancel_spi to true during applespi_suspend(),
is it ever reset to false in applespi_resume()?

Could this cause applespi_async() to permanently reject new transfers with
-ESHUTDOWN after the system resumes?

[Severity: High]
This isn't a bug introduced by this patch, but while looking at the driver
teardown path, does applespi_remove() need to call
cancel_work_sync(&applespi->work)?

If applespi->work is queued to register the touchpad and the module is
unbound, could devres free the applespi structure before applespi_worker()
executes, resulting in a use-after-free regression?

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