lkml.org 
[lkml]   [2024]   [Jan]   [12]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
SubjectRe: [PATCH 2/2] HID: hid-steam: Fix cleanup in probe()
From
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;
> }
>

\
 
 \ /
  Last update: 2024-01-13 03:24    [W:0.022 / U:0.324 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site