Re: [PATCH 1/2] Input: applespi - register touchpad device synchronously in probe

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 6 potential issue(s) to consider:
- [Critical] Re-entrancy in async SPI completion leads to callback slot overwrite and NULL pointer dereference.
- [High] Missing timeouts in probe error path and remove path cause indefinite system hang if the SPI controller wedges.
- [High] Synchronous touchpad detection failure forces a complete driver teardown, disabling the functional keyboard.
- [Medium] Lockless copy and evaluation of touchpad info struct causes a data race.
- [Medium] Timeout condition fails if the touchpad's model number is 0.
- [Low] Redundant and contradictory debugfs NULL pointer check.
--

--- Patch [1]: [PATCH 1/2] Input: applespi - register touchpad device synchronously in probe ---
commit 4961cde9c37a41e959c60a2aa6d8bde99a5e96b5
Author: Shih-Yuan Lee <fourdollars@debian.org>

Input: applespi - register touchpad device synchronously in probe

This patch simplifies the applespi driver by making touchpad detection and
registration synchronous during probe rather than using an asynchronous worker.
It also introduces timeouts for wait events and changes the probe type to prefer
asynchronous execution by the driver core.

> diff --git a/drivers/input/keyboard/applespi.c b/drivers/input/keyboard/applespi.c
> index b5ff71cd5a701..c1065a6ba96c8 100644
> --- a/drivers/input/keyboard/applespi.c
> +++ b/drivers/input/keyboard/applespi.c
[ ... ]
> @@ -607,13 +612,61 @@ static void applespi_setup_write_txfrs(struct applespi_data *applespi)
>  	spi_message_add_tail(st_t, msg);
>  }
>  
> +static bool applespi_async_outstanding(struct applespi_data *applespi)
> +{
> +	return applespi->spi_complete[0].complete ||
> +	       applespi->spi_complete[1].complete;
> +}
> +
> +static void applespi_async_complete(void *context)
> +{
> +	struct applespi_complete_info *info = context;
> +	struct applespi_data *applespi = info->applespi;
> +	unsigned long flags;
> +
> +	info->complete(applespi);
> +
> +	spin_lock_irqsave(&applespi->cmd_msg_lock, flags);
> +
> +	info->complete = NULL;

[Severity: Critical]
Can this execution order cause a nested applespi_async() call to mistakenly
view the slot as occupied?

Because info->complete(applespi) executes before info->complete = NULL, if the
callback queues a new SPI message, it might see this slot as still in use.

> +
> +	if (applespi->cancel_spi && !applespi_async_outstanding(applespi))
> +		wake_up_all(&applespi->wait_queue);
> +
> +	spin_unlock_irqrestore(&applespi->cmd_msg_lock, flags);
> +}
> +
>  static int applespi_async(struct applespi_data *applespi,
>  			  struct spi_message *message, void (*complete)(void *))
>  {
> -	message->complete = complete;
> -	message->context = applespi;
> +	struct applespi_complete_info *info;
> +	int sts;
>  
> -	return spi_async(applespi->spi, message);
> +	if (applespi->cancel_spi) {
> +		if (!applespi_async_outstanding(applespi))
> +			wake_up_all(&applespi->wait_queue);
> +		return -ESHUTDOWN;
> +	}
> +
> +	/*
> +	 * There can only be at most 2 spi requests in flight, one for "reads"
> +	 * and one for "writes".
> +	 */
> +	if (!applespi->spi_complete[0].complete)
> +		info = &applespi->spi_complete[0];
> +	else
> +		info = &applespi->spi_complete[1];
> +	info->complete = complete;

[Severity: Critical]
Does this blindly overwrite the second slot if the first is occupied, even if
the second slot is also in use?

If a re-entrant call happens before info->complete is cleared in
applespi_async_complete(), this might overwrite the callback of a concurrent
in-flight transfer. This could lead to a NULL pointer dereference or
use-after-free later.

> +	info->applespi = applespi;
> +
> +	message->complete = applespi_async_complete;
> +	message->context = info;
> +
> +	sts = spi_async(applespi->spi, message);
[ ... ]
> @@ -963,12 +1016,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;
> +

[Severity: Low]
Is this NULL pointer check redundant?

The commit message states that the patch "obsoletes the need for async work
cancellation on driver remove and debugfs NULL pointer checks." However, this
adds a new NULL pointer check.

Since the probe is now synchronous, the debugfs file isn't created until the
touchpad is fully registered, making it impossible for touchpad_input_dev to be
NULL here.

>  	snprintf(applespi->tp_dim_val, sizeof(applespi->tp_dim_val),
>  		 "0x%.4x %dx%d+%u+%u\n",
[ ... ]
> @@ -1324,26 +1383,14 @@ applespi_register_touchpad_device(struct applespi_data *applespi,
>  	return 0;
>  }
>  
> -static void applespi_worker(struct work_struct *work)
> -{
[ ... ]
> -}
> -
>  static void applespi_handle_cmd_response(struct applespi_data *applespi,
>  					 struct spi_packet *packet,
>  					 struct message *message)
>  {
>  	if (packet->device == PACKET_DEV_INFO &&
>  	    le16_to_cpu(message->type) == 0x1020) {
> -		/*
> -		 * We're not allowed to sleep here, but registering an input
> -		 * device can sleep.
> -		 */
>  		applespi->rcvd_tp_info = message->tp_info;
> -		schedule_work(&applespi->work);
> +		wake_up_all(&applespi->wait_queue);

[Severity: Medium]
Is there a data race here since the 112-byte message->tp_info struct is copied
locklessly before waking the wait queue?

>  		return;
>  	}
[ ... ]
> @@ -1757,6 +1803,22 @@ static int applespi_probe(struct spi_device *spi)
>  	/* trigger touchpad setup */
>  	applespi_init(applespi, false);
>  
> +	/* set up the touchpad as a separate input device */
> +	sts = wait_event_timeout(applespi->wait_queue,
> +				 applespi->rcvd_tp_info.model_no,
> +				 msecs_to_jiffies(3000));

[Severity: Medium]
Because applespi->rcvd_tp_info.model_no is evaluated locklessly without an
acquire memory barrier, could the waiting thread observe a non-zero model_no
and wake up before preceding fields are fully committed to memory?

[Severity: Medium]
What happens if a legitimate touchpad reports a model_no of 0?

Since applespi->rcvd_tp_info.model_no is used as a boolean flag, a value of 0
would cause the wait condition to fail and trigger the 3-second timeout, even
though the hardware responded successfully.

> +	if (!sts) {
> +		dev_err(&applespi->spi->dev,
> +			"Timed out waiting for device info\n");
> +		sts = -ETIMEDOUT;
> +		goto cancel_spi;
> +	}

[Severity: High]
Does a touchpad detection timeout here completely abort the driver probe and
disable a fully functional keyboard?

In the previous asynchronous design, a missing info packet safely skipped
touchpad registration while leaving the keyboard intact.

> +
> +	sts = applespi_register_touchpad_device(applespi,
> +						&applespi->rcvd_tp_info);
> +	if (sts)
> +		goto cancel_spi;
> +
>  	/*
>  	 * By default this device is not enabled for wakeup; but USB keyboards
>  	 * generally are, so the expectation is that by default the keyboard
> @@ -1789,25 +1851,50 @@ static int applespi_probe(struct spi_device *spi)
>  			    &applespi_tp_dim_fops);
>  
>  	return 0;
> +
> +cancel_spi:
> +	acpi_disable_gpe(NULL, applespi->gpe);
> +	acpi_remove_gpe_handler(NULL, applespi->gpe, applespi_notify);
> +
> +	spin_lock_irqsave(&applespi->cmd_msg_lock, flags);
> +	applespi->cancel_spi = true;
> +	wait_event_lock_irq(applespi->wait_queue,
> +			    !applespi_async_outstanding(applespi),
> +			    applespi->cmd_msg_lock);

[Severity: High]
Does this probe error path lack a timeout?

The commit message mentions adding 3-second timeouts to prevent suspend/remove
hangs, but if the SPI controller wedges, this wait_event_lock_irq() could hang
indefinitely. This same issue appears to be present in applespi_remove().

> +	spin_unlock_irqrestore(&applespi->cmd_msg_lock, flags);
> +
> +	return sts;
>  }

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