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
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 <[email protected]>
---
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
Hi Hanno,
On Mon, Feb 17, 2020 at 4:24 PM Hanno Zulla <[email protected]> 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
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
On Tue, Feb 18, 2020 at 12:44 PM Hanno Zulla <[email protected]> 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
>