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

[sashiko-bot@kernel.org]

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

New issues:
- [High] The client driver improperly modifies the shared SPI host controller's `can_dma` callback, causing an execute-after-free vulnerability upon driver unbind or probe failure.

Pre-existing issues:
- [High] Missing `cancel_work_sync(&applespi->work)` during driver removal leads to a Use-After-Free of the managed private data structure.
- [High] The `tp_dim` debugfs file can be opened by userspace before the touchpad input device is fully initialized, causing a NULL pointer dereference.
--

--- Patch [1]: [PATCH 1/1] Input: applespi - force PIO mode on MacBook8,1 ---
commit e15d04076e4b2b62df355b30d858accad86794ac
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 after warm
reboots. Address this by introducing a DMI quirk inside the probe function
that detects the MacBook8,1 model and overrides the controller's can_dma
callback to a custom helper that always returns false.

> diff --git a/drivers/input/keyboard/applespi.c b/drivers/input/keyboard/applespi.c
> index b5ff71cd5a701..edf81e3836122 100644
> --- a/drivers/input/keyboard/applespi.c
> +++ b/drivers/input/keyboard/applespi.c
> @@ -1605,6 +1606,13 @@ static void applespi_save_bl_level(struct applespi_data *applespi,

[ ... ]

> +static bool applespi_can_not_dma(struct spi_controller *controller,
> +				 struct spi_device *spi,
> +				 struct spi_transfer *xfer)
> +{
> +	return false;
> +}
> +
>  static int applespi_probe(struct spi_device *spi)
>  {
>  	struct applespi_data *applespi;
> @@ -1613,6 +1621,15 @@ static int applespi_probe(struct spi_device *spi)
>  	int sts, i;
>  	unsigned long long gpe, usb_status;
>  
> +	/*
> +	 * MacBook8,1's SPI host controller DMA is broken (timeout errors).
> +	 * Force PIO mode by overriding the controller's can_dma callback.
> +	 */
> +	if (dmi_match(DMI_PRODUCT_NAME, "MacBook8,1")) {
> +		dev_info(&spi->dev, "Disabling DMA for MacBook8,1 SPI to force PIO mode\n");
> +		spi->controller->can_dma = applespi_can_not_dma;

[Severity: High]
Could this overwrite of the shared SPI host controller's can_dma callback
cause a regression by leaving a dangling pointer if the applespi module is
unloaded?

Since applespi_can_not_dma() is a local function inside this driver, won't
subsequent SPI transfers by other devices on this bus attempt to execute
unmapped memory after this module is removed?

> +	}
> +

This isn't a bug introduced by this patch, but looking at applespi_remove():

drivers/input/keyboard/applespi.c:applespi_remove() {
    ...
    applespi_drain_reads(applespi);

    debugfs_remove_recursive(applespi->debugfs_root);
}

[Severity: High]
Does this teardown path miss a call to cancel_work_sync(&applespi->work)?

Because devres will free the applespi structure when applespi_remove()
returns, could an already pending or running applespi_worker cause a
use-after-free here?

This is also a pre-existing issue, but looking at applespi_tp_dim_open():

drivers/input/keyboard/applespi.c:applespi_tp_dim_open() {
    ...
    snprintf(applespi->tp_dim_val, sizeof(applespi->tp_dim_val),
             "0x%.4x %dx%d+%u+%u\n",
             applespi->touchpad_input_dev->id.product,
    ...
}

[Severity: High]
Can userspace open the tp_dim debugfs file before the asynchronous worker
has finished initializing applespi->touchpad_input_dev?

Since the debugfs file is registered synchronously during applespi_probe(),
could an early read trigger a NULL pointer dereference here?

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