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

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Use-after-free of the applespi structure inside the SPI asynchronous read completion callback during driver removal.
- [High] Deadlock/Hang on driver unbind or suspend due to un-cleared read_active flag on CRC error.
- [High] Missing payload length validation leads to reading uninitialized heap memory or old packet data.
- [High] Unbounded allocation and registration of touchpad input devices triggered by untrusted device input.
- [High] Use-after-free if probe fails after input device registration while async SPI writes are in-flight.
- [High] NULL pointer dereference when opening the tp_dim debugfs file before touchpad initialization.
- [Low] Redundant manual assignment of input_dev->dev.parent when using devm_input_allocate_device().
--

--- Patch [2]: [PATCH v2 2/3] Input: applespi - cancel pending work on driver remove ---
commit fee01b08b7f49cc284abe2da246718c6d357db23
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
> index 07a910cb8459f..b6b4d258d0dd5 100644
> --- a/drivers/input/keyboard/applespi.c
> +++ b/drivers/input/keyboard/applespi.c
> @@ -1847,6 +1847,8 @@ static void applespi_remove(struct spi_device *spi)
>  
>  	applespi_drain_reads(applespi);
>  

[Severity: High]
This isn't a bug introduced by this patch, but does the read completion callback
access the private data structure after it has been freed during driver removal?

applespi_drain_reads() unblocks when applespi_msg_complete() clears read_active:

applespi_msg_complete()
{
...
	if (is_read_compl)
		applespi->read_active = false;
...
	if (applespi->drain && !applespi->write_active)
		wake_up_all(&applespi->drain_complete);
}

However, applespi_msg_complete() is called by applespi_async_read_complete()
before it finishes using the applespi structure:

applespi_async_read_complete(void *context)
{
	struct applespi_data *applespi = context;
...
		applespi_msg_complete(applespi, true, true);
	} else {
		applespi_got_data(applespi);
	}

	acpi_finish_gpe(NULL, applespi->gpe);
}

Once wake_up_all() returns, applespi_remove() can finish, which allows devres
to free the applespi structure. Does the final acpi_finish_gpe() call then
dereference a freed applespi->gpe?


[Severity: High]
This is a pre-existing issue, but can the kernel hang indefinitely during
driver unbind if an SPI read packet fails the CRC check?

In applespi_got_data(), if the CRC check fails and drain is false, it returns
early without clearing read_active:

applespi_got_data()
{
...
	if (!applespi_verify_crc(applespi, applespi->rx_buffer,
				 APPLESPI_PACKET_SIZE)) {
		guard(spinlock_irqsave)(&applespi->cmd_msg_lock);

		if (applespi->drain) {
			applespi->read_active = false;
...
		}

		return;
	}

Since read_active remains true, when applespi_remove() later calls
applespi_drain_reads(), will wait_event_lock_irq() wait indefinitely for
read_active to be cleared, resulting in a deadlock?

> +	cancel_work_sync(&applespi->work);
> +

[Severity: High]
This is a pre-existing issue, but can the device trigger unbounded memory
allocations and input device registrations?

In applespi_handle_cmd_response(), whenever an INFO packet is received,
it unconditionally schedules the initialization worker:

applespi_handle_cmd_response()
{
	if (packet->device == PACKET_DEV_INFO &&
	    le16_to_cpu(message->type) == 0x1020) {
...
		applespi->rcvd_tp_info = message->tp_info;
		schedule_work(&applespi->work);
		return;
	}
...

The worker then allocates and registers a new touchpad input device
using devm_input_allocate_device():

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

If the device repeatedly sends 0x1020 INFO responses, won't this continually
allocate new managed devices and leak the previous ones?

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

[Severity: Low]
This isn't a bug introduced by this patch, but is the manual assignment of
input_dev->dev.parent redundant here?

Looking at applespi_probe(), it uses devm_input_allocate_device(), which
automatically sets the parent to the device passed to it:

applespi_probe()
{
...
	applespi->keyboard_input_dev = devm_input_allocate_device(&spi->dev);
...
	applespi->keyboard_input_dev->dev.parent = &spi->dev;
...

The same applies to touchpad_input_dev in applespi_register_touchpad_device().


[Severity: High]
This is a pre-existing issue, but do applespi_got_data() and
applespi_handle_cmd_response() read out-of-bounds memory when processing
messages?

applespi_got_data() checks the packet length against sizeof(packet->data):

	if (len > sizeof(packet->data)) {
...
		goto msg_complete;
	}

But when handling the message payload, it casts the data and processes it
without checking if the packet actually contains the full struct:

	if (packet->flags == PACKET_TYPE_READ &&
	    packet->device == PACKET_DEV_KEYB) {
		applespi_handle_keyboard_event(applespi, &message->keyboard);

And similarly in applespi_handle_cmd_response():

	if (packet->device == PACKET_DEV_INFO &&
	    le16_to_cpu(message->type) == 0x1020) {
...
		applespi->rcvd_tp_info = message->tp_info;

If an undersized packet is received, does this lead to reading uninitialized
buffer memory (stale data) or out-of-bounds memory?


[Severity: High]
This is a pre-existing issue, but is there a use-after-free risk if
applespi_probe() fails after the keyboard input device is registered?

In applespi_probe(), the keyboard input device is registered:

	sts = input_register_device(applespi->keyboard_input_dev);
	if (sts) {
...

Later in probe, ACPI GPE setup can fail, returning an error:

	acpi_sts = acpi_install_gpe_handler(...);
	if (ACPI_FAILURE(acpi_sts)) {
...
		return -ENODEV;
	}

Because the input device was already visible to userspace, an LED event might
have queued an asynchronous SPI write. If probe returns an error, devres will
free the applespi structure, but the in-flight SPI write is not cancelled.

Will the completion callback then access the freed applespi structure?

applespi_async_write_complete()
{
...
	if (!applespi_check_write_status(applespi, applespi->wr_m.status)) {
...


[Severity: High]
This is a pre-existing issue, but is there a risk of a NULL pointer dereference
if the tp_dim debugfs file is opened before the touchpad is initialized?

In applespi_probe(), the tp_dim debugfs file is created synchronously:

	debugfs_create_file("tp_dim", 0400, applespi->debugfs_root, applespi,
			    &applespi_tp_dim_fops);

However, applespi->touchpad_input_dev is allocated asynchronously in
applespi_worker(). If userspace opens the file before the worker executes
(or if the hardware never responds), applespi_tp_dim_open() will dereference
a NULL touchpad_input_dev:

static int applespi_tp_dim_open(struct inode *inode, struct file *file)
{
...
	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 result in a kernel panic?

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