Re: [PATCH v5 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 1 potential issue(s) to consider:

Pre-existing issues:
- [High] Unsolicited or repeated PACKET_DEV_INFO messages cause multiple touchpad input devices to be allocated and registered, leaking previously registered ones.
--

--- Patch [2]: [PATCH v5 2/3] Input: applespi - fix NULL pointer dereference in tp_dim open ---
commit 083469b928bf6d69e48361fe4f8c93f87f2a7820
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.

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

[Severity: High]
This isn't a bug introduced by this patch, but looking at the worker
thread that sets applespi->touchpad_input_dev, is there a resource leak
if the hardware sends repeated PACKET_DEV_INFO messages?

In applespi_handle_cmd_response(), work is unconditionally scheduled
whenever a PACKET_DEV_INFO packet is received:

drivers/input/keyboard/applespi.c:applespi_handle_cmd_response() {
    ...
    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);
        return;
    }
    ...
}

When the worker runs applespi_register_touchpad_device(), it unconditionally
allocates and registers a new input device via devm_input_allocate_device()
and input_register_device(), overwriting the applespi->touchpad_input_dev
pointer.

Because the allocated device is devm managed, the old device and its associated
character device minor numbers are never unregistered or freed until the SPI
driver unbinds. Does this code need to check if the touchpad device is already
initialized before allocating and registering a new one to prevent exhausting
system memory?

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

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