Re: [PATCH v4 2/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] Missing state clearance in early return paths of `applespi_got_data` causes deadlocks on device suspend or removal.
- [High] Re-submitting an active `spi_message` on timeout causes SPI core queue list corruption.
--

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

Input: applespi - fix NULL pointer dereference in tp_dim open

This commit fixes a NULL pointer dereference in applespi_tp_dim_open().
It uses smp_load_acquire() to safely load the asynchronously initialized
touchpad_input_dev and returns -ENODEV if it is not yet available.

> diff --git a/drivers/input/keyboard/applespi.c b/drivers/input/keyboard/applespi.c
> index 3bdb9e7cfb8b2..4d339445e9c71 100644
> --- a/drivers/input/keyboard/applespi.c
> +++ b/drivers/input/keyboard/applespi.c
> @@ -963,12 +963,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 can the early return paths in
applespi_got_data() cause a deadlock on device suspend or removal?

In drivers/input/keyboard/applespi.c:applespi_got_data(), if a packet arrives
with a CRC error or as a partial multi-packet receipt, the function returns
early without clearing applespi->read_active:

    if (applespi->drain) {
        applespi->read_active = false;
        applespi->write_active = false;
        wake_up_all(&applespi->drain_complete);
    }
    return;

If the device is then suspended, applespi_drain_reads() is called, which waits
indefinitely:

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

Since GPE interrupts are disabled during suspend and no new reads are queued,
does read_active remain true forever, deadlocking the system?


[Severity: High]
This is a pre-existing issue, but does the timeout logic in
applespi_send_cmd_msg() cancel the active SPI message before resubmitting it?

When a timeout occurs, the internal state is cleared:

    if (ktime_ms_delta(ktime_get(), applespi->cmd_msg_queued) < 1000)
        return 0;

    dev_warn(&applespi->spi->dev, "Command %d timed out\n",
             applespi->cmd_evt_type);

    applespi->cmd_msg_queued = 0;
    applespi->write_active = false;

But then the exact same spi_message (&applespi->wr_m) is resubmitted:

    sts = applespi_async(applespi, &applespi->wr_m,
                         applespi_async_write_complete);

If the original message is still actively linked in the SPI core's queue,
will passing the same spi_message to spi_async() again cause list corruption
and a kernel panic?

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