Re: [PATCH v2 3/3] Input: applespi - fix NULL pointer dereference in tp_dim open

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Unbounded allocations of touchpad input devices triggered by repeated PACKET_DEV_INFO SPI messages.
- [High] Deadlock in `applespi_drain_reads` during module unload or system suspend after a CRC error.
--

--- Patch [3]: [PATCH v2 3/3] Input: applespi - fix NULL pointer dereference in tp_dim open ---
commit 54174cce6bfb0203a63026dc8e54eb65b7ee620c
Author: Shih-Yuan Lee <fourdollars@debian.org>

Input: applespi - fix NULL pointer dereference in tp_dim open

The tp_dim debugfs file is registered synchronously during driver probe
in applespi_probe(). However, the applespi->touchpad_input_dev is
initialized and registered asynchronously in the driver's worker thread.
If a userspace process opens the debugfs file before the worker thread
has completed initialization, it will dereference the NULL pointer.
This commit uses smp_load_acquire to safely load the pointer and returns
-ENODEV if not initialized.

> diff --git a/drivers/input/keyboard/applespi.c b/drivers/input/keyboard/applespi.c
> index b6b4d258d0dd5..1f8e4ae902855 100644
> --- a/drivers/input/keyboard/applespi.c
> +++ b/drivers/input/keyboard/applespi.c
> @@ -968,12 +968,18 @@ static void applespi_debug_update_dimensions(struct applespi_data *applespi,
>  static int applespi_tp_dim_open(struct inode *inode, struct file *file)
>  {
>  	struct applespi_data *applespi = inode->i_private;
> +	struct input_dev *touchpad;
>  
>  	file->private_data = applespi;
>  
> +	/* Pairs with smp_store_release in applespi_register_touchpad_device() */
> +	touchpad = smp_load_acquire(&applespi->touchpad_input_dev);
> +	if (!touchpad)
> +		return -ENODEV;
> +
>  	snprintf(applespi->tp_dim_val, sizeof(applespi->tp_dim_val),
>  		 "0x%.4x %dx%d+%u+%u\n",
> -		 applespi->touchpad_input_dev->id.product,
> +		 touchpad->id.product,
>  		 applespi->tp_dim_min_x, applespi->tp_dim_min_y,
>  		 applespi->tp_dim_max_x - applespi->tp_dim_min_x,
>  		 applespi->tp_dim_max_y - applespi->tp_dim_min_y);

[Severity: High]
This isn't a bug introduced by this patch, but could this lead to an unbounded
allocation of touchpad input devices?

When the SPI device sends a PACKET_DEV_INFO message,
applespi_handle_cmd_response() unconditionally queues applespi->work:

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

The worker function applespi_worker() then calls
applespi_register_touchpad_device(), which unconditionally allocates and
registers a new touchpad device:

	/* create touchpad input device */
	touchpad_input_dev = devm_input_allocate_device(&applespi->spi->dev);
	...
	/* register input device */
	sts = input_register_device(touchpad_input_dev);

This appears to overwrite the applespi->touchpad_input_dev pointer without
freeing or unregistering the previous one. 

If the hardware sends multiple info packets, does this cause a resource leak 
of devm-managed memory and input devices?

[Severity: High]
This is a pre-existing issue, but is there a potential deadlock during module
unload or system suspend after a CRC error?

In applespi_got_data(), if a SPI packet fails the CRC check and the driver
is not currently draining, the function returns early without clearing
applespi->read_active:

	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;
			applespi->write_active = false;
			wake_up_all(&applespi->drain_complete);
		}
		return;
	}

If no further SPI interrupts arrive before a system suspend or rmmod,
applespi_remove() and applespi_suspend() disable GPE interrupts and then call
applespi_drain_reads():

	/* disable GPE interrupt */
	acpi_disable_gpe(NULL, applespi->gpe);
	applespi_drain_reads(applespi);

applespi_drain_reads() will then wait for !applespi->read_active:

	wait_event_lock_irq(applespi->drain_complete, !applespi->read_active,
			    applespi->cmd_msg_lock);

Since interrupts are disabled, no new read can arrive to clear the flag. 

Can this result in an unrecoverable deadlock waiting for read_active to clear?

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