| Message ID | 20230307154157.1184826-1-zyytlz.wz@163.com |
|---|---|
| State | Superseded |
| Headers | show |
| Series | media: hantro: fix use after free bug in hantro_remove due to race condition | expand |
Hi, This patch just fix the race condition in hantro_remove. All of the analysis is made by static analysis. So it may be false positives. Please feel free to let me know if there is something I've missed. Regards, Zheng Zheng Wang <zyytlz.wz@163.com> 于2023年3月7日周二 23:43写道： > > In hantro_probe, vpu->watchdog_work is bound with > hantro_watchdog. Then hantro_end_prepare_run may > be called to start the work. > > If we close the file or remove the module which will > call hantro_release and hantro_remove to make cleanup, > there may be a unfinished work. The possible sequence > is as follows, which will cause a typical UAF bug. > > The same thing will happen in hantro_release, and use > ctx after freeing it. > > Fix it by canceling the work before cleanup in hantro_release. > > CPU0 CPU1 > > |hantro_watchdog > hantro_remove | > v4l2_m2m_release | > kfree(m2m_dev); | > | > | v4l2_m2m_get_curr_priv > | m2m_dev->curr_ctx //use > > Signed-off-by: Zheng Wang <zyytlz.wz@163.com> > --- > drivers/media/platform/verisilicon/hantro_drv.c | 1 + > 1 file changed, 1 insertion(+) > > diff --git a/drivers/media/platform/verisilicon/hantro_drv.c b/drivers/media/platform/verisilicon/hantro_drv.c > index b0aeedae7b65..80bd856a4da9 100644 > --- a/drivers/media/platform/verisilicon/hantro_drv.c > +++ b/drivers/media/platform/verisilicon/hantro_drv.c > @@ -1099,6 +1099,7 @@ static int hantro_remove(struct platform_device *pdev) > > v4l2_info(&vpu->v4l2_dev, "Removing %s\n", pdev->name); > > + cancel_delayed_work(&vpu->watchdog_work); > media_device_unregister(&vpu->mdev); > hantro_remove_dec_func(vpu); > hantro_remove_enc_func(vpu); > -- > 2.25.1 >
On 13/03/2023 16:08, Hans Verkuil wrote: > On 07/03/2023 16:41, Zheng Wang wrote: >> In hantro_probe, vpu->watchdog_work is bound with >> hantro_watchdog. Then hantro_end_prepare_run may >> be called to start the work. >> >> If we close the file or remove the module which will >> call hantro_release and hantro_remove to make cleanup, >> there may be a unfinished work. The possible sequence >> is as follows, which will cause a typical UAF bug. >> >> The same thing will happen in hantro_release, and use >> ctx after freeing it. >> >> Fix it by canceling the work before cleanup in hantro_release. >> >> CPU0 CPU1 >> >> |hantro_watchdog >> hantro_remove | >> v4l2_m2m_release | >> kfree(m2m_dev); | >> | >> | v4l2_m2m_get_curr_priv >> | m2m_dev->curr_ctx //use >> >> Signed-off-by: Zheng Wang <zyytlz.wz@163.com> >> --- >> drivers/media/platform/verisilicon/hantro_drv.c | 1 + >> 1 file changed, 1 insertion(+) >> >> diff --git a/drivers/media/platform/verisilicon/hantro_drv.c b/drivers/media/platform/verisilicon/hantro_drv.c >> index b0aeedae7b65..80bd856a4da9 100644 >> --- a/drivers/media/platform/verisilicon/hantro_drv.c >> +++ b/drivers/media/platform/verisilicon/hantro_drv.c >> @@ -1099,6 +1099,7 @@ static int hantro_remove(struct platform_device *pdev) >> >> v4l2_info(&vpu->v4l2_dev, "Removing %s\n", pdev->name); >> >> + cancel_delayed_work(&vpu->watchdog_work); > > Use cancel_delayed_work_sync(): that ensures the code waits for a running > watchdog function to finish. > > Ditto for the other patch. And also the cedrus and rkvdec patches. Regards, Hans > > Regards, > > Hans > >> media_device_unregister(&vpu->mdev); >> hantro_remove_dec_func(vpu); >> hantro_remove_enc_func(vpu); >
Hans Verkuil <hverkuil@xs4all.nl> 于2023年3月13日周一 23:09写道： > > On 13/03/2023 16:08, Hans Verkuil wrote: > > On 07/03/2023 16:41, Zheng Wang wrote: > >> In hantro_probe, vpu->watchdog_work is bound with > >> hantro_watchdog. Then hantro_end_prepare_run may > >> be called to start the work. > >> > >> If we close the file or remove the module which will > >> call hantro_release and hantro_remove to make cleanup, > >> there may be a unfinished work. The possible sequence > >> is as follows, which will cause a typical UAF bug. > >> > >> The same thing will happen in hantro_release, and use > >> ctx after freeing it. > >> > >> Fix it by canceling the work before cleanup in hantro_release. > >> > >> CPU0 CPU1 > >> > >> |hantro_watchdog > >> hantro_remove | > >> v4l2_m2m_release | > >> kfree(m2m_dev); | > >> | > >> | v4l2_m2m_get_curr_priv > >> | m2m_dev->curr_ctx //use > >> > >> Signed-off-by: Zheng Wang <zyytlz.wz@163.com> > >> --- > >> drivers/media/platform/verisilicon/hantro_drv.c | 1 + > >> 1 file changed, 1 insertion(+) > >> > >> diff --git a/drivers/media/platform/verisilicon/hantro_drv.c b/drivers/media/platform/verisilicon/hantro_drv.c > >> index b0aeedae7b65..80bd856a4da9 100644 > >> --- a/drivers/media/platform/verisilicon/hantro_drv.c > >> +++ b/drivers/media/platform/verisilicon/hantro_drv.c > >> @@ -1099,6 +1099,7 @@ static int hantro_remove(struct platform_device *pdev) > >> > >> v4l2_info(&vpu->v4l2_dev, "Removing %s\n", pdev->name); > >> > >> + cancel_delayed_work(&vpu->watchdog_work); > > > > Use cancel_delayed_work_sync(): that ensures the code waits for a running > > watchdog function to finish. > > > > Ditto for the other patch. > > And also the cedrus and rkvdec patches. > > Regards, > Dear Hans, Thanks for your review. I'll use cancel_delayed_work_sync in the next version. Best regards, Zheng > > > >> media_device_unregister(&vpu->mdev); > >> hantro_remove_dec_func(vpu); > >> hantro_remove_enc_func(vpu); > > >
diff --git a/drivers/media/platform/verisilicon/hantro_drv.c b/drivers/media/platform/verisilicon/hantro_drv.c index b0aeedae7b65..80bd856a4da9 100644 --- a/drivers/media/platform/verisilicon/hantro_drv.c +++ b/drivers/media/platform/verisilicon/hantro_drv.c @@ -1099,6 +1099,7 @@ static int hantro_remove(struct platform_device *pdev) v4l2_info(&vpu->v4l2_dev, "Removing %s\n", pdev->name); + cancel_delayed_work(&vpu->watchdog_work); media_device_unregister(&vpu->mdev); hantro_remove_dec_func(vpu); hantro_remove_enc_func(vpu);
In hantro_probe, vpu->watchdog_work is bound with hantro_watchdog. Then hantro_end_prepare_run may be called to start the work. If we close the file or remove the module which will call hantro_release and hantro_remove to make cleanup, there may be a unfinished work. The possible sequence is as follows, which will cause a typical UAF bug. The same thing will happen in hantro_release, and use ctx after freeing it. Fix it by canceling the work before cleanup in hantro_release. CPU0 CPU1 |hantro_watchdog hantro_remove | v4l2_m2m_release | kfree(m2m_dev); | | | v4l2_m2m_get_curr_priv | m2m_dev->curr_ctx //use Signed-off-by: Zheng Wang <zyytlz.wz@163.com> --- drivers/media/platform/verisilicon/hantro_drv.c | 1 + 1 file changed, 1 insertion(+)