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

Pre-existing issues:
- [High] Repeated processing of expected or malicious hardware `PACKET_DEV_INFO` responses causes unbounded allocation and registration of `devm`-managed input devices.
- [High] Use-After-Free of `applespi` private data in the asynchronous SPI read callback during driver unbind.
- [High] Missing bounds check on keyboard release events causes an out-of-bounds read of kernel memory, leading to an information leak.
--

--- Patch [3]: [PATCH v3 3/3] Input: applespi - fix NULL pointer dereference in tp_dim open ---
commit 44778c849d055105696c2f1ad69bc1cf09ff47d0
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()
when userspace accesses the debugfs file before asynchronous initialization
completes. It uses smp_load_acquire() to safely load the touchpad input
device and returns -ENODEV if not initialized.

> diff --git a/drivers/input/keyboard/applespi.c b/drivers/input/keyboard/applespi.c
> index fd785dba1174e..e3e239f430bd9 100644
> --- a/drivers/input/keyboard/applespi.c
> +++ b/drivers/input/keyboard/applespi.c
> @@ -972,12 +972,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: High]
This isn't a bug introduced by this patch, but does the driver leak resources
if it repeatedly processes hardware device info responses?

If applespi_handle_cmd_response() receives this packet, it schedules the
worker:

applespi_handle_cmd_response() {
    ...
    schedule_work(&applespi->work);
}

The worker unconditionally calls applespi_register_touchpad_device(), which
allocates a new devm-managed device:

applespi_register_touchpad_device() {
    ...
    touchpad_input_dev = devm_input_allocate_device(&applespi->spi->dev);
    ...
}

Could this lead to unbounded allocation and registration of devm-managed input
devices, losing the reference to the old ones and eventually exhausting memory?

[Severity: High]
This is a pre-existing issue, but is there a potential use-after-free in the
asynchronous SPI read callback during driver unbind?

During driver removal, applespi_remove() calls applespi_drain_reads(), which
waits for read_active to become false:

applespi_remove() {
    ...
    applespi_drain_reads(applespi);
    ...
}

In applespi_async_read_complete(), applespi_msg_complete() sets read_active
to false and wakes the waitqueue, then proceeds to use applespi->gpe:

applespi_async_read_complete() {
    ...
    applespi_msg_complete(applespi, false, false);
    ...
    acpi_finish_gpe(NULL, applespi->gpe);
}

If devres cleanup frees the applespi struct immediately after
applespi_drain_reads() returns, could accessing applespi->gpe cause a
use-after-free?

[Severity: High]
This isn't a bug introduced by this patch, but is there an out-of-bounds read
when processing keyboard release events?

In applespi_handle_keyboard_event(), the unvalidated keys_pressed array is
copied directly into last_keys_pressed:

applespi_handle_keyboard_event() {
    ...
    memcpy(&applespi->last_keys_pressed, keyboard_protocol->keys_pressed,
           sizeof(applespi->last_keys_pressed));
    ...
}

On a subsequent keyboard event, if a previously stored invalid keycode is
processed as released, it is passed to applespi_code_to_key():

applespi_handle_keyboard_event() {
    ...
    key = applespi_code_to_key(applespi->last_keys_pressed[i],
                               applespi->last_keys_fn_pressed[i]);
    ...
}

applespi_code_to_key() uses this unvalidated keycode to index the
applespi_scancodes array:

applespi_code_to_key() {
    ...
    unsigned int key = applespi_scancodes[code];
    ...
}

Could a maliciously crafted SPI packet cause an out-of-bounds read of
applespi_scancodes leading to an information leak?

>  	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);
> 
>  	return nonseekable_open(inode, file);
>  }

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