Re: [PATCH v2 4/5] Input: applespi - prefer asynchronous driver probing

[sashiko-bot@kernel.org]

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

New issues:
- [High] Failure to cancel in-flight SPI async transfers upon unbind timeout results in use-after-free in completion callbacks.

Pre-existing issues:
- [High] Kernel panic in `applespi_tp_dim_open` due to an unchecked NULL `touchpad_input_dev` when touchpad initialization times out.
--

--- Patch [4]: [PATCH v2 4/5] Input: applespi - prefer asynchronous driver probing ---
commit df316467951827e13575294cf4f7b16d6876f89a
Author: Shih-Yuan Lee <fourdollars@debian.org>

Input: applespi - prefer asynchronous driver probing
    
Set probe_type to PROBE_PREFER_ASYNCHRONOUS to allow the driver core
to run applespi_probe() asynchronously. This improves system boot speeds by
avoiding blocking the main kernel thread during the 3-second touchpad
detection wait.
    
Additionally, clean up applespi_tp_dim_open() by simplifying product ID
retrieval and avoiding dereferencing touchpad_input_dev without a helper
variable.

> diff --git a/drivers/input/keyboard/applespi.c b/drivers/input/keyboard/applespi.c
> --- a/drivers/input/keyboard/applespi.c
> +++ b/drivers/input/keyboard/applespi.c
> @@ -1027,12 +1027,13 @@ 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 = applespi->touchpad_input_dev;
>  
>  	file->private_data = applespi;
>  
>  	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,

[Severity: High]
This isn't a bug introduced by this patch, but can this dereference a NULL
pointer if the touchpad fails to initialize?

If the 3-second wait in applespi_probe() times out:

applespi_probe() {
    ...
    sts = wait_event_timeout(applespi->wait_queue,
                             READ_ONCE(applespi->have_tp_info),
                             msecs_to_jiffies(3000));
    ...
}

Then applespi->touchpad_input_dev is never allocated. However, the
debugfs file is still created unconditionally later in applespi_probe():

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

If a user subsequently reads this file, applespi_tp_dim_open() is called
and will dereference the unconditionally NULL touchpad pointer when
retrieving the product ID.

>  		 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);

[ ... ]

> @@ -2015,6 +2016,7 @@ static struct spi_driver applespi_driver = {
>  		.name			= "applespi",
>  		.acpi_match_table	= applespi_acpi_match,
>  		.pm			= pm_sleep_ptr(&applespi_pm_ops),
> +		.probe_type		= PROBE_PREFER_ASYNCHRONOUS,

[Severity: High]
Does the timeout added in this series for unbinding lead to a use-after-free
if SPI transfers are still active?

In applespi_remove(), the driver attempts to drain in-flight operations:

applespi_remove() {
    applespi_drain_writes(applespi);
    acpi_disable_gpe(NULL, applespi->gpe);
    ...
    applespi_drain_reads(applespi);
    ...
}

These drain functions wait with a 3-second timeout:

applespi_drain_writes() {
    wait_event_lock_irq_timeout(applespi->wait_queue,
                                !applespi->write_active,
                                applespi->cmd_msg_lock,
                                msecs_to_jiffies(3000));
}

If this timeout expires because the hardware is unresponsive, the function
returns and applespi_remove() completes. The devres subsystem then frees
the applespi structure and its DMA-mapped buffers.

When the asynchronous SPI transfer eventually completes, the SPI core invokes
applespi_async_complete() with the freed context pointer:

applespi_async_complete() {
    struct applespi_complete_info *info = context;
    struct applespi_data *applespi = info->applespi;
    ...
    spin_lock_irqsave(&applespi->cmd_msg_lock, flags);
    ...
}

Will this dereference the freed applespi object and corrupt memory?

>  	},
>  	.probe		= applespi_probe,
>  	.remove		= applespi_remove,

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