| Message ID | 1fd87904-dabf-4879-bb89-72d13ebfc91e@moroto.mountain (mailing list archive) |
|---|---|
| State | New |
| Delegated to: | Jiri Kosina |
| Headers | show |
| Series | [1/2] HID: hid-steam: remove pointless error message | expand |
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 --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; }
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(-)