[PATCH] hwmon: (corsair-cpro) Remove debugfs entries when probe fails

[PATCH] hwmon: (corsair-cpro) Remove debugfs entries when probe fails

[Linmao Li]

ccp_debugfs_init() registers debugfs files whose private data is the devm
allocated ccp.  If hwmon_device_register_with_info() fails right after it,
ccp_probe() returns without removing them: the HID core then frees ccp,
and ccp_remove() is not called for a failed probe, so the files stay
behind.  Reading one of them dereferences the freed pointer.

Remove the debugfs entries on that error path.  debugfs_remove_recursive()
waits for readers already inside the show callbacks, so ccp is no longer
reachable through debugfs by the time probe returns.

Reported-by: Sashiko <sashiko-bot@kernel.org>
Closes: https://lore.kernel.org/linux-hwmon/20260708031612.BD7E61F000E9@smtp.kernel.org/
Fixes: 5997eb60f896 ("hwmon: (corsair-cpro) Add firmware and bootloader information")
Signed-off-by: Linmao Li <lilinmao@kylinos.cn>
---
 drivers/hwmon/corsair-cpro.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/drivers/hwmon/corsair-cpro.c b/drivers/hwmon/corsair-cpro.c
index 8354a002f4c5..30fe5d8e724c 100644
--- a/drivers/hwmon/corsair-cpro.c
+++ b/drivers/hwmon/corsair-cpro.c
@@ -638,11 +638,13 @@ static int ccp_probe(struct hid_device *hdev, const struct hid_device_id *id)
 							 ccp, &ccp_chip_info, NULL);
 	if (IS_ERR(ccp->hwmon_dev)) {
 		ret = PTR_ERR(ccp->hwmon_dev);
-		goto out_hw_close;
+		goto out_debugfs_remove;
 	}
 
 	return 0;
 
+out_debugfs_remove:
+	debugfs_remove_recursive(ccp->debugfs);
 out_hw_close:
 	hid_hw_close(hdev);
 	hid_device_io_stop(hdev);

base-commit: 8d3ae59288f1e7d58d76558a6ee96d533bc5019f
-- 
2.25.1

Re: [PATCH] hwmon: (corsair-cpro) Remove debugfs entries when probe fails

[sashiko-bot]

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

Pre-existing issues:
- [High] Late USB responses to timed-out commands complete newly initialized completions, causing subsequent commands to read stale data.
--

commit d32f451c210791b2b9c123593fe5a6e2e781620f
Author: Linmao Li <lilinmao@kylinos.cn>

hwmon: (corsair-cpro) Remove debugfs entries when probe fails

This commit fixes a use-after-free issue in the corsair-cpro driver. It
ensures that debugfs entries are properly removed if hwmon device
registration fails during probe, preventing dangling pointers when the
debugfs files are later accessed.

Closes: https://lore.kernel.org/linux-hwmon/20260708031612.BD7E61F000E9@smtp.kernel.org/

> diff --git a/drivers/hwmon/corsair-cpro.c b/drivers/hwmon/corsair-cpro.c
> index 8354a002f4c5e..30fe5d8e724c6 100644
> --- a/drivers/hwmon/corsair-cpro.c
> +++ b/drivers/hwmon/corsair-cpro.c
> @@ -638,11 +638,13 @@ static int ccp_probe(struct hid_device *hdev, const struct hid_device_id *id)
>  							 ccp, &ccp_chip_info, NULL);
>  	if (IS_ERR(ccp->hwmon_dev)) {
>  		ret = PTR_ERR(ccp->hwmon_dev);
> -		goto out_hw_close;
> +		goto out_debugfs_remove;
>  	}
>  
>  	return 0;
>  
> +out_debugfs_remove:
> +	debugfs_remove_recursive(ccp->debugfs);
>  out_hw_close:
>  	hid_hw_close(hdev);
>  	hid_device_io_stop(hdev);

[Severity: High]
This is a pre-existing issue, but while reviewing the driver I noticed a
potential synchronization problem between commands and responses.

Can late USB responses to timed-out commands complete newly initialized
completions, causing subsequent commands to read stale data?

For example, when a command is sent:

drivers/hwmon/corsair-cpro.c:send_usb_cmd() {
	...
	spin_lock_bh(&ccp->wait_input_report_lock);
	reinit_completion(&ccp->wait_input_report);
	spin_unlock_bh(&ccp->wait_input_report_lock);
	...
}

And when a response arrives:

drivers/hwmon/corsair-cpro.c:ccp_raw_event() {
	...
	spin_lock(&ccp->wait_input_report_lock);
	if (!completion_done(&ccp->wait_input_report)) {
		memcpy(ccp->buffer, data, min(IN_BUFFER_SIZE, size));
		ccp->buffer_recv_size = size;
		complete_all(&ccp->wait_input_report);
	}
	spin_unlock(&ccp->wait_input_report_lock);
	...
}

If a USB command is sent but the device is slow, the completion wait
might time out. If a subsequent command is then sent, reinit_completion()
is called. This opens a window where a late response from the first
command will be accepted.

Because the protocol lacks sequence IDs, the late response would complete
the newly initialized wait and copy its stale data to ccp->buffer. The new
command would then immediately read this stale data. This could lead to
permanent desynchronization of the command/response queue, resulting in
incorrect sensor readings (such as reading the wrong channel's temperature
or returning garbage data).

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20260828061949.3151191-1-lilinmao@kylinos.cn?part=1

Re: [PATCH] hwmon: (corsair-cpro) Remove debugfs entries when probe fails

[Guenter Roeck]

On 8/27/26 23:19, Linmao Li wrote:
> ccp_debugfs_init() registers debugfs files whose private data is the devm
> allocated ccp.  If hwmon_device_register_with_info() fails right after it,
> ccp_probe() returns without removing them: the HID core then frees ccp,
> and ccp_remove() is not called for a failed probe, so the files stay
> behind.  Reading one of them dereferences the freed pointer.
> 
> Remove the debugfs entries on that error path.  debugfs_remove_recursive()
> waits for readers already inside the show callbacks, so ccp is no longer
> reachable through debugfs by the time probe returns.
> 
> Reported-by: Sashiko <sashiko-bot@kernel.org>
> Closes: https://lore.kernel.org/linux-hwmon/20260708031612.BD7E61F000E9@smtp.kernel.org/
> Fixes: 5997eb60f896 ("hwmon: (corsair-cpro) Add firmware and bootloader information")
> Signed-off-by: Linmao Li <lilinmao@kylinos.cn>
> ---
>   drivers/hwmon/corsair-cpro.c | 4 +++-
>   1 file changed, 3 insertions(+), 1 deletion(-)
> 
> diff --git a/drivers/hwmon/corsair-cpro.c b/drivers/hwmon/corsair-cpro.c
> index 8354a002f4c5..30fe5d8e724c 100644
> --- a/drivers/hwmon/corsair-cpro.c
> +++ b/drivers/hwmon/corsair-cpro.c
> @@ -638,11 +638,13 @@ static int ccp_probe(struct hid_device *hdev, const struct hid_device_id *id)
>   							 ccp, &ccp_chip_info, NULL);
>   	if (IS_ERR(ccp->hwmon_dev)) {
>   		ret = PTR_ERR(ccp->hwmon_dev);
> -		goto out_hw_close;
> +		goto out_debugfs_remove;
>   	}
>   

Any reason to not move the call to ccp_debugfs_init() instead ?

Thanks,
Guenter

>   	return 0;
>   
> +out_debugfs_remove:
> +	debugfs_remove_recursive(ccp->debugfs);
>   out_hw_close:
>   	hid_hw_close(hdev);
>   	hid_device_io_stop(hdev);
> 
> base-commit: 8d3ae59288f1e7d58d76558a6ee96d533bc5019f