diff mbox series

[2/2] HID: hid-steam: Fix cleanup in probe()

Message ID 1fd87904-dabf-4879-bb89-72d13ebfc91e@moroto.mountain
State New
Headers
Series [1/2] HID: hid-steam: remove pointless error message |

Commit Message

Dan Carpenter Jan. 12, 2024, 2:35 p.m. UTC 
  There are a number of issues in this code.  First of all if
steam_create_client_hid() fails then it leads to an error pointer
dereference when we call hid_destroy_device(steam->client_hdev).

Also there are a number of leaks.  hid_hw_stop() is not called if
hid_hw_open() fails for example.  And it doesn't call steam_unregister()
or hid_hw_close().

Fixes: 691ead124a0c ("HID: hid-steam: Clean up locking")
Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
---
This is just from static analysis and code review.  I haven't tested
it.  I only included the fixes tag for the error pointer dereference.

 drivers/hid/hid-steam.c | 26 +++++++++++++++-----------
 1 file changed, 15 insertions(+), 11 deletions(-)

Comments

Vicki Pfau Jan. 13, 2024, 2:11 a.m. UTC | #1 
I have applied this to our downstream and made sure it compiles and runs 
without obvious issues.

Reviewed-by: Vicki Pfau <vi@endrift.com>

Thanks,
Vicki

On 1/12/24 06:35, Dan Carpenter wrote:
> There are a number of issues in this code.  First of all if
> steam_create_client_hid() fails then it leads to an error pointer
> dereference when we call hid_destroy_device(steam->client_hdev).
> 
> Also there are a number of leaks.  hid_hw_stop() is not called if
> hid_hw_open() fails for example.  And it doesn't call steam_unregister()
> or hid_hw_close().
> 
> Fixes: 691ead124a0c ("HID: hid-steam: Clean up locking")
> Signed-off-by: Dan Carpenter <dan.carpenter@linaro.org>
> ---
> This is just from static analysis and code review.  I haven't tested
> it.  I only included the fixes tag for the error pointer dereference.
> 
>   drivers/hid/hid-steam.c | 26 +++++++++++++++-----------
>   1 file changed, 15 insertions(+), 11 deletions(-)
> 
> diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
> index 59df6ead7b54..b08a5ab58528 100644
> --- a/drivers/hid/hid-steam.c
> +++ b/drivers/hid/hid-steam.c
> @@ -1128,14 +1128,14 @@ static int steam_probe(struct hid_device *hdev,
>   	 */
>   	ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT & ~HID_CONNECT_HIDRAW);
>   	if (ret)
> -		goto hid_hw_start_fail;
> +		goto err_cancel_work;
>   
>   	ret = hid_hw_open(hdev);
>   	if (ret) {
>   		hid_err(hdev,
>   			"%s:hid_hw_open\n",
>   			__func__);
> -		goto hid_hw_open_fail;
> +		goto err_hw_stop;
>   	}
>   
>   	if (steam->quirks & STEAM_QUIRK_WIRELESS) {
> @@ -1151,33 +1151,37 @@ static int steam_probe(struct hid_device *hdev,
>   			hid_err(hdev,
>   				"%s:steam_register failed with error %d\n",
>   				__func__, ret);
> -			goto input_register_fail;
> +			goto err_hw_close;
>   		}
>   	}
>   
>   	steam->client_hdev = steam_create_client_hid(hdev);
>   	if (IS_ERR(steam->client_hdev)) {
>   		ret = PTR_ERR(steam->client_hdev);
> -		goto client_hdev_fail;
> +		goto err_stream_unregister;
>   	}
>   	steam->client_hdev->driver_data = steam;
>   
>   	ret = hid_add_device(steam->client_hdev);
>   	if (ret)
> -		goto client_hdev_add_fail;
> +		goto err_destroy;
>   
>   	return 0;
>   
> -client_hdev_add_fail:
> -	hid_hw_stop(hdev);
> -client_hdev_fail:
> +err_destroy:
>   	hid_destroy_device(steam->client_hdev);
> -input_register_fail:
> -hid_hw_open_fail:
> -hid_hw_start_fail:
> +err_stream_unregister:
> +	if (steam->connected)
> +		steam_unregister(steam);
> +err_hw_close:
> +	hid_hw_close(hdev);
> +err_hw_stop:
> +	hid_hw_stop(hdev);
> +err_cancel_work:
>   	cancel_work_sync(&steam->work_connect);
>   	cancel_delayed_work_sync(&steam->mode_switch);
>   	cancel_work_sync(&steam->rumble_work);
> +
>   	return ret;
>   }
>
diff mbox series

Patch

diff --git a/drivers/hid/hid-steam.c b/drivers/hid/hid-steam.c
index 59df6ead7b54..b08a5ab58528 100644
--- a/drivers/hid/hid-steam.c
+++ b/drivers/hid/hid-steam.c
@@ -1128,14 +1128,14 @@  static int steam_probe(struct hid_device *hdev,
 	 */
 	ret = hid_hw_start(hdev, HID_CONNECT_DEFAULT & ~HID_CONNECT_HIDRAW);
 	if (ret)
-		goto hid_hw_start_fail;
+		goto err_cancel_work;
 
 	ret = hid_hw_open(hdev);
 	if (ret) {
 		hid_err(hdev,
 			"%s:hid_hw_open\n",
 			__func__);
-		goto hid_hw_open_fail;
+		goto err_hw_stop;
 	}
 
 	if (steam->quirks & STEAM_QUIRK_WIRELESS) {
@@ -1151,33 +1151,37 @@  static int steam_probe(struct hid_device *hdev,
 			hid_err(hdev,
 				"%s:steam_register failed with error %d\n",
 				__func__, ret);
-			goto input_register_fail;
+			goto err_hw_close;
 		}
 	}
 
 	steam->client_hdev = steam_create_client_hid(hdev);
 	if (IS_ERR(steam->client_hdev)) {
 		ret = PTR_ERR(steam->client_hdev);
-		goto client_hdev_fail;
+		goto err_stream_unregister;
 	}
 	steam->client_hdev->driver_data = steam;
 
 	ret = hid_add_device(steam->client_hdev);
 	if (ret)
-		goto client_hdev_add_fail;
+		goto err_destroy;
 
 	return 0;
 
-client_hdev_add_fail:
-	hid_hw_stop(hdev);
-client_hdev_fail:
+err_destroy:
 	hid_destroy_device(steam->client_hdev);
-input_register_fail:
-hid_hw_open_fail:
-hid_hw_start_fail:
+err_stream_unregister:
+	if (steam->connected)
+		steam_unregister(steam);
+err_hw_close:
+	hid_hw_close(hdev);
+err_hw_stop:
+	hid_hw_stop(hdev);
+err_cancel_work:
 	cancel_work_sync(&steam->work_connect);
 	cancel_delayed_work_sync(&steam->mode_switch);
 	cancel_work_sync(&steam->rumble_work);
+
 	return ret;
 }