| Message ID | 20190207224650.GA49861@dtor-ws (mailing list archive) |
|---|---|
| State | Mainlined |
| Commit | a342083abe576db43594a32d458a61fa81f7cb32 |
| Headers | show |
| Series | Input: matrix_keypad - use flush_delayed_work() | expand |
Hi Dmitry, On Thu, Feb 7, 2019 at 5:46 PM Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > > We should be using flush_delayed_work() instead of flush_work() in > matrix_keypad_stop() to ensure that we are not missing work that is > scheduled but not yet put in the workqueue (i.e. its delay timer has not > expired yet). > Could the following scenario cause a use-after-free? (I am adding comments on lines starting with -->) a) user closes the device handle: static void matrix_keypad_stop(struct input_dev *dev) { struct matrix_keypad *keypad = input_get_drvdata(dev); spin_lock_irq(&keypad->lock); keypad->stopped = true; spin_unlock_irq(&keypad->lock); flush_work(&keypad->work.work); --> --> new interrupt comes in, and schedules new delayed keypad->work (1) --> /* * matrix_keypad_scan() will leave IRQs enabled; * we should disable them now. */ disable_row_irqs(keypad); } b) user removes the keypad, or unloads the module: static int matrix_keypad_remove(struct platform_device *pdev) { struct matrix_keypad *keypad = platform_get_drvdata(pdev); matrix_keypad_free_gpio(keypad); input_unregister_device(keypad->input_dev); kfree(keypad); --> --> delayed keypad->work scheduled at (1) above executes anywhere past here, --> and causes a user-after-free. --> return 0; }
Hi Sven, On Sun, Feb 10, 2019 at 12:43:21PM -0500, Sven Van Asbroeck wrote: > Hi Dmitry, > > On Thu, Feb 7, 2019 at 5:46 PM Dmitry Torokhov > <dmitry.torokhov@gmail.com> wrote: > > > > We should be using flush_delayed_work() instead of flush_work() in > > matrix_keypad_stop() to ensure that we are not missing work that is > > scheduled but not yet put in the workqueue (i.e. its delay timer has not > > expired yet). > > > > Could the following scenario cause a use-after-free? > (I am adding comments on lines starting with -->) > > a) user closes the device handle: > > static void matrix_keypad_stop(struct input_dev *dev) > { > struct matrix_keypad *keypad = input_get_drvdata(dev); > > spin_lock_irq(&keypad->lock); > keypad->stopped = true; > spin_unlock_irq(&keypad->lock); > > flush_work(&keypad->work.work); > --> > --> new interrupt comes in, and schedules new delayed keypad->work (1) It will not schedule new work because we check keypad->stopped flag in ISR. Thanks.
On Mon, Feb 11, 2019 at 3:29 AM Dmitry Torokhov <dmitry.torokhov@gmail.com> wrote: > > It will not schedule new work because we check keypad->stopped flag > in ISR. > Yes, you are correct. Sorry for the confusion. Reviewed-by: Sven Van Asbroeck <TheSven73@gmail.com>
diff --git a/drivers/input/keyboard/matrix_keypad.c b/drivers/input/keyboard/matrix_keypad.c index 403452ef00e6..3d1cb7bf5e35 100644 --- a/drivers/input/keyboard/matrix_keypad.c +++ b/drivers/input/keyboard/matrix_keypad.c @@ -222,7 +222,7 @@ static void matrix_keypad_stop(struct input_dev *dev) keypad->stopped = true; spin_unlock_irq(&keypad->lock); - flush_work(&keypad->work.work); + flush_delayed_work(&keypad->work); /* * matrix_keypad_scan() will leave IRQs enabled; * we should disable them now.
We should be using flush_delayed_work() instead of flush_work() in matrix_keypad_stop() to ensure that we are not missing work that is scheduled but not yet put in the workqueue (i.e. its delay timer has not expired yet). Signed-off-by: Dmitry Torokhov <dmitry.torokhov@gmail.com> --- drivers/input/keyboard/matrix_keypad.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)