[PATCH 0/3] hwmon: (corsair-cpro) Fix issues when hidraw is used

[PATCH 0/3] hwmon: (corsair-cpro) Fix issues when hidraw is used

[Aleksa Savic]

This patch series fixes the behavior of the corsair-cpro driver while
hidraw is used from userspace.

The first patch introduces a separate buffer for sending commands to
the device to prevent it from being overwritten thanks to a hidraw
userspace call.

The second patch replaces the complete() call in the raw event parsing
function with complete_all() to signify that the completion is done
until reinit.

The third patch introduces locking for the ccp->wait_input_report
completion as it's touched in functions that could be executing in
parallel.

Aleksa Savic (3):
  hwmon: (corsair-cpro) Use a separate buffer for sending commands
  hwmon: (corsair-cpro) Use complete_all() instead of complete() in
    ccp_raw_event()
  hwmon: (corsair-cpro) Protect ccp->wait_input_report with a spinlock

 drivers/hwmon/corsair-cpro.c | 45 +++++++++++++++++++++++++-----------
 1 file changed, 32 insertions(+), 13 deletions(-)

-- 
2.44.0

[PATCH 1/3] hwmon: (corsair-cpro) Use a separate buffer for sending commands

[Aleksa Savic]

Introduce cmd_buffer, a separate buffer for storing only
the command that is sent to the device. Before this separation,
the existing buffer was shared for both the command and the
report received in ccp_raw_event(), which was copied into it.

However, because of hidraw, the raw event parsing may be triggered
in the middle of sending a command, resulting in outputting gibberish
to the device. Using a separate buffer resolves this.

Fixes: 40c3a4454225 ("hwmon: add Corsair Commander Pro driver")
Signed-off-by: Aleksa Savic <savicaleksa83@gmail.com>
---
 drivers/hwmon/corsair-cpro.c | 19 ++++++++++++-------
 1 file changed, 12 insertions(+), 7 deletions(-)

diff --git a/drivers/hwmon/corsair-cpro.c b/drivers/hwmon/corsair-cpro.c
index a284a02839fb..8d85f66f8143 100644
--- a/drivers/hwmon/corsair-cpro.c
+++ b/drivers/hwmon/corsair-cpro.c
@@ -79,6 +79,7 @@ struct ccp_device {
 	struct device *hwmon_dev;
 	struct completion wait_input_report;
 	struct mutex mutex; /* whenever buffer is used, lock before send_usb_cmd */
+	u8 *cmd_buffer;
 	u8 *buffer;
 	int target[6];
 	DECLARE_BITMAP(temp_cnct, NUM_TEMP_SENSORS);
@@ -111,15 +112,15 @@ static int send_usb_cmd(struct ccp_device *ccp, u8 command, u8 byte1, u8 byte2,
 	unsigned long t;
 	int ret;
 
-	memset(ccp->buffer, 0x00, OUT_BUFFER_SIZE);
-	ccp->buffer[0] = command;
-	ccp->buffer[1] = byte1;
-	ccp->buffer[2] = byte2;
-	ccp->buffer[3] = byte3;
+	memset(ccp->cmd_buffer, 0x00, OUT_BUFFER_SIZE);
+	ccp->cmd_buffer[0] = command;
+	ccp->cmd_buffer[1] = byte1;
+	ccp->cmd_buffer[2] = byte2;
+	ccp->cmd_buffer[3] = byte3;
 
 	reinit_completion(&ccp->wait_input_report);
 
-	ret = hid_hw_output_report(ccp->hdev, ccp->buffer, OUT_BUFFER_SIZE);
+	ret = hid_hw_output_report(ccp->hdev, ccp->cmd_buffer, OUT_BUFFER_SIZE);
 	if (ret < 0)
 		return ret;
 
@@ -492,7 +493,11 @@ static int ccp_probe(struct hid_device *hdev, const struct hid_device_id *id)
 	if (!ccp)
 		return -ENOMEM;
 
-	ccp->buffer = devm_kmalloc(&hdev->dev, OUT_BUFFER_SIZE, GFP_KERNEL);
+	ccp->cmd_buffer = devm_kmalloc(&hdev->dev, OUT_BUFFER_SIZE, GFP_KERNEL);
+	if (!ccp->cmd_buffer)
+		return -ENOMEM;
+
+	ccp->buffer = devm_kmalloc(&hdev->dev, IN_BUFFER_SIZE, GFP_KERNEL);
 	if (!ccp->buffer)
 		return -ENOMEM;
 
-- 
2.44.0

Re: [PATCH 1/3] hwmon: (corsair-cpro) Use a separate buffer for sending commands

[Marius Zachmann]

On 04.05.24 at 11:25:01 MESZ, Aleksa Savic wrote
> Introduce cmd_buffer, a separate buffer for storing only
> the command that is sent to the device. Before this separation,
> the existing buffer was shared for both the command and the
> report received in ccp_raw_event(), which was copied into it.
> 
> However, because of hidraw, the raw event parsing may be triggered
> in the middle of sending a command, resulting in outputting gibberish
> to the device. Using a separate buffer resolves this.
> 
> Fixes: 40c3a4454225 ("hwmon: add Corsair Commander Pro driver")
> Signed-off-by: Aleksa Savic <savicaleksa83@gmail.com>
> ---
>  drivers/hwmon/corsair-cpro.c | 19 ++++++++++++-------
>  1 file changed, 12 insertions(+), 7 deletions(-)
> 
> diff --git a/drivers/hwmon/corsair-cpro.c b/drivers/hwmon/corsair-cpro.c
> index a284a02839fb..8d85f66f8143 100644
> --- a/drivers/hwmon/corsair-cpro.c
> +++ b/drivers/hwmon/corsair-cpro.c
> @@ -79,6 +79,7 @@ struct ccp_device {
>  	struct device *hwmon_dev;
>  	struct completion wait_input_report;
>  	struct mutex mutex; /* whenever buffer is used, lock before send_usb_cmd */
> +	u8 *cmd_buffer;
>  	u8 *buffer;
>  	int target[6];
>  	DECLARE_BITMAP(temp_cnct, NUM_TEMP_SENSORS);
> @@ -111,15 +112,15 @@ static int send_usb_cmd(struct ccp_device *ccp, u8 command, u8 byte1, u8 byte2,
>  	unsigned long t;
>  	int ret;
>  
> -	memset(ccp->buffer, 0x00, OUT_BUFFER_SIZE);
> -	ccp->buffer[0] = command;
> -	ccp->buffer[1] = byte1;
> -	ccp->buffer[2] = byte2;
> -	ccp->buffer[3] = byte3;
> +	memset(ccp->cmd_buffer, 0x00, OUT_BUFFER_SIZE);
> +	ccp->cmd_buffer[0] = command;
> +	ccp->cmd_buffer[1] = byte1;
> +	ccp->cmd_buffer[2] = byte2;
> +	ccp->cmd_buffer[3] = byte3;
>  
>  	reinit_completion(&ccp->wait_input_report);
>  
> -	ret = hid_hw_output_report(ccp->hdev, ccp->buffer, OUT_BUFFER_SIZE);
> +	ret = hid_hw_output_report(ccp->hdev, ccp->cmd_buffer, OUT_BUFFER_SIZE);
>  	if (ret < 0)
>  		return ret;
>  
> @@ -492,7 +493,11 @@ static int ccp_probe(struct hid_device *hdev, const struct hid_device_id *id)
>  	if (!ccp)
>  		return -ENOMEM;
>  
> -	ccp->buffer = devm_kmalloc(&hdev->dev, OUT_BUFFER_SIZE, GFP_KERNEL);
> +	ccp->cmd_buffer = devm_kmalloc(&hdev->dev, OUT_BUFFER_SIZE, GFP_KERNEL);
> +	if (!ccp->cmd_buffer)
> +		return -ENOMEM;
> +
> +	ccp->buffer = devm_kmalloc(&hdev->dev, IN_BUFFER_SIZE, GFP_KERNEL);
>  	if (!ccp->buffer)
>  		return -ENOMEM;
>  
> 

Thank you!
Acked-by: Marius Zachmann <mail@mariuszachmann.de>

[PATCH 2/3] hwmon: (corsair-cpro) Use complete_all() instead of complete() in ccp_raw_event()

[Aleksa Savic]

In ccp_raw_event(), the ccp->wait_input_report completion is
completed once. Since we're waiting for exactly one report in
send_usb_cmd(), use complete_all() instead of complete()
to mark the completion as spent.

Fixes: 40c3a4454225 ("hwmon: add Corsair Commander Pro driver")
Signed-off-by: Aleksa Savic <savicaleksa83@gmail.com>
---
 drivers/hwmon/corsair-cpro.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/drivers/hwmon/corsair-cpro.c b/drivers/hwmon/corsair-cpro.c
index 8d85f66f8143..6ab4d2478b1f 100644
--- a/drivers/hwmon/corsair-cpro.c
+++ b/drivers/hwmon/corsair-cpro.c
@@ -140,7 +140,7 @@ static int ccp_raw_event(struct hid_device *hdev, struct hid_report *report, u8
 		return 0;
 
 	memcpy(ccp->buffer, data, min(IN_BUFFER_SIZE, size));
-	complete(&ccp->wait_input_report);
+	complete_all(&ccp->wait_input_report);
 
 	return 0;
 }
-- 
2.44.0

Re: [PATCH 2/3] hwmon: (corsair-cpro) Use complete_all() instead of complete() in ccp_raw_event()

[Marius Zachmann]

On 04.05.24 at 11:25:02 MESZ, Aleksa Savic wrote
> In ccp_raw_event(), the ccp->wait_input_report completion is
> completed once. Since we're waiting for exactly one report in
> send_usb_cmd(), use complete_all() instead of complete()
> to mark the completion as spent.
> 
> Fixes: 40c3a4454225 ("hwmon: add Corsair Commander Pro driver")
> Signed-off-by: Aleksa Savic <savicaleksa83@gmail.com>
> ---
>  drivers/hwmon/corsair-cpro.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/drivers/hwmon/corsair-cpro.c b/drivers/hwmon/corsair-cpro.c
> index 8d85f66f8143..6ab4d2478b1f 100644
> --- a/drivers/hwmon/corsair-cpro.c
> +++ b/drivers/hwmon/corsair-cpro.c
> @@ -140,7 +140,7 @@ static int ccp_raw_event(struct hid_device *hdev, struct hid_report *report, u8
>  		return 0;
>  
>  	memcpy(ccp->buffer, data, min(IN_BUFFER_SIZE, size));
> -	complete(&ccp->wait_input_report);
> +	complete_all(&ccp->wait_input_report);
>  
>  	return 0;
>  }
> 

Acked-by: Marius Zachmann <mail@mariuszachmann.de>

[PATCH 3/3] hwmon: (corsair-cpro) Protect ccp->wait_input_report with a spinlock

[Aleksa Savic]

Through hidraw, userspace can cause a status report to be sent
from the device. The parsing in ccp_raw_event() may happen in
parallel to a send_usb_cmd() call (which resets the completion
for tracking the report) if it's running on a different CPU where
bottom half interrupts are not disabled.

Add a spinlock around the complete_all() in ccp_raw_event() and
reinit_completion() in send_usb_cmd() to prevent race issues.

Fixes: 40c3a4454225 ("hwmon: add Corsair Commander Pro driver")
Signed-off-by: Aleksa Savic <savicaleksa83@gmail.com>
---
 drivers/hwmon/corsair-cpro.c | 24 +++++++++++++++++++-----
 1 file changed, 19 insertions(+), 5 deletions(-)

diff --git a/drivers/hwmon/corsair-cpro.c b/drivers/hwmon/corsair-cpro.c
index 6ab4d2478b1f..3e63666a61bd 100644
--- a/drivers/hwmon/corsair-cpro.c
+++ b/drivers/hwmon/corsair-cpro.c
@@ -16,6 +16,7 @@
 #include <linux/module.h>
 #include <linux/mutex.h>
 #include <linux/slab.h>
+#include <linux/spinlock.h>
 #include <linux/types.h>
 
 #define USB_VENDOR_ID_CORSAIR			0x1b1c
@@ -77,6 +78,8 @@
 struct ccp_device {
 	struct hid_device *hdev;
 	struct device *hwmon_dev;
+	/* For reinitializing the completion below */
+	spinlock_t wait_input_report_lock;
 	struct completion wait_input_report;
 	struct mutex mutex; /* whenever buffer is used, lock before send_usb_cmd */
 	u8 *cmd_buffer;
@@ -118,7 +121,15 @@ static int send_usb_cmd(struct ccp_device *ccp, u8 command, u8 byte1, u8 byte2,
 	ccp->cmd_buffer[2] = byte2;
 	ccp->cmd_buffer[3] = byte3;
 
+	/*
+	 * Disable raw event parsing for a moment to safely reinitialize the
+	 * completion. Reinit is done because hidraw could have triggered
+	 * the raw event parsing and marked the ccp->wait_input_report
+	 * completion as done.
+	 */
+	spin_lock_bh(&ccp->wait_input_report_lock);
 	reinit_completion(&ccp->wait_input_report);
+	spin_unlock_bh(&ccp->wait_input_report_lock);
 
 	ret = hid_hw_output_report(ccp->hdev, ccp->cmd_buffer, OUT_BUFFER_SIZE);
 	if (ret < 0)
@@ -136,11 +147,12 @@ static int ccp_raw_event(struct hid_device *hdev, struct hid_report *report, u8
 	struct ccp_device *ccp = hid_get_drvdata(hdev);
 
 	/* only copy buffer when requested */
-	if (completion_done(&ccp->wait_input_report))
-		return 0;
-
-	memcpy(ccp->buffer, data, min(IN_BUFFER_SIZE, size));
-	complete_all(&ccp->wait_input_report);
+	spin_lock(&ccp->wait_input_report_lock);
+	if (!completion_done(&ccp->wait_input_report)) {
+		memcpy(ccp->buffer, data, min(IN_BUFFER_SIZE, size));
+		complete_all(&ccp->wait_input_report);
+	}
+	spin_unlock(&ccp->wait_input_report_lock);
 
 	return 0;
 }
@@ -515,7 +527,9 @@ static int ccp_probe(struct hid_device *hdev, const struct hid_device_id *id)
 
 	ccp->hdev = hdev;
 	hid_set_drvdata(hdev, ccp);
+
 	mutex_init(&ccp->mutex);
+	spin_lock_init(&ccp->wait_input_report_lock);
 	init_completion(&ccp->wait_input_report);
 
 	hid_device_io_start(hdev);
-- 
2.44.0

Re: [PATCH 3/3] hwmon: (corsair-cpro) Protect ccp->wait_input_report with a spinlock

[Marius Zachmann]

On 04.05.24 at 11:25:03 MESZ, Aleksa Savic wrote
> Through hidraw, userspace can cause a status report to be sent
> from the device. The parsing in ccp_raw_event() may happen in
> parallel to a send_usb_cmd() call (which resets the completion
> for tracking the report) if it's running on a different CPU where
> bottom half interrupts are not disabled.
> 
> Add a spinlock around the complete_all() in ccp_raw_event() and
> reinit_completion() in send_usb_cmd() to prevent race issues.
> 
> Fixes: 40c3a4454225 ("hwmon: add Corsair Commander Pro driver")
> Signed-off-by: Aleksa Savic <savicaleksa83@gmail.com>
> ---
>  drivers/hwmon/corsair-cpro.c | 24 +++++++++++++++++++-----
>  1 file changed, 19 insertions(+), 5 deletions(-)
> 
> diff --git a/drivers/hwmon/corsair-cpro.c b/drivers/hwmon/corsair-cpro.c
> index 6ab4d2478b1f..3e63666a61bd 100644
> --- a/drivers/hwmon/corsair-cpro.c
> +++ b/drivers/hwmon/corsair-cpro.c
> @@ -16,6 +16,7 @@
>  #include <linux/module.h>
>  #include <linux/mutex.h>
>  #include <linux/slab.h>
> +#include <linux/spinlock.h>
>  #include <linux/types.h>
>  
>  #define USB_VENDOR_ID_CORSAIR			0x1b1c
> @@ -77,6 +78,8 @@
>  struct ccp_device {
>  	struct hid_device *hdev;
>  	struct device *hwmon_dev;
> +	/* For reinitializing the completion below */
> +	spinlock_t wait_input_report_lock;
>  	struct completion wait_input_report;
>  	struct mutex mutex; /* whenever buffer is used, lock before send_usb_cmd */
>  	u8 *cmd_buffer;
> @@ -118,7 +121,15 @@ static int send_usb_cmd(struct ccp_device *ccp, u8 command, u8 byte1, u8 byte2,
>  	ccp->cmd_buffer[2] = byte2;
>  	ccp->cmd_buffer[3] = byte3;
>  
> +	/*
> +	 * Disable raw event parsing for a moment to safely reinitialize the
> +	 * completion. Reinit is done because hidraw could have triggered
> +	 * the raw event parsing and marked the ccp->wait_input_report
> +	 * completion as done.
> +	 */
> +	spin_lock_bh(&ccp->wait_input_report_lock);
>  	reinit_completion(&ccp->wait_input_report);
> +	spin_unlock_bh(&ccp->wait_input_report_lock);
>  
>  	ret = hid_hw_output_report(ccp->hdev, ccp->cmd_buffer, OUT_BUFFER_SIZE);
>  	if (ret < 0)
> @@ -136,11 +147,12 @@ static int ccp_raw_event(struct hid_device *hdev, struct hid_report *report, u8
>  	struct ccp_device *ccp = hid_get_drvdata(hdev);
>  
>  	/* only copy buffer when requested */
> -	if (completion_done(&ccp->wait_input_report))
> -		return 0;
> -
> -	memcpy(ccp->buffer, data, min(IN_BUFFER_SIZE, size));
> -	complete_all(&ccp->wait_input_report);
> +	spin_lock(&ccp->wait_input_report_lock);
> +	if (!completion_done(&ccp->wait_input_report)) {
> +		memcpy(ccp->buffer, data, min(IN_BUFFER_SIZE, size));
> +		complete_all(&ccp->wait_input_report);
> +	}
> +	spin_unlock(&ccp->wait_input_report_lock);
>  
>  	return 0;
>  }
> @@ -515,7 +527,9 @@ static int ccp_probe(struct hid_device *hdev, const struct hid_device_id *id)
>  
>  	ccp->hdev = hdev;
>  	hid_set_drvdata(hdev, ccp);
> +
>  	mutex_init(&ccp->mutex);
> +	spin_lock_init(&ccp->wait_input_report_lock);
>  	init_completion(&ccp->wait_input_report);
>  
>  	hid_device_io_start(hdev);
> 

Acked-by: Marius Zachmann <mail@mariuszachmann.de>

Re: [PATCH 0/3] hwmon: (corsair-cpro) Fix issues when hidraw is used

[Guenter Roeck]

On 5/4/24 02:25, Aleksa Savic wrote:
> This patch series fixes the behavior of the corsair-cpro driver while
> hidraw is used from userspace.
> 
> The first patch introduces a separate buffer for sending commands to
> the device to prevent it from being overwritten thanks to a hidraw
> userspace call.
> 
> The second patch replaces the complete() call in the raw event parsing
> function with complete_all() to signify that the completion is done
> until reinit.
> 
> The third patch introduces locking for the ccp->wait_input_report
> completion as it's touched in functions that could be executing in
> parallel.
> 
> Aleksa Savic (3):
>    hwmon: (corsair-cpro) Use a separate buffer for sending commands
>    hwmon: (corsair-cpro) Use complete_all() instead of complete() in
>      ccp_raw_event()
>    hwmon: (corsair-cpro) Protect ccp->wait_input_report with a spinlock
> 
>   drivers/hwmon/corsair-cpro.c | 45 +++++++++++++++++++++++++-----------
>   1 file changed, 32 insertions(+), 13 deletions(-)
> 

Series applied.

Thanks,
Guenter