Re: [PATCH 1/4] hwmon: (corsair-cpro) Stop device IO before calling hid_hw_stop

[sashiko-bot@kernel.org]

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

New issues:
- [Low] The patch introduces a spurious kernel warning in the probe error path when `hid_hw_open()` fails by unconditionally calling `hid_device_io_stop()`.

Pre-existing issues:
- [High] The driver leaks a debugfs directory on probe failure, creating a Use-After-Free vulnerability.
- [High] A timeout during a USB command causes the driver to consume stale delayed responses for subsequent commands, permanently desynchronizing the protocol.
--

--- Patch [1]: [PATCH 1/4] hwmon: (corsair-cpro) Stop device IO before calling hid_hw_stop ---
commit 0cdcb75d39b8efee212d1a27b0eb0c56aaafb5e9
Author: Guenter Roeck <linux@roeck-us.net>

hwmon: (corsair-cpro) Stop device IO before calling hid_hw_stop

Calling hid_hw_stop() does not stop the device IO. This results in a race
condition between hid_input_report() and driver probe failure. This commit
calls hid_device_io_stop() before hid_hw_stop() to fix the issue.

> diff --git a/drivers/hwmon/corsair-cpro.c b/drivers/hwmon/corsair-cpro.c
> --- a/drivers/hwmon/corsair-cpro.c
> +++ b/drivers/hwmon/corsair-cpro.c
[ ... ]
> @@ -646,6 +646,7 @@ static int ccp_probe(struct hid_device *hdev, const struct hid_device_id *id)
>  out_hw_close:
>  	hid_hw_close(hdev);
>  out_hw_stop:
> +	hid_device_io_stop(hdev);

[Severity: Low]
Will this cause a spurious kernel warning if hid_hw_open() fails? 

If hid_hw_open() fails earlier in ccp_probe(), the code jumps directly to 
the out_hw_stop label:

ccp_probe() {
    ...
    ret = hid_hw_open(hdev);
    if (ret)
        goto out_hw_stop;
    ...
    hid_device_io_start(hdev);
    ...
}

Since hid_device_io_start() hasn't been called yet, hdev->io_started will 
be false. Calling hid_device_io_stop() here will trigger the dev_warn() 
"io already stopped" from the HID core. Could we conditionally check if IO 
was started, or introduce a new error label to avoid this?

>  	hid_hw_stop(hdev);
>  	return ret;
>  }

[Severity: High]
This isn't a bug introduced by this patch, but does the driver leak a 
debugfs directory if hwmon registration fails?

If hwmon_device_register_with_info() fails in ccp_probe(), the code jumps 
to out_hw_close:

ccp_probe() {
    ...
    ccp_debugfs_init(ccp);

    ccp->hwmon_dev = hwmon_device_register_with_info(&hdev->dev, ...);
    if (IS_ERR(ccp->hwmon_dev)) {
        ret = PTR_ERR(ccp->hwmon_dev);
        goto out_hw_close;
    }
    ...
}

Because ccp is allocated with devm_kzalloc(), devres will free it when the 
probe fails. However, without a call to debugfs_remove_recursive(), the 
debugfs files remain. If userspace later reads a leaked file like 
firmware_version, firmware_show() will dereference the freed ccp pointer.

[Severity: High]
This is also a pre-existing issue, but does a timeout in send_usb_cmd() 
cause permanent protocol desynchronization?

If wait_for_completion_timeout() times out, send_usb_cmd() returns 
-ETIMEDOUT without marking the completion as done:

send_usb_cmd() {
    ...
    t = wait_for_completion_timeout(&ccp->wait_input_report, ...);
    if (!t)
        return -ETIMEDOUT;
    ...
}

If the delayed hardware response arrives later, after a subsequent command 
has called reinit_completion(), the interrupt handler will accept the stale 
response:

ccp_raw_event() {
    ...
    if (!completion_done(&ccp->wait_input_report)) {
        memcpy(ccp->buffer, data, min(IN_BUFFER_SIZE, size));
        ...
        complete_all(&ccp->wait_input_report);
    }
    ...
}

This causes the new command to consume the stale response, leaving its own 
response pending for the next command, permanently shifting the responses.

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260708015806.213790-1-linux@roeck-us.net?part=1