linux-kernel - Re: [PATCH 2/2] HID: hid-steam: Fix cleanup in probe()

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite for Android: free password hash cracker in your pocket

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date: Fri, 12 Jan 2024 18:11:05 -0800
From: Vicki Pfau <vi@...rift.com>
To: Dan Carpenter <dan.carpenter@...aro.org>
Cc: Jiri Kosina <jikos@...nel.org>,
 Benjamin Tissoires <benjamin.tissoires@...hat.com>,
 linux-input@...r.kernel.org, linux-kernel@...r.kernel.org,
 kernel-janitors@...r.kernel.org
Subject: Re: [PATCH 2/2] HID: hid-steam: Fix cleanup in probe()

I have applied this to our downstream and made sure it compiles and runs 
without obvious issues.

Reviewed-by: Vicki Pfau <vi@...rift.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@...aro.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;
>   }
>