Re: [PATCH v3 2/3] Input: applespi - cancel pending work on driver remove

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Use-After-Free of `applespi` structure in SPI async read completion callback
- [High] Unbounded wait in `applespi_drain_writes` leading to deadlock on driver unbind or suspend
- [High] NULL pointer dereference in debugfs 'tp_dim' open handler
- [High] CRC error in SPI packet permanently leaks `read_active` flag, causing deadlock
- [High] Unsafe DMA allocation using `devm_kmalloc` leads to cacheline corruption
--

--- Patch [2]: [PATCH v3 2/3] Input: applespi - cancel pending work on driver remove ---
commit 600aa576cb85623de00e20bade1177db921cccac
Author: Shih-Yuan Lee <fourdollars@debian.org>

Input: applespi - cancel pending work on driver remove
    
During driver removal in applespi_remove(), the managed private data
structure is freed by devres. However, the driver does not cancel the
asynchronous work applespi->work, which registers the touchpad input
device.

> 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
> @@ -1861,6 +1861,8 @@ static void applespi_remove(struct spi_device *spi)
>  
>  	applespi_drain_reads(applespi);

[Severity: High]
This is a pre-existing issue, but does applespi_drain_reads() create a race
condition with the SPI async read completion callback?

If applespi_drain_reads() waits for read_active to become false, and the
callback applespi_async_read_complete() clears this flag via
applespi_msg_complete(), it unblocks the removal thread immediately:

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

Can the removal thread finish applespi_remove() and free the applespi
structure via devres before the completion callback finishes executing,
leading to a use-after-free when acpi_finish_gpe() is called?

[Severity: High]
This is a pre-existing issue, but can this unbind or suspend path deadlock
due to unbounded waits?

Looking at applespi_drain_writes() called earlier in this path, it waits
for write_active to become false:

applespi_drain_writes() {
    applespi->drain = true;
    wait_event_lock_irq(applespi->drain_complete, !applespi->write_active,
                        applespi->cmd_msg_lock);
}

If the hardware fails to respond to a command, write_active remains true.
Because drain is set to true, the lazy timeout logic in
applespi_send_cmd_msg() is unreachable. Does this wait_event block
indefinitely without a fallback timeout if a write response is never received?

> +	cancel_work_sync(&applespi->work);
> +
>  	if (override_dma)
>  		spi->controller->can_dma = applespi->original_can_dma;
>  

[ ... ]

[Severity: High]
This is a pre-existing issue, but is there a risk of a NULL pointer
dereference when accessing the tp_dim debugfs file?

The tp_dim debugfs file is created synchronously during probe, but
applespi->touchpad_input_dev is allocated asynchronously in a workqueue.
If a user opens the debugfs file before the workqueue executes (or if it
never executes due to a hardware failure), applespi_tp_dim_open() will see
a NULL pointer:

applespi_tp_dim_open() {
    ...
    applespi->touchpad_input_dev->id.product,
    ...
}

Can this unconditionally dereference touchpad_input_dev while it is NULL?

[Severity: High]
This is a pre-existing issue, but does a CRC error permanently leak the
read_active state?

In applespi_got_data(), if a packet fails CRC validation and drain is
false, it returns early:

applespi_got_data() {
    if (!applespi_verify_crc(...)) {
        if (applespi->drain) {
            ...
        }
        return;
    }
    ...
}

This skips calling applespi_msg_complete(), leaving read_active set to
true. When the system later suspends or the driver is unbound, will
applespi_drain_reads() wait indefinitely for read_active to become
false, causing a deadlock?

[Severity: High]
This is a pre-existing issue, but are the SPI buffers safe for DMA transfers?

In applespi_probe(), the buffers are allocated using devm_kmalloc():

applespi_probe() {
    ...
    applespi->tx_buffer = devm_kmalloc(&spi->dev, APPLESPI_PACKET_SIZE, ...);
    applespi->rx_buffer = devm_kmalloc(&spi->dev, APPLESPI_PACKET_SIZE, ...);
    ...
}

Since devm allocations prepend a struct devres header immediately before
the returned pointer, the devres header shares the same cacheline as the
start of the buffer.

When the SPI core maps these buffers for DMA, will DMA cache invalidation
(especially DMA_FROM_DEVICE for Rx) invalidate the CPU cacheline and
corrupt the devres linked list headers?

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