Re: [PATCH v5 1/3] Input: applespi - cancel pending work on driver remove

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Repeated PACKET_DEV_INFO packets lead to unbounded devm memory leaks and duplicate input device registrations.
- [High] Unchecked access of asynchronously allocated applespi->touchpad_input_dev in the debugfs tp_dim open handler.
- [High] Unbound wait in applespi_drain_writes() can block driver unbind and suspend forever if hardware is unresponsive.
- [High] Missing async teardown in applespi_probe error path if GPE enablement fails.
- [High] Mismatched finger data reported to input subsystem due to flawed iteration logic.
- [Low] Redundant manual assignment of dev.parent on input devices allocated with devm_input_allocate_device().
--

--- Patch [1]: [PATCH v5 1/3] Input: applespi - cancel pending work on driver remove ---
commit 541134ed7dfbfee9d31126ce14257c1f72d8e7a7
Author: Shih-Yuan Lee <fourdollars@debian.org>

Input: applespi - cancel pending work on driver remove

During driver removal in applespi_remove(), the managed private data
structure is freed by devres. However, the driver does not cancel the
asynchronous work applespi->work, which registers the touchpad input
device.

> 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
> @@ -1820,8 +1820,10 @@ static void applespi_remove(struct spi_device *spi)
>  {
>  	struct applespi_data *applespi = spi_get_drvdata(spi);
>  
>  	applespi_drain_writes(applespi);

[Severity: High]
This isn't a bug introduced by this patch, but can this wait indefinitely
and block driver unbind or system suspend?

applespi_drain_writes() waits for the hardware without a timeout:

drivers/input/keyboard/applespi.c:applespi_drain_writes() {
    ...
    wait_event_lock_irq(applespi->drain_complete, !applespi->write_active,
                        applespi->cmd_msg_lock);
    ...
}

If the external hardware hangs and write_active remains true, would this
task become blocked forever?

>  
>  	acpi_disable_gpe(NULL, applespi->gpe);
>  	acpi_remove_gpe_handler(NULL, applespi->gpe, applespi_notify);
>  	device_wakeup_disable(&spi->dev);
>  
>  	applespi_drain_reads(applespi);
>  
> +	cancel_work_sync(&applespi->work);
> +
>  	debugfs_remove_recursive(applespi->debugfs_root);
>  }

[Severity: Low]
This isn't a bug introduced by this patch, but does this code redundantly
assign the parent device?

drivers/input/keyboard/applespi.c:applespi_register_touchpad_device() {
    ...
    touchpad_input_dev = devm_input_allocate_device(&applespi->spi->dev);
    ...
    touchpad_input_dev->dev.parent = &applespi->spi->dev;
    ...
}

And similarly in applespi_probe():

drivers/input/keyboard/applespi.c:applespi_probe() {
    ...
    applespi->keyboard_input_dev->dev.parent = &spi->dev;
    ...
}

The input subsystem guidelines state that devm_input_allocate_device()
automatically sets the parent, making manual assignment redundant.

[Severity: High]
This isn't a bug introduced by this patch, but can a repeated
PACKET_DEV_INFO from the hardware cause an unbounded memory leak and
duplicate device registrations?

drivers/input/keyboard/applespi.c:applespi_handle_cmd_response() {
    ...
    if (packet->type == PACKET_DEV_INFO) {
        ...
        schedule_work(&applespi->work);
    }
    ...
}

When the work runs, it executes applespi_register_touchpad_device(),
which unconditionally allocates a new managed device:

drivers/input/keyboard/applespi.c:applespi_register_touchpad_device() {
    ...
    touchpad_input_dev = devm_input_allocate_device(&applespi->spi->dev);
    ...
    error = input_register_device(touchpad_input_dev);
    ...
}

If the peripheral sends multiple PACKET_DEV_INFO packets, would this
blindly allocate and register multiple touchpad devices without checking
if one already exists?

[Severity: High]
This isn't a bug introduced by this patch, but could this dereference a
NULL pointer if the async worker hasn't completed initializing the
touchpad?

drivers/input/keyboard/applespi.c:applespi_probe() {
    ...
    debugfs_create_file("tp_dim", 0400, applespi->debugfs_root, applespi,
                        &applespi_tp_dim_fops);
    ...
}

If userspace opens this file before applespi_register_touchpad_device()
is finished by the workqueue, applespi_tp_dim_open() is called:

drivers/input/keyboard/applespi.c:applespi_tp_dim_open() {
    ...
    applespi->touchpad_input_dev->id.product,
    ...
}

Since touchpad_input_dev might still be NULL, will this crash the kernel?

[Severity: High]
This isn't a bug introduced by this patch, but does the probe error path
miss cleaning up async tasks and SPI reads?

drivers/input/keyboard/applespi.c:applespi_probe() {
    ...
    acpi_sts = acpi_enable_gpe(NULL, applespi->gpe);
    if (ACPI_FAILURE(acpi_sts)) {
        dev_err(&spi->dev, "failed to enable GPE\n");
        return -ENODEV;
    }
    ...
}

If acpi_install_gpe_handler() (called earlier) receives an interrupt
before acpi_enable_gpe() fails, it could queue an async SPI transfer or
work item. Returning -ENODEV immediately frees the applespi structure
without calling applespi_drain_reads() or cancel_work_sync(), which
could result in a use-after-free.

[Severity: High]
This isn't a bug introduced by this patch, but does this iteration logic
mismatch the finger data reported to the input subsystem?

drivers/input/keyboard/applespi.c:report_tp_state() {
    ...
    report_finger_data(input, applespi->slots[i], &applespi->pos[i],
                       &t->fingers[i]);
    ...
}

If earlier loops filter out fingers (e.g., touch_major == 0) and store
their coordinates densely in applespi->pos, reusing the same sequential
index i for t->fingers[i] could pair the packed positions with the
wrong original finger data from the hardware. Could this corrupt
multitouch reporting?

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