Re: [PATCH v2] HID: wacom: stop hardware after post-start probe failures

[sashiko-bot@kernel.org]

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

Pre-existing issues:
- [High] Failing to cancel scheduled delayed works and timers on the probe error path leads to a Use-After-Free when the `wacom` structure is freed.
- [High] Nullifying input pointers in `wacom_register_inputs` before stopping the hardware leaves a race window where incoming interrupts can cause a NULL pointer dereference.
- [High] Hardware interrupts can arrive before `wacom->remote` is allocated, causing a NULL pointer dereference in the IRQ handlers.
--

commit 517a367ac93aa0d6c6470c3a21cdb477449345e3
Author: Myeonghun Pak <mhun512@gmail.com>
Subject: HID: wacom: stop hardware after post-start probe failures

This patch routes post-hid_hw_start() failures in wacom_parse_and_register()
through hid_hw_stop() before releasing driver resources. This ensures that the
HID hardware is properly stopped before the wacom resources are freed.

> diff --git a/drivers/hid/wacom_sys.c b/drivers/hid/wacom_sys.c
> index 2220168bf1164..252771dcfd8f8 100644
> --- a/drivers/hid/wacom_sys.c
> +++ b/drivers/hid/wacom_sys.c
> @@ -2462,16 +2462,16 @@ static int wacom_parse_and_register(struct wacom *wacom, bool wireless)
>  
>  	error = wacom_register_inputs(wacom);
>  	if (error)
> -		goto fail;
> +		goto fail_hw_stop;

[Severity: High]
This is a pre-existing issue, but does this leave a small race window where
an interrupt could cause a NULL pointer dereference?

If wacom_register_inputs() fails, its error path sets the input pointers to
NULL:

    fail:
        wacom_wac->pad_input = NULL;
        wacom_wac->touch_input = NULL;
        wacom_wac->pen_input = NULL;
        return error;

The code then jumps to fail_hw_stop. Since hid_hw_start() has already been
called, the hardware is active. Could an interrupt arrive before hid_hw_stop()
completes, triggering an IRQ handler like wacom_intuos_irq() which
dereferences wacom->pen_input without checking for NULL?

>  
>  	if (wacom->wacom_wac.features.device_type & WACOM_DEVICETYPE_PAD) {
>  		error = wacom_initialize_leds(wacom);
>  		if (error)
> -			goto fail;
> +			goto fail_hw_stop;
>  
>  		error = wacom_initialize_remotes(wacom);
>  		if (error)
> -			goto fail;
> +			goto fail_hw_stop;

[Severity: High]
This is a pre-existing issue, but I have a question about how this handles
interrupts arriving before initialization is complete.

Since hid_hw_start() is called prior to wacom_initialize_remotes():

    error = hid_hw_start(hdev, connect_mask);
    ...
    error = wacom_initialize_remotes(wacom);

Could an early hardware event or a malicious USB report arrive and trigger
wacom_remote_irq() or wacom_remote_status_irq()? Those handlers appear to
dereference wacom->remote to acquire a spinlock:

    spin_lock_irqsave(&remote->remote_lock, flags);

If the interrupt arrives before wacom_initialize_remotes() has allocated
wacom->remote, would this lead to a NULL pointer dereference?

[ ... ]

> @@ -2501,7 +2501,7 @@ static int wacom_parse_and_register(struct wacom *wacom, bool wireless)
>  
>  	return 0;
>  
> -fail_quirks:
> +fail_hw_stop:
>  	hid_hw_stop(hdev);
>  fail:
>  	wacom_release_resources(wacom);

[Severity: High]
This isn't a bug introduced by this patch, but it seems there might be a
pre-existing issue with pending works on the error path.

If wacom_query_tablet_data() is called earlier:

    wacom_query_tablet_data(struct wacom *wacom)
    {
        schedule_delayed_work(&wacom->init_work, msecs_to_jiffies(1000));
    }

And then a subsequent step like hid_hw_open() fails, we jump to fail_hw_stop
which stops the hardware and releases resources:

    fail_hw_stop:
        hid_hw_stop(hdev);
    fail:
        wacom_release_resources(wacom);
        return error;

Since the error is returned to wacom_probe() which can cause the devres core
to free the wacom structure, could the uncancelled wacom->init_work (or other
works/timers scheduled by interrupts) execute later and dereference the freed
wacom memory?

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260604045710.25512-1-mhun512@gmail.com?part=1