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

[sashiko-bot@kernel.org]

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