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

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

Posted by Linmao Li 4 weeks, 1 day ago

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

Posted by Guenter Roeck 3 weeks, 3 days ago

On Fri, Aug 28, 2026 at 02:19:49PM +0800, 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>

Applied.

Thanks,
Guenter

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

Posted by Guenter Roeck 4 weeks, 1 day ago

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

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

Posted by Linmao Li 3 weeks, 5 days ago

在 2026/8/28 21:14, 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 ?
ccp_debugfs_init() also calls get_fw_version() and get_bl_version(), which
send USB commands without taking ccp->mutex. That is fine where the call
sits now, because nothing else calls send_usb_cmd() until the hwmon device
is registered. Moving it unchanged past the registration would let the
hwmon callbacks race with those two reads on ccp->cmd_buffer, ccp->buffer
and wait_input_report.

I will split the version queries from the debugfs creation in v2: the
queries stay where they are, and the debugfs entries are created only
after hwmon_device_register_with_info() has succeeded, which removes the
error path altogether.

Thanks,
Linmao
>
> 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