Re: [PATCH v3 1/3] Input: applespi - force PIO mode on MacBook8,1

[sashiko-bot@kernel.org]

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

New issues:
- [High] The child device driver (`applespi`) overwrites the parent SPI controller's `can_dma` callback, causing an architectural layering violation, data races (TOCTOU) with concurrent SPI transfers, and potential execute-after-free vulnerabilities upon module unload.

Pre-existing issues:
- [High] Race condition between manual cleanup and devres unregistration leads to a Use-After-Free via `applespi->work`.
--

--- Patch [1]: [PATCH v3 1/3] Input: applespi - force PIO mode on MacBook8,1 ---
commit fb42db4427f33df07132a7b2bd913031f55465ea
Author: Shih-Yuan Lee <fourdollars@debian.org>

Input: applespi - force PIO mode on MacBook8,1

On MacBook8,1 (early 2015 12" MacBook), the LPSS SPI controller's DMA
handshake and interrupt routing frequently fail or time out on boot,
causing the keyboard and trackpad to become unresponsive.

Link: https://bugzilla.kernel.org/show_bug.cgi?id=108331

> 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
[ ... ]
> @@ -1628,6 +1645,13 @@ static int applespi_probe(struct spi_device *spi)
>  
>  	applespi->spi = spi;
>  
> +	/* Save original can_dma and override if requested or needed */
> +	applespi->original_can_dma = spi->controller->can_dma;
> +	if (override_dma) {
> +		dev_info(&spi->dev, "Disabling DMA for SPI to force PIO mode\n");
> +		spi->controller->can_dma = applespi_can_not_dma;

[Severity: High]
Is it safe for a child device driver to overwrite the parent SPI controller's
shared can_dma callback?

Modifying spi->controller->can_dma directly without a controller-wide lock
could cause a data race if the SPI worker thread concurrently dereferences
ctlr->can_dma for another device on the same bus.

> +	}
> +
>  	INIT_WORK(&applespi->work, applespi_worker);
[ ... ]
> @@ -1822,6 +1861,9 @@ static void applespi_remove(struct spi_device *spi)
>  
>  	applespi_drain_reads(applespi);
>  
>  	cancel_work_sync(&applespi->work);

[Severity: High]
This is a pre-existing issue, but does this create a use-after-free window
during module unload?

In applespi_remove(), the work is canceled, but the input and LED devices
are managed by devm_ and aren't unregistered until the devres release phase
after applespi_remove() completes. 

During this window, if a user writes to the LED sysfs interface, it will
call applespi_set_bl_level() which calls schedule_work(&applespi->work),
re-arming the worker. When devres subsequently frees the applespi structure,
the queued work might execute on freed memory.

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

[Severity: High]
Could restoring the can_dma callback without synchronization leave a dangling
pointer if multiple instances bind or unbind?

The restore happens conditionally based on a dynamically recalculated
override_dma boolean. If the module is unloaded but fails to restore the
pointer properly, a dangling pointer will remain in the shared SPI controller.
The SPI core could then invoke the freed applespi_can_not_dma function inside
the unloaded module, resulting in an execute-after-free.

> +
>  	debugfs_remove_recursive(applespi->debugfs_root);
>  }

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