| Message ID | 20230802033737.9738-1-duoming@zju.edu.cn (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | sh: push-switch: reorder cleanup operations to avoid UAF bug | expand |
On Wed, Aug 2, 2023 at 5:46 AM Duoming Zhou <duoming@zju.edu.cn> wrote: > The original code puts flush_work() before timer_shutdown_sync() > in switch_drv_remove(). Although we use flush_work() to stop > the worker, it could be re-scheduled in switch_timer. As a result, > the UAF bug will happen. The detail is shown below: > > (cpu 0) | (cpu 1) > switch_drv_remove() | > flush_work() | > ... | switch_timer //timer > | schedule_work(&psw->work) > timer_shutdown_sync() | > ... | switch_work_handler //worker > kfree(psw) //free | > | psw->state = 0 //use > > This patch puts timer_shutdown_sync() before flush_work() to > mitigate the bugs. As a result, the worker and timer could > be stopped safely before the deallocate operations. > > Fixes: 9f5e8eee5cfe ("sh: generic push-switch framework.") > Signed-off-by: Duoming Zhou <duoming@zju.edu.cn> Reviewed-by: Geert Uytterhoeven <geert+renesas@glider.be> Gr{oetje,eeting}s, Geert
On Wed, 2023-08-02 at 11:37 +0800, Duoming Zhou wrote: > The original code puts flush_work() before timer_shutdown_sync() > in switch_drv_remove(). Although we use flush_work() to stop > the worker, it could be re-scheduled in switch_timer. As a result, > the UAF bug will happen. The detail is shown below: > > (cpu 0) | (cpu 1) > switch_drv_remove() | > flush_work() | > ... | switch_timer //timer > | schedule_work(&psw->work) > timer_shutdown_sync() | > ... | switch_work_handler //worker > kfree(psw) //free | > | psw->state = 0 //use > > This patch puts timer_shutdown_sync() before flush_work() to > mitigate the bugs. As a result, the worker and timer could > be stopped safely before the deallocate operations. > > Fixes: 9f5e8eee5cfe ("sh: generic push-switch framework.") > Signed-off-by: Duoming Zhou <duoming@zju.edu.cn> > --- > arch/sh/drivers/push-switch.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/arch/sh/drivers/push-switch.c b/arch/sh/drivers/push-switch.c > index c95f48ff3f6..6ecba5f521e 100644 > --- a/arch/sh/drivers/push-switch.c > +++ b/arch/sh/drivers/push-switch.c > @@ -101,8 +101,8 @@ static int switch_drv_remove(struct platform_device *pdev) > device_remove_file(&pdev->dev, &dev_attr_switch); > > platform_set_drvdata(pdev, NULL); > - flush_work(&psw->work); > timer_shutdown_sync(&psw->debounce); > + flush_work(&psw->work); > free_irq(irq, pdev); > > kfree(psw); Reviewed-by: John Paul Adrian Glaubitz <glaubitz@physik.fu-berlin.de>
diff --git a/arch/sh/drivers/push-switch.c b/arch/sh/drivers/push-switch.c index c95f48ff3f6..6ecba5f521e 100644 --- a/arch/sh/drivers/push-switch.c +++ b/arch/sh/drivers/push-switch.c @@ -101,8 +101,8 @@ static int switch_drv_remove(struct platform_device *pdev) device_remove_file(&pdev->dev, &dev_attr_switch); platform_set_drvdata(pdev, NULL); - flush_work(&psw->work); timer_shutdown_sync(&psw->debounce); + flush_work(&psw->work); free_irq(irq, pdev); kfree(psw);
The original code puts flush_work() before timer_shutdown_sync() in switch_drv_remove(). Although we use flush_work() to stop the worker, it could be re-scheduled in switch_timer. As a result, the UAF bug will happen. The detail is shown below: (cpu 0) | (cpu 1) switch_drv_remove() | flush_work() | ... | switch_timer //timer | schedule_work(&psw->work) timer_shutdown_sync() | ... | switch_work_handler //worker kfree(psw) //free | | psw->state = 0 //use This patch puts timer_shutdown_sync() before flush_work() to mitigate the bugs. As a result, the worker and timer could be stopped safely before the deallocate operations. Fixes: 9f5e8eee5cfe ("sh: generic push-switch framework.") Signed-off-by: Duoming Zhou <duoming@zju.edu.cn> --- arch/sh/drivers/push-switch.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)