| Message ID | 20230125-hid-unregister-leds-v3-1-0a52ac225e00@diag.uniroma1.it (mailing list archive) |
|---|---|
| State | Superseded |
| Delegated to: | Jiri Kosina |
| Headers | show |
| Series | HID: use spinlocks to safely schedule led workers | expand |
On Fri, Feb 10, 2023 at 2:24 PM Hillf Danton <hdanton@sina.com> wrote: > > On Thu, 09 Feb 2023 23:58:55 +0000 Pietro Borrello <borrello@diag.uniroma1.it> > > Use spinlocks to deal with workers introducing a wrapper > > bigben_schedule_work(), and several spinlock checks. > > Otherwise, bigben_set_led() may schedule bigben->worker after the > > structure has been freed, causing a use-after-free. > > > > Fixes: 4eb1b01de5b9 ("HID: hid-bigbenff: fix race condition for scheduled work during removal") > > Given the flag added in 4eb1b01de5b9 and the spinlock added in this > patchset, devm_led_classdev_register() looks to not work for you. Actually, looking at the code now, it is clear that we need that lock. The current code is happily changing the struct bigben_device from multiple contexts, and pulls that without any barrier in the work struct which should produce some interesting results :) And we can probably abuse that lock to prevent scheduling a new work as it is done in hid-playstation.c I'll comment in the patch which parts need to be changed, because it is true that this patch is definitely not mergeable as such and will need another revision. > > How about replacing the advanced devm_ method with the traditional plain > pair of led_classdev_un/register(), with the flag mentioned cut off but > without bothering to add another lock? > As mentioned above, the lock is needed anyway, and will probably need to be added in a separate patch. Reverting to a non devm version of the led class would complexify the driver for the error paths, and is probably not the best move IMO. Cheers, Benjamin
On Feb 09 2023, Pietro Borrello wrote: > Use spinlocks to deal with workers introducing a wrapper > bigben_schedule_work(), and several spinlock checks. > Otherwise, bigben_set_led() may schedule bigben->worker after the > structure has been freed, causing a use-after-free. > > Fixes: 4eb1b01de5b9 ("HID: hid-bigbenff: fix race condition for scheduled work during removal") > Signed-off-by: Pietro Borrello <borrello@diag.uniroma1.it> > --- > drivers/hid/hid-bigbenff.c | 34 +++++++++++++++++++++++++++++----- > 1 file changed, 29 insertions(+), 5 deletions(-) > > diff --git a/drivers/hid/hid-bigbenff.c b/drivers/hid/hid-bigbenff.c > index e8b16665860d..28769aa7fed6 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; > + spinlock_t lock; > bool removed; > u8 led_state; /* LED1 = 1 .. LED4 = 8 */ > u8 right_motor_on; /* right motor off/on 0/1 */ > @@ -184,15 +185,24 @@ struct bigben_device { > struct work_struct worker; > }; > > +static inline void bigben_schedule_work(struct bigben_device *bigben) > +{ > + unsigned long flags; > + > + spin_lock_irqsave(&bigben->lock, flags); > + if (!bigben->removed) > + schedule_work(&bigben->worker); > + spin_unlock_irqrestore(&bigben->lock, flags); > +} > > static void bigben_worker(struct work_struct *work) > { > struct bigben_device *bigben = container_of(work, > struct bigben_device, worker); > struct hid_field *report_field = bigben->report->field[0]; > + unsigned long flags; > > - if (bigben->removed || !report_field) You are removing an important test here: if (!report_field), please keep it. > - return; > + spin_lock_irqsave(&bigben->lock, flags); > > if (bigben->work_led) { > bigben->work_led = false; > @@ -219,6 +229,8 @@ static void bigben_worker(struct work_struct *work) > report_field->value[7] = 0x00; /* padding */ > hid_hw_request(bigben->hid, bigben->report, HID_REQ_SET_REPORT); > } > + > + spin_unlock_irqrestore(&bigben->lock, flags); Ouch, having hid_hw_request() called whithin a spinlock is definitely not something that should be done. However, the spinlock should be protecting 2 kinds of things: - any access to any value of struct bigben_device, but in an atomic way (i.e. copy everything you need locally in a spinlock, then release it and never read that struct again in that function). - the access to bigben->removed, which should be checked only in bigben_schedule_work() and in the .remove() function. Please note that this is what the playstation driver does: it prepares the report under the spinlock (which is really fast) before sending the report to the device which can be slow and be interrupted. With that being said, it is clear that we need 2 patches for this one: - the first one introduces the spinlock and protects the concurrent accesses to struct bigben_device (which is roughly everything below with the changes I just said) - the second one introduces bigben_schedule_work() and piggy backs on top of that new lock. Cheers, Benjamin > } > > static int hid_bigben_play_effect(struct input_dev *dev, void *data, > @@ -228,6 +240,7 @@ static int hid_bigben_play_effect(struct input_dev *dev, void *data, > struct bigben_device *bigben = hid_get_drvdata(hid); > u8 right_motor_on; > u8 left_motor_force; > + unsigned long flags; > > if (!bigben) { > hid_err(hid, "no device data\n"); > @@ -242,10 +255,13 @@ static int hid_bigben_play_effect(struct input_dev *dev, void *data, > > if (right_motor_on != bigben->right_motor_on || > left_motor_force != bigben->left_motor_force) { > + spin_lock_irqsave(&bigben->lock, flags); > bigben->right_motor_on = right_motor_on; > bigben->left_motor_force = left_motor_force; > bigben->work_ff = true; > - schedule_work(&bigben->worker); > + spin_unlock_irqrestore(&bigben->lock, flags); > + > + bigben_schedule_work(bigben); > } > > return 0; > @@ -259,6 +275,7 @@ static void bigben_set_led(struct led_classdev *led, > struct bigben_device *bigben = hid_get_drvdata(hid); > int n; > bool work; > + unsigned long flags; > > if (!bigben) { > hid_err(hid, "no device data\n"); > @@ -267,6 +284,7 @@ static void bigben_set_led(struct led_classdev *led, > > for (n = 0; n < NUM_LEDS; n++) { > if (led == bigben->leds[n]) { > + spin_lock_irqsave(&bigben->lock, flags); > if (value == LED_OFF) { > work = (bigben->led_state & BIT(n)); > bigben->led_state &= ~BIT(n); > @@ -274,10 +292,11 @@ static void bigben_set_led(struct led_classdev *led, > work = !(bigben->led_state & BIT(n)); > bigben->led_state |= BIT(n); > } > + spin_unlock_irqrestore(&bigben->lock, flags); > > if (work) { > bigben->work_led = true; > - schedule_work(&bigben->worker); > + bigben_schedule_work(bigben); > } > return; > } > @@ -307,8 +326,12 @@ static enum led_brightness bigben_get_led(struct led_classdev *led) > static void bigben_remove(struct hid_device *hid) > { > struct bigben_device *bigben = hid_get_drvdata(hid); > + unsigned long flags; > > + spin_lock_irqsave(&bigben->lock, flags); > bigben->removed = true; > + spin_unlock_irqrestore(&bigben->lock, flags); > + > cancel_work_sync(&bigben->worker); > hid_hw_stop(hid); > } > @@ -362,6 +385,7 @@ static int bigben_probe(struct hid_device *hid, > set_bit(FF_RUMBLE, hidinput->input->ffbit); > > INIT_WORK(&bigben->worker, bigben_worker); > + spin_lock_init(&bigben->lock); > > error = input_ff_create_memless(hidinput->input, NULL, > hid_bigben_play_effect); > @@ -402,7 +426,7 @@ static int bigben_probe(struct hid_device *hid, > bigben->left_motor_force = 0; > bigben->work_led = true; > bigben->work_ff = true; > - schedule_work(&bigben->worker); > + bigben_schedule_work(bigben); > > hid_info(hid, "LED and force feedback support for BigBen gamepad\n"); > > > -- > 2.25.1 >
On Fri, 10 Feb 2023 at 15:26, Benjamin Tissoires <benjamin.tissoires@redhat.com> wrote: > > [...] > > > > - if (bigben->removed || !report_field) > > You are removing an important test here: if (!report_field), please keep > it. To my understanding, that check was added in commit 918aa1ef104d ("HID: bigbenff: prevent null pointer dereference") to prevent the NULL pointer crash, only fixing the crash point. However, the true root cause was a missing check for output reports patched in commit c7bf714f8755 ("HID: check empty report_list in bigben_probe()"), where the type-confused report list_entry was overlapping with a NULL pointer, which was then causing the crash. Let me know if there is any other path that may result in having a report with no fields. In that case, it would make sense to keep the check. > > > - return; > > + spin_lock_irqsave(&bigben->lock, flags); > > > > if (bigben->work_led) { > > bigben->work_led = false; > > @@ -219,6 +229,8 @@ static void bigben_worker(struct work_struct *work) > > report_field->value[7] = 0x00; /* padding */ > > hid_hw_request(bigben->hid, bigben->report, HID_REQ_SET_REPORT); > > } > > + > > + spin_unlock_irqrestore(&bigben->lock, flags); > > Ouch, having hid_hw_request() called whithin a spinlock is definitely not > something that should be done. > > However, the spinlock should be protecting 2 kinds of things: > - any access to any value of struct bigben_device, but in an atomic way > (i.e. copy everything you need locally in a spinlock, then release it > and never read that struct again in that function). > - the access to bigben->removed, which should be checked only in > bigben_schedule_work() and in the .remove() function. > > Please note that this is what the playstation driver does: it prepares > the report under the spinlock (which is really fast) before sending the > report to the device which can be slow and be interrupted. > > With that being said, it is clear that we need 2 patches for this one: > - the first one introduces the spinlock and protects the concurrent > accesses to struct bigben_device (which is roughly everything below > with the changes I just said) > - the second one introduces bigben_schedule_work() and piggy backs on > top of that new lock. Thanks for clarifying. I will work on a v4 patch. Best regards, Pietro
On Sat, Feb 11, 2023 at 3:01 AM Hillf Danton <hdanton@sina.com> wrote: > > On Fri, 10 Feb 2023 15:11:26 +0100 Benjamin Tissoires <benjamin.tissoires@redhat.com> > > On Fri, Feb 10, 2023 at 2:24 PM Hillf Danton <hdanton@sina.com> wrote: > > > On Thu, 09 Feb 2023 23:58:55 +0000 Pietro Borrello <borrello@diag.uniroma1.it> > > > > Use spinlocks to deal with workers introducing a wrapper > > > > bigben_schedule_work(), and several spinlock checks. > > > > Otherwise, bigben_set_led() may schedule bigben->worker after the > > > > structure has been freed, causing a use-after-free. > > > > > > > > Fixes: 4eb1b01de5b9 ("HID: hid-bigbenff: fix race condition for scheduled work during removal") > > > > > > Given the flag added in 4eb1b01de5b9 and the spinlock added in this > > > patchset, devm_led_classdev_register() looks to not work for you. > > > > Actually, looking at the code now, it is clear that we need that lock. > > The current code is happily changing the struct bigben_device from > > multiple contexts, and pulls that without any barrier in the work > > struct which should produce some interesting results :) > > > > And we can probably abuse that lock to prevent scheduling a new work > > as it is done in hid-playstation.c > > > > I'll comment in the patch which parts need to be changed, because it > > is true that this patch is definitely not mergeable as such and will > > need another revision. > > > > > > > > How about replacing the advanced devm_ method with the traditional plain > > > pair of led_classdev_un/register(), with the flag mentioned cut off but > > > without bothering to add another lock? > > > > > > > As mentioned above, the lock is needed anyway, and will probably need > > to be added in a separate patch. > > Reverting to a non devm version of the led class would complexify the > > driver for the error paths, and is probably not the best move IMO. > > After this patch, > > cpu 0 cpu 2 > --- --- > bigben_remove() > spin_lock_irqsave(&bigben->lock, flags); > bigben->removed = true; > spin_unlock_irqrestore(&bigben->lock, flags); > > spin_lock_irqsave(&bigben->lock, flags); > > what makes it safe for cpu2 to acquire lock after the removed flag is true? The remove flag is just a way to prevent any other workqueue from starting. It doesn't mean that the struct bigben has been freed, so acquiring a lock at that point is fine. We then rely on 2 things: - devm_class_led to be released before struct bigben, because it was devm-allocated *after* the struct bigben was devm-allocated - we prevent any new workqueue to start and we guarantee that any running workqueue is terminated before leaving the .remove function. Given that the ledclass is gracefully shutting down all of its potential queues, we don't have any other possibility to have an unsafe call AFAIU. Cheers, Benjamin
On Mon, Feb 13, 2023 at 11:37 AM Hillf Danton <hdanton@sina.com> wrote: > > On Mon, 13 Feb 2023 09:25:37 +0100 Benjamin Tissoires <benjamin.tissoires@redhat.com> > > > > The remove flag is just a way to prevent any other workqueue from > > starting. It doesn't mean that the struct bigben has been freed, so > > acquiring a lock at that point is fine. > > We then rely on 2 things: > > - devm_class_led to be released before struct bigben, because it was > > devm-allocated *after* the struct bigben was devm-allocated > > This is the current behavior and it is intact after this patch. > > > - we prevent any new workqueue to start and we guarantee that any > > running workqueue is terminated before leaving the .remove function. > > If spinlock is added for not scheduling new workqueue then it is not > needed, because the removed flag is set before running workqueue is > terminated. Checking the flag is enough upon queuing new work. > I tend to disagree (based on Pietro's v4: - no worker is running - a led sysfs call is made - the line "if (!bigben->removed)" is true - this gets interrupted/or another CPU kicks in for the next one -> .remove gets called - bigben->removed is set to false - cancel_work_sync() called the led call continues, and schedules the work .removes terminates, and devm kicks in, killing led_class and struct bigben while the workqueue is running. So having a simple spinlocks ensures the atomicity between checking for bigben->removed and scheduling a workqueue. Cheers, Benjamin
diff --git a/drivers/hid/hid-bigbenff.c b/drivers/hid/hid-bigbenff.c index e8b16665860d..28769aa7fed6 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; + spinlock_t lock; bool removed; u8 led_state; /* LED1 = 1 .. LED4 = 8 */ u8 right_motor_on; /* right motor off/on 0/1 */ @@ -184,15 +185,24 @@ struct bigben_device { struct work_struct worker; }; +static inline void bigben_schedule_work(struct bigben_device *bigben) +{ + unsigned long flags; + + spin_lock_irqsave(&bigben->lock, flags); + if (!bigben->removed) + schedule_work(&bigben->worker); + spin_unlock_irqrestore(&bigben->lock, flags); +} static void bigben_worker(struct work_struct *work) { struct bigben_device *bigben = container_of(work, struct bigben_device, worker); struct hid_field *report_field = bigben->report->field[0]; + unsigned long flags; - if (bigben->removed || !report_field) - return; + spin_lock_irqsave(&bigben->lock, flags); if (bigben->work_led) { bigben->work_led = false; @@ -219,6 +229,8 @@ static void bigben_worker(struct work_struct *work) report_field->value[7] = 0x00; /* padding */ hid_hw_request(bigben->hid, bigben->report, HID_REQ_SET_REPORT); } + + spin_unlock_irqrestore(&bigben->lock, flags); } static int hid_bigben_play_effect(struct input_dev *dev, void *data, @@ -228,6 +240,7 @@ static int hid_bigben_play_effect(struct input_dev *dev, void *data, struct bigben_device *bigben = hid_get_drvdata(hid); u8 right_motor_on; u8 left_motor_force; + unsigned long flags; if (!bigben) { hid_err(hid, "no device data\n"); @@ -242,10 +255,13 @@ static int hid_bigben_play_effect(struct input_dev *dev, void *data, if (right_motor_on != bigben->right_motor_on || left_motor_force != bigben->left_motor_force) { + spin_lock_irqsave(&bigben->lock, flags); bigben->right_motor_on = right_motor_on; bigben->left_motor_force = left_motor_force; bigben->work_ff = true; - schedule_work(&bigben->worker); + spin_unlock_irqrestore(&bigben->lock, flags); + + bigben_schedule_work(bigben); } return 0; @@ -259,6 +275,7 @@ static void bigben_set_led(struct led_classdev *led, struct bigben_device *bigben = hid_get_drvdata(hid); int n; bool work; + unsigned long flags; if (!bigben) { hid_err(hid, "no device data\n"); @@ -267,6 +284,7 @@ static void bigben_set_led(struct led_classdev *led, for (n = 0; n < NUM_LEDS; n++) { if (led == bigben->leds[n]) { + spin_lock_irqsave(&bigben->lock, flags); if (value == LED_OFF) { work = (bigben->led_state & BIT(n)); bigben->led_state &= ~BIT(n); @@ -274,10 +292,11 @@ static void bigben_set_led(struct led_classdev *led, work = !(bigben->led_state & BIT(n)); bigben->led_state |= BIT(n); } + spin_unlock_irqrestore(&bigben->lock, flags); if (work) { bigben->work_led = true; - schedule_work(&bigben->worker); + bigben_schedule_work(bigben); } return; } @@ -307,8 +326,12 @@ static enum led_brightness bigben_get_led(struct led_classdev *led) static void bigben_remove(struct hid_device *hid) { struct bigben_device *bigben = hid_get_drvdata(hid); + unsigned long flags; + spin_lock_irqsave(&bigben->lock, flags); bigben->removed = true; + spin_unlock_irqrestore(&bigben->lock, flags); + cancel_work_sync(&bigben->worker); hid_hw_stop(hid); } @@ -362,6 +385,7 @@ static int bigben_probe(struct hid_device *hid, set_bit(FF_RUMBLE, hidinput->input->ffbit); INIT_WORK(&bigben->worker, bigben_worker); + spin_lock_init(&bigben->lock); error = input_ff_create_memless(hidinput->input, NULL, hid_bigben_play_effect); @@ -402,7 +426,7 @@ static int bigben_probe(struct hid_device *hid, bigben->left_motor_force = 0; bigben->work_led = true; bigben->work_ff = true; - schedule_work(&bigben->worker); + bigben_schedule_work(bigben); hid_info(hid, "LED and force feedback support for BigBen gamepad\n");
Use spinlocks to deal with workers introducing a wrapper bigben_schedule_work(), and several spinlock checks. Otherwise, bigben_set_led() may schedule bigben->worker after the structure has been freed, causing a use-after-free. Fixes: 4eb1b01de5b9 ("HID: hid-bigbenff: fix race condition for scheduled work during removal") Signed-off-by: Pietro Borrello <borrello@diag.uniroma1.it> --- drivers/hid/hid-bigbenff.c | 34 +++++++++++++++++++++++++++++----- 1 file changed, 29 insertions(+), 5 deletions(-)