* [PATCH 0/3] HID: hid-bigbenff: fixing three crash bugs in a gamepad driver @ 2020-02-17 15:24 Hanno Zulla 2020-02-17 15:25 ` [PATCH 1/3] HID: hid-bigbenff: fix general protection fault caused by double kfree Hanno Zulla 2020-02-18 10:40 ` [PATCH 0/3] HID: hid-bigbenff: fixing three crash bugs in a gamepad driver Benjamin Tissoires 0 siblings, 2 replies; 10+ messages in thread From: Hanno Zulla @ 2020-02-17 15:24 UTC (permalink / raw) To: Jiri Kosina, Benjamin Tissoires, linux-input, linux-kernel Hi there, the hid-bigbenff.c had three bugs causing possible kernel crashes. The first patch fixes a double free during device removal, which was caused by a wrong use of input_ff_create_memless(). The "driver-specific data to be passed into play_effect" parameter of input_ff_create_memless() would later be freed automatically when the ff device is removed. Since the driver also uses the managed resource API, it would automatically free the memory of this parameter twice, causing a general protection fault moments later. The second patch fixes the error path after hid_hw_start(), as a call to hid_hw_stop() is required in case of an error. The second patch also removes the hid_hw_close() call during device removal, as several other hid device drivers don't call this routine, either. The third patch adds a flag to avoid a race condition when there is still scheduled work left (or newly being scheduled) during or after device removal, which could cause a kernel crash. Thanks in advance for your review & kind regards, Hanno ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/3] HID: hid-bigbenff: fix general protection fault caused by double kfree 2020-02-17 15:24 [PATCH 0/3] HID: hid-bigbenff: fixing three crash bugs in a gamepad driver Hanno Zulla @ 2020-02-17 15:25 ` Hanno Zulla 2020-02-17 15:26 ` [PATCH 2/3] HID: hid-bigbenff: call hid_hw_stop() in case of error Hanno Zulla 2020-02-18 10:40 ` [PATCH 0/3] HID: hid-bigbenff: fixing three crash bugs in a gamepad driver Benjamin Tissoires 1 sibling, 1 reply; 10+ messages in thread From: Hanno Zulla @ 2020-02-17 15:25 UTC (permalink / raw) To: Jiri Kosina, Benjamin Tissoires, linux-input, linux-kernel HID: hid-bigbenff: fix general protection fault caused by double kfree The struct *bigben was allocated via devm_kzalloc() and then used as a parameter in input_ff_create_memless(). This caused a double kfree during removal of the device, since both the managed resource API and ml_ff_destroy() in drivers/input/ff-memless.c would call kfree() on it. Signed-off-by: Hanno Zulla <kontakt@hanno.de> --- drivers/hid/hid-bigbenff.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/drivers/hid/hid-bigbenff.c b/drivers/hid/hid-bigbenff.c index 3f6abd190df4..f7e85bacb688 100644 --- a/drivers/hid/hid-bigbenff.c +++ b/drivers/hid/hid-bigbenff.c @@ -220,10 +220,16 @@ static void bigben_worker(struct work_struct *work) static int hid_bigben_play_effect(struct input_dev *dev, void *data, struct ff_effect *effect) { - struct bigben_device *bigben = data; + struct hid_device *hid = input_get_drvdata(dev); + struct bigben_device *bigben = hid_get_drvdata(hid); u8 right_motor_on; u8 left_motor_force; + if (!bigben) { + hid_err(hid, "no device data\n"); + return 0; + } + if (effect->type != FF_RUMBLE) return 0; @@ -341,7 +347,7 @@ static int bigben_probe(struct hid_device *hid, INIT_WORK(&bigben->worker, bigben_worker); - error = input_ff_create_memless(hidinput->input, bigben, + error = input_ff_create_memless(hidinput->input, NULL, hid_bigben_play_effect); if (error) return error; -- 2.20.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 2/3] HID: hid-bigbenff: call hid_hw_stop() in case of error 2020-02-17 15:25 ` [PATCH 1/3] HID: hid-bigbenff: fix general protection fault caused by double kfree Hanno Zulla @ 2020-02-17 15:26 ` Hanno Zulla 2020-02-17 15:27 ` [PATCH 3/3] HID: hid-bigbenff: fix race condition for scheduled work during removal Hanno Zulla 0 siblings, 1 reply; 10+ messages in thread From: Hanno Zulla @ 2020-02-17 15:26 UTC (permalink / raw) To: Jiri Kosina, Benjamin Tissoires, linux-input, linux-kernel [PATCH 2/3] HID: hid-bigbenff: call hid_hw_stop() in case of error It's required to call hid_hw_stop() once hid_hw_start() was called previously, so error cases need to handle this. Also, hid_hw_close() is not necessary during removal. Signed-off-by: Hanno Zulla <kontakt@hanno.de> --- drivers/hid/hid-bigbenff.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/drivers/hid/hid-bigbenff.c b/drivers/hid/hid-bigbenff.c index f7e85bacb688..f8c552b64a89 100644 --- a/drivers/hid/hid-bigbenff.c +++ b/drivers/hid/hid-bigbenff.c @@ -305,7 +305,6 @@ static void bigben_remove(struct hid_device *hid) struct bigben_device *bigben = hid_get_drvdata(hid); cancel_work_sync(&bigben->worker); - hid_hw_close(hid); hid_hw_stop(hid); } @@ -350,7 +349,7 @@ static int bigben_probe(struct hid_device *hid, error = input_ff_create_memless(hidinput->input, NULL, hid_bigben_play_effect); if (error) - return error; + goto error_hw_stop; name_sz = strlen(dev_name(&hid->dev)) + strlen(":red:bigben#") + 1; @@ -360,8 +359,10 @@ static int bigben_probe(struct hid_device *hid, sizeof(struct led_classdev) + name_sz, GFP_KERNEL ); - if (!led) - return -ENOMEM; + if (!led) { + error = -ENOMEM; + goto error_hw_stop; + } name = (void *)(&led[1]); snprintf(name, name_sz, "%s:red:bigben%d", @@ -375,7 +376,7 @@ static int bigben_probe(struct hid_device *hid, bigben->leds[n] = led; error = devm_led_classdev_register(&hid->dev, led); if (error) - return error; + goto error_hw_stop; } /* initial state: LED1 is on, no rumble effect */ @@ -389,6 +390,10 @@ static int bigben_probe(struct hid_device *hid, hid_info(hid, "LED and force feedback support for BigBen gamepad\n"); return 0; + +error_hw_stop: + hid_hw_stop(hid); + return error; } static __u8 *bigben_report_fixup(struct hid_device *hid, __u8 *rdesc, -- 2.20.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH 3/3] HID: hid-bigbenff: fix race condition for scheduled work during removal 2020-02-17 15:26 ` [PATCH 2/3] HID: hid-bigbenff: call hid_hw_stop() in case of error Hanno Zulla @ 2020-02-17 15:27 ` Hanno Zulla 0 siblings, 0 replies; 10+ messages in thread From: Hanno Zulla @ 2020-02-17 15:27 UTC (permalink / raw) To: Jiri Kosina, Benjamin Tissoires, linux-input, linux-kernel HID: hid-bigbenff: fix race condition for scheduled work during removal It's possible that there is scheduled work left while the device is already being removed, which can cause a kernel crash. Adding a flag will avoid this. Signed-off-by: Hanno Zulla <kontakt@hanno.de> --- drivers/hid/hid-bigbenff.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/drivers/hid/hid-bigbenff.c b/drivers/hid/hid-bigbenff.c index f8c552b64a89..db6da21ade06 100644 --- a/drivers/hid/hid-bigbenff.c +++ b/drivers/hid/hid-bigbenff.c @@ -174,6 +174,7 @@ static __u8 pid0902_rdesc_fixed[] = { struct bigben_device { struct hid_device *hid; struct hid_report *report; + bool removed; u8 led_state; /* LED1 = 1 .. LED4 = 8 */ u8 right_motor_on; /* right motor off/on 0/1 */ u8 left_motor_force; /* left motor force 0-255 */ @@ -190,6 +191,9 @@ static void bigben_worker(struct work_struct *work) struct bigben_device, worker); struct hid_field *report_field = bigben->report->field[0]; + if (bigben->removed) + return; + if (bigben->work_led) { bigben->work_led = false; report_field->value[0] = 0x01; /* 1 = led message */ @@ -304,6 +308,7 @@ static void bigben_remove(struct hid_device *hid) { struct bigben_device *bigben = hid_get_drvdata(hid); + bigben->removed = true; cancel_work_sync(&bigben->worker); hid_hw_stop(hid); } @@ -324,6 +329,7 @@ static int bigben_probe(struct hid_device *hid, return -ENOMEM; hid_set_drvdata(hid, bigben); bigben->hid = hid; + bigben->removed = false; error = hid_parse(hid); if (error) { -- 2.20.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 0/3] HID: hid-bigbenff: fixing three crash bugs in a gamepad driver 2020-02-17 15:24 [PATCH 0/3] HID: hid-bigbenff: fixing three crash bugs in a gamepad driver Hanno Zulla 2020-02-17 15:25 ` [PATCH 1/3] HID: hid-bigbenff: fix general protection fault caused by double kfree Hanno Zulla @ 2020-02-18 10:40 ` Benjamin Tissoires 2020-02-18 11:30 ` [PATCH v2 " Hanno Zulla 1 sibling, 1 reply; 10+ messages in thread From: Benjamin Tissoires @ 2020-02-18 10:40 UTC (permalink / raw) To: Hanno Zulla; +Cc: Jiri Kosina, open list:HID CORE LAYER, lkml Hi Hanno, On Mon, Feb 17, 2020 at 4:24 PM Hanno Zulla <abos@hanno.de> wrote: > > Hi there, > > the hid-bigbenff.c had three bugs causing possible kernel crashes. > > The first patch fixes a double free during device removal, which was > caused by a wrong use of input_ff_create_memless(). The > "driver-specific data to be passed into play_effect" parameter of > input_ff_create_memless() would later be freed automatically when the ff > device is removed. Since the driver also uses the managed resource API, > it would automatically free the memory of this parameter twice, causing > a general protection fault moments later. > > The second patch fixes the error path after hid_hw_start(), as a call > to hid_hw_stop() is required in case of an error. > > The second patch also removes the hid_hw_close() call during device > removal, as several other hid device drivers don't call this routine, > either. > > The third patch adds a flag to avoid a race condition when there is > still scheduled work left (or newly being scheduled) during or after > device removal, which could cause a kernel crash. > > Thanks in advance for your review & kind regards, > I think the patches are correct (have you tested them with actual HW?). However, checkpatch complains that the From and Signed-off-by email differ. Can you send a v2 with a fix for that? Cheers, Benjamin ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2 0/3] HID: hid-bigbenff: fixing three crash bugs in a gamepad driver 2020-02-18 10:40 ` [PATCH 0/3] HID: hid-bigbenff: fixing three crash bugs in a gamepad driver Benjamin Tissoires @ 2020-02-18 11:30 ` Hanno Zulla 2020-02-18 11:37 ` [PATCH v2 1/3] HID: hid-bigbenff: fix general protection fault caused by double kfree Hanno Zulla 2020-02-19 7:54 ` [PATCH v2 0/3] HID: hid-bigbenff: fixing three crash bugs in a gamepad driver Benjamin Tissoires 0 siblings, 2 replies; 10+ messages in thread From: Hanno Zulla @ 2020-02-18 11:30 UTC (permalink / raw) To: Benjamin Tissoires; +Cc: Jiri Kosina, open list:HID CORE LAYER, lkml Hi Benjamin, > I think the patches are correct (have you tested them with actual HW?). Yes, I did, and am also properly embarrassed that I didn't notice the double free bug in the original driver. > However, checkpatch complains that the From and Signed-off-by email > differ. Can you send a v2 with a fix for that? Here it is. Thanks, Hanno ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH v2 1/3] HID: hid-bigbenff: fix general protection fault caused by double kfree 2020-02-18 11:30 ` [PATCH v2 " Hanno Zulla @ 2020-02-18 11:37 ` Hanno Zulla 2020-02-18 11:38 ` [PATCH v2 2/3] HID: hid-bigbenff: call hid_hw_stop() in case of error Hanno Zulla 2020-02-19 7:54 ` [PATCH v2 0/3] HID: hid-bigbenff: fixing three crash bugs in a gamepad driver Benjamin Tissoires 1 sibling, 1 reply; 10+ messages in thread From: Hanno Zulla @ 2020-02-18 11:37 UTC (permalink / raw) To: Benjamin Tissoires; +Cc: Jiri Kosina, open list:HID CORE LAYER, lkml HID: hid-bigbenff: fix general protection fault caused by double kfree The struct *bigben was allocated via devm_kzalloc() and then used as a parameter in input_ff_create_memless(). This caused a double kfree during removal of the device, since both the managed resource API and ml_ff_destroy() in drivers/input/ff-memless.c would call kfree() on it. Signed-off-by: Hanno Zulla <kontakt@hanno.de> --- drivers/hid/hid-bigbenff.c | 10 ++++++++-- 1 file changed, 8 insertions(+), 2 deletions(-) diff --git a/drivers/hid/hid-bigbenff.c b/drivers/hid/hid-bigbenff.c index 3f6abd190df4..f7e85bacb688 100644 --- a/drivers/hid/hid-bigbenff.c +++ b/drivers/hid/hid-bigbenff.c @@ -220,10 +220,16 @@ static void bigben_worker(struct work_struct *work) static int hid_bigben_play_effect(struct input_dev *dev, void *data, struct ff_effect *effect) { - struct bigben_device *bigben = data; + struct hid_device *hid = input_get_drvdata(dev); + struct bigben_device *bigben = hid_get_drvdata(hid); u8 right_motor_on; u8 left_motor_force; + if (!bigben) { + hid_err(hid, "no device data\n"); + return 0; + } + if (effect->type != FF_RUMBLE) return 0; @@ -341,7 +347,7 @@ static int bigben_probe(struct hid_device *hid, INIT_WORK(&bigben->worker, bigben_worker); - error = input_ff_create_memless(hidinput->input, bigben, + error = input_ff_create_memless(hidinput->input, NULL, hid_bigben_play_effect); if (error) return error; -- 2.20.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v2 2/3] HID: hid-bigbenff: call hid_hw_stop() in case of error 2020-02-18 11:37 ` [PATCH v2 1/3] HID: hid-bigbenff: fix general protection fault caused by double kfree Hanno Zulla @ 2020-02-18 11:38 ` Hanno Zulla 2020-02-18 11:39 ` [PATCH v2 3/3] HID: hid-bigbenff: fix race condition for scheduled work during removal Hanno Zulla 0 siblings, 1 reply; 10+ messages in thread From: Hanno Zulla @ 2020-02-18 11:38 UTC (permalink / raw) To: Benjamin Tissoires; +Cc: Jiri Kosina, open list:HID CORE LAYER, lkml HID: hid-bigbenff: call hid_hw_stop() in case of error It's required to call hid_hw_stop() once hid_hw_start() was called previously, so error cases need to handle this. Also, hid_hw_close() is not necessary during removal. Signed-off-by: Hanno Zulla <kontakt@hanno.de> --- drivers/hid/hid-bigbenff.c | 15 ++++++++++----- 1 file changed, 10 insertions(+), 5 deletions(-) diff --git a/drivers/hid/hid-bigbenff.c b/drivers/hid/hid-bigbenff.c index f7e85bacb688..f8c552b64a89 100644 --- a/drivers/hid/hid-bigbenff.c +++ b/drivers/hid/hid-bigbenff.c @@ -305,7 +305,6 @@ static void bigben_remove(struct hid_device *hid) struct bigben_device *bigben = hid_get_drvdata(hid); cancel_work_sync(&bigben->worker); - hid_hw_close(hid); hid_hw_stop(hid); } @@ -350,7 +349,7 @@ static int bigben_probe(struct hid_device *hid, error = input_ff_create_memless(hidinput->input, NULL, hid_bigben_play_effect); if (error) - return error; + goto error_hw_stop; name_sz = strlen(dev_name(&hid->dev)) + strlen(":red:bigben#") + 1; @@ -360,8 +359,10 @@ static int bigben_probe(struct hid_device *hid, sizeof(struct led_classdev) + name_sz, GFP_KERNEL ); - if (!led) - return -ENOMEM; + if (!led) { + error = -ENOMEM; + goto error_hw_stop; + } name = (void *)(&led[1]); snprintf(name, name_sz, "%s:red:bigben%d", @@ -375,7 +376,7 @@ static int bigben_probe(struct hid_device *hid, bigben->leds[n] = led; error = devm_led_classdev_register(&hid->dev, led); if (error) - return error; + goto error_hw_stop; } /* initial state: LED1 is on, no rumble effect */ @@ -389,6 +390,10 @@ static int bigben_probe(struct hid_device *hid, hid_info(hid, "LED and force feedback support for BigBen gamepad\n"); return 0; + +error_hw_stop: + hid_hw_stop(hid); + return error; } static __u8 *bigben_report_fixup(struct hid_device *hid, __u8 *rdesc, -- 2.20.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* [PATCH v2 3/3] HID: hid-bigbenff: fix race condition for scheduled work during removal 2020-02-18 11:38 ` [PATCH v2 2/3] HID: hid-bigbenff: call hid_hw_stop() in case of error Hanno Zulla @ 2020-02-18 11:39 ` Hanno Zulla 0 siblings, 0 replies; 10+ messages in thread From: Hanno Zulla @ 2020-02-18 11:39 UTC (permalink / raw) To: Benjamin Tissoires; +Cc: Jiri Kosina, open list:HID CORE LAYER, lkml HID: hid-bigbenff: fix race condition for scheduled work during removal It's possible that there is scheduled work left while the device is already being removed, which can cause a kernel crash. Adding a flag will avoid this. Signed-off-by: Hanno Zulla <kontakt@hanno.de> --- drivers/hid/hid-bigbenff.c | 6 ++++++ 1 file changed, 6 insertions(+) diff --git a/drivers/hid/hid-bigbenff.c b/drivers/hid/hid-bigbenff.c index f8c552b64a89..db6da21ade06 100644 --- a/drivers/hid/hid-bigbenff.c +++ b/drivers/hid/hid-bigbenff.c @@ -174,6 +174,7 @@ static __u8 pid0902_rdesc_fixed[] = { struct bigben_device { struct hid_device *hid; struct hid_report *report; + bool removed; u8 led_state; /* LED1 = 1 .. LED4 = 8 */ u8 right_motor_on; /* right motor off/on 0/1 */ u8 left_motor_force; /* left motor force 0-255 */ @@ -190,6 +191,9 @@ static void bigben_worker(struct work_struct *work) struct bigben_device, worker); struct hid_field *report_field = bigben->report->field[0]; + if (bigben->removed) + return; + if (bigben->work_led) { bigben->work_led = false; report_field->value[0] = 0x01; /* 1 = led message */ @@ -304,6 +308,7 @@ static void bigben_remove(struct hid_device *hid) { struct bigben_device *bigben = hid_get_drvdata(hid); + bigben->removed = true; cancel_work_sync(&bigben->worker); hid_hw_stop(hid); } @@ -324,6 +329,7 @@ static int bigben_probe(struct hid_device *hid, return -ENOMEM; hid_set_drvdata(hid, bigben); bigben->hid = hid; + bigben->removed = false; error = hid_parse(hid); if (error) { -- 2.20.1 ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH v2 0/3] HID: hid-bigbenff: fixing three crash bugs in a gamepad driver 2020-02-18 11:30 ` [PATCH v2 " Hanno Zulla 2020-02-18 11:37 ` [PATCH v2 1/3] HID: hid-bigbenff: fix general protection fault caused by double kfree Hanno Zulla @ 2020-02-19 7:54 ` Benjamin Tissoires 1 sibling, 0 replies; 10+ messages in thread From: Benjamin Tissoires @ 2020-02-19 7:54 UTC (permalink / raw) To: Hanno Zulla; +Cc: Jiri Kosina, open list:HID CORE LAYER, lkml On Tue, Feb 18, 2020 at 12:44 PM Hanno Zulla <kontakt@hanno.de> wrote: > > Hi Benjamin, > > > I think the patches are correct (have you tested them with actual HW?). > > Yes, I did, and am also properly embarrassed that I didn't notice the > double free bug in the original driver. > > > However, checkpatch complains that the From and Signed-off-by email > > differ. Can you send a v2 with a fix for that? > > Here it is. > Thanks for the quick respin. Not sure what happened, but the commit title was duplicated in all of the commits. Anyway, not a big deal, fixed and pushed to for-5.6/upstream-fixes Cheers, Benjamin > Thanks, > > Hanno > ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-02-19 7:55 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2020-02-17 15:24 [PATCH 0/3] HID: hid-bigbenff: fixing three crash bugs in a gamepad driver Hanno Zulla 2020-02-17 15:25 ` [PATCH 1/3] HID: hid-bigbenff: fix general protection fault caused by double kfree Hanno Zulla 2020-02-17 15:26 ` [PATCH 2/3] HID: hid-bigbenff: call hid_hw_stop() in case of error Hanno Zulla 2020-02-17 15:27 ` [PATCH 3/3] HID: hid-bigbenff: fix race condition for scheduled work during removal Hanno Zulla 2020-02-18 10:40 ` [PATCH 0/3] HID: hid-bigbenff: fixing three crash bugs in a gamepad driver Benjamin Tissoires 2020-02-18 11:30 ` [PATCH v2 " Hanno Zulla 2020-02-18 11:37 ` [PATCH v2 1/3] HID: hid-bigbenff: fix general protection fault caused by double kfree Hanno Zulla 2020-02-18 11:38 ` [PATCH v2 2/3] HID: hid-bigbenff: call hid_hw_stop() in case of error Hanno Zulla 2020-02-18 11:39 ` [PATCH v2 3/3] HID: hid-bigbenff: fix race condition for scheduled work during removal Hanno Zulla 2020-02-19 7:54 ` [PATCH v2 0/3] HID: hid-bigbenff: fixing three crash bugs in a gamepad driver Benjamin Tissoires
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).