| Message ID | 20230313154856.3691660-1-zyytlz.wz@163.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v4] media: hantro: fix use after free bug in hantro_remove due to race condition | expand |
Hi Zheng, Hans, On Mon, Mar 13, 2023 at 12:49 PM Zheng Wang <zyytlz.wz@163.com> 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 an 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> > Fixes: 932a9317ac49 ("media: hantro: Add helpers to prepare/finish a run") Thanks for the patch. Give me a few days to take a closer look. Thanks, Ezequiel > --- > v4: > - add Fixes label to help with the fix > > v3: > - use cancel_delayed_work_sync instead of cancel_delayed_work and add it to > hantro_release suggested by Hans Verkuil > > v2: > - move the cancel-work-related code to hantro_remove suggested by Hans Verkuil > --- > drivers/media/platform/verisilicon/hantro_drv.c | 2 ++ > 1 file changed, 2 insertions(+) > > diff --git a/drivers/media/platform/verisilicon/hantro_drv.c b/drivers/media/platform/verisilicon/hantro_drv.c > index b0aeedae7b65..86a4c0fa8c7d 100644 > --- a/drivers/media/platform/verisilicon/hantro_drv.c > +++ b/drivers/media/platform/verisilicon/hantro_drv.c > @@ -597,6 +597,7 @@ static int hantro_release(struct file *filp) > struct hantro_ctx *ctx = > container_of(filp->private_data, struct hantro_ctx, fh); > > + cancel_delayed_work_sync(&ctx->dev->watchdog_work); > /* > * No need for extra locking because this was the last reference > * to this file. > @@ -1099,6 +1100,7 @@ static int hantro_remove(struct platform_device *pdev) > > v4l2_info(&vpu->v4l2_dev, "Removing %s\n", pdev->name); > > + cancel_delayed_work_sync(&vpu->watchdog_work); > media_device_unregister(&vpu->mdev); > hantro_remove_dec_func(vpu); > hantro_remove_enc_func(vpu); > -- > 2.25.1 >
Ezequiel Garcia <ezequiel@vanguardiasur.com.ar> 于2023年3月14日周二 20:05写道： > > Hi Zheng, Hans, > > On Mon, Mar 13, 2023 at 12:49 PM Zheng Wang <zyytlz.wz@163.com> 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 an 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> > > Fixes: 932a9317ac49 ("media: hantro: Add helpers to prepare/finish a run") > > Thanks for the patch. > > Give me a few days to take a closer look. > Thanks for your reply. Hope you have a nice day :) Best regards, Zheng > Thanks, > Ezequiel > > > --- > > v4: > > - add Fixes label to help with the fix > > > > v3: > > - use cancel_delayed_work_sync instead of cancel_delayed_work and add it to > > hantro_release suggested by Hans Verkuil > > > > v2: > > - move the cancel-work-related code to hantro_remove suggested by Hans Verkuil > > --- > > drivers/media/platform/verisilicon/hantro_drv.c | 2 ++ > > 1 file changed, 2 insertions(+) > > > > diff --git a/drivers/media/platform/verisilicon/hantro_drv.c b/drivers/media/platform/verisilicon/hantro_drv.c > > index b0aeedae7b65..86a4c0fa8c7d 100644 > > --- a/drivers/media/platform/verisilicon/hantro_drv.c > > +++ b/drivers/media/platform/verisilicon/hantro_drv.c > > @@ -597,6 +597,7 @@ static int hantro_release(struct file *filp) > > struct hantro_ctx *ctx = > > container_of(filp->private_data, struct hantro_ctx, fh); > > > > + cancel_delayed_work_sync(&ctx->dev->watchdog_work); > > /* > > * No need for extra locking because this was the last reference > > * to this file. > > @@ -1099,6 +1100,7 @@ static int hantro_remove(struct platform_device *pdev) > > > > v4l2_info(&vpu->v4l2_dev, "Removing %s\n", pdev->name); > > > > + cancel_delayed_work_sync(&vpu->watchdog_work); > > media_device_unregister(&vpu->mdev); > > hantro_remove_dec_func(vpu); > > hantro_remove_enc_func(vpu); > > -- > > 2.25.1 > >
diff --git a/drivers/media/platform/verisilicon/hantro_drv.c b/drivers/media/platform/verisilicon/hantro_drv.c index b0aeedae7b65..86a4c0fa8c7d 100644 --- a/drivers/media/platform/verisilicon/hantro_drv.c +++ b/drivers/media/platform/verisilicon/hantro_drv.c @@ -597,6 +597,7 @@ static int hantro_release(struct file *filp) struct hantro_ctx *ctx = container_of(filp->private_data, struct hantro_ctx, fh); + cancel_delayed_work_sync(&ctx->dev->watchdog_work); /* * No need for extra locking because this was the last reference * to this file. @@ -1099,6 +1100,7 @@ static int hantro_remove(struct platform_device *pdev) v4l2_info(&vpu->v4l2_dev, "Removing %s\n", pdev->name); + cancel_delayed_work_sync(&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 an 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> Fixes: 932a9317ac49 ("media: hantro: Add helpers to prepare/finish a run") --- v4: - add Fixes label to help with the fix v3: - use cancel_delayed_work_sync instead of cancel_delayed_work and add it to hantro_release suggested by Hans Verkuil v2: - move the cancel-work-related code to hantro_remove suggested by Hans Verkuil --- drivers/media/platform/verisilicon/hantro_drv.c | 2 ++ 1 file changed, 2 insertions(+)