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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Multiple processing of `PACKET_DEV_INFO` causes the driver to unconditionally allocate and register multiple touchpad devices, leaking previously allocated devm resources.
- [High] Potential use-after-free of `applespi->gpe` in `applespi_async_read_complete()` due to premature driver unbind.
- [High] Dereferencing `applespi->touchpad_input_dev` before it is initialized in `applespi_tp_dim_open`.
--

--- Patch [1]: [PATCH v4 1/3] Input: applespi - cancel pending work on driver remove ---
commit 89c8a3961b949e3c0049b442d0905f82cd245fbd
Author: Shih-Yuan Lee <fourdollars@debian.org>

Input: applespi - cancel pending work on driver remove
    
This commit fixes a use-after-free vulnerability during driver removal.
It explicitly cancels the pending worker thread that registers the touchpad
device before devres frees the private data structure.

> diff --git a/drivers/input/keyboard/applespi.c b/drivers/input/keyboard/applespi.c
> index b5ff71cd5a701..3bdb9e7cfb8b2 100644
> --- a/drivers/input/keyboard/applespi.c
> +++ b/drivers/input/keyboard/applespi.c
> @@ -1822,6 +1822,8 @@ static void applespi_remove(struct spi_device *spi)
>  
>  	applespi_drain_reads(applespi);

[Severity: High]
This is a pre-existing issue, but can applespi_drain_reads() lead to a
use-after-free of applespi->gpe?

In applespi_remove(), applespi_drain_reads() waits for the read to complete:

static void applespi_drain_reads(struct applespi_data *applespi)
{
	...
	wait_event_lock_irq(applespi->drain_complete, !applespi->read_active,
			    applespi->cmd_msg_lock);
	...
}

The async read callback signals completion and wakes the waitqueue:

static void applespi_async_read_complete(void *context)
{
	...
	applespi_msg_complete(applespi, true, true);
	...
	acpi_finish_gpe(NULL, applespi->gpe);
}

If applespi_remove() unblocks, completes, and triggers devres cleanup
before the read callback finishes, could the callback dereference the freed
applespi->gpe when calling acpi_finish_gpe()?

> +	cancel_work_sync(&applespi->work);
> +
>  	debugfs_remove_recursive(applespi->debugfs_root);
>  }

[Severity: High]
This is a pre-existing issue, but does the driver leak devm resources if
multiple info packets are received?

When the driver receives a PACKET_DEV_INFO packet:

applespi_handle_cmd_response()
    applespi->rcvd_tp_info = message->tp_info;
    schedule_work(&applespi->work);

The worker unconditionally allocates and registers the touchpad device:

applespi_register_touchpad_device()
    touchpad_input_dev = devm_input_allocate_device(&applespi->spi->dev);
    ...
    sts = input_register_device(touchpad_input_dev);
    ...
    smp_store_release(&applespi->touchpad_input_dev, touchpad_input_dev);

If a malfunctioning or malicious device sends multiple PACKET_DEV_INFO
messages, does this overwrite applespi->touchpad_input_dev and leak the
previously allocated devm resources and registered input devices?

[Severity: High]
This is a pre-existing issue, but can the debugfs tp_dim file dereference a
NULL pointer?

The touchpad_input_dev is allocated asynchronously by the worker:

applespi_worker()
    applespi_register_touchpad_device()

However, the debugfs entry is created synchronously during probe:

applespi_probe()
    debugfs_create_file("tp_dim", ...);

If userspace opens the tp_dim file before the worker completes initialization,
applespi_tp_dim_open() will unconditionally dereference the pointer:

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,

Could this lead to a kernel panic since applespi->touchpad_input_dev might
still be NULL?

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