media: hantro: fix use after free bug in hantro_release due to race condition

  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(+)
  

On 07/03/2023 16:35, 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..cf00ccaa7829 100644
> --- a/drivers/media/platform/verisilicon/hantro_drv.c
> +++ b/drivers/media/platform/verisilicon/hantro_drv.c
> @@ -601,6 +601,7 @@ static int hantro_release(struct file *filp)
>  	 * No need for extra locking because this was the last reference
>  	 * to this file.
>  	 */
> +	cancel_delayed_work(&vpu->watchdog_work);
>  	v4l2_m2m_ctx_release(ctx->fh.m2m_ctx);
>  	v4l2_fh_del(&ctx->fh);
>  	v4l2_fh_exit(&ctx->fh);

drivers/media/platform/verisilicon/hantro_drv.c: In function ‘hantro_release’:
drivers/media/platform/verisilicon/hantro_drv.c:604:30: error: ‘vpu’ undeclared (first use in this function); did you mean ‘fpu’?
  604 |         cancel_delayed_work(&vpu->watchdog_work);
      |                              ^~~
      |                              fpu
drivers/media/platform/verisilicon/hantro_drv.c:604:30: note: each undeclared identifier is reported only once for each function it appears in

You clearly didn't compile this patch!

Regards,

	Hans
  

Hans Verkuil <hverkuil@xs4all.nl> 于2023年3月13日周一 21:17写道：
>
> On 07/03/2023 16:35, 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..cf00ccaa7829 100644
> > --- a/drivers/media/platform/verisilicon/hantro_drv.c
> > +++ b/drivers/media/platform/verisilicon/hantro_drv.c
> > @@ -601,6 +601,7 @@ static int hantro_release(struct file *filp)
> >        * No need for extra locking because this was the last reference
> >        * to this file.
> >        */
> > +     cancel_delayed_work(&vpu->watchdog_work);
> >       v4l2_m2m_ctx_release(ctx->fh.m2m_ctx);
> >       v4l2_fh_del(&ctx->fh);
> >       v4l2_fh_exit(&ctx->fh);
>
> drivers/media/platform/verisilicon/hantro_drv.c: In function ‘hantro_release’:
> drivers/media/platform/verisilicon/hantro_drv.c:604:30: error: ‘vpu’ undeclared (first use in this function); did you mean ‘fpu’?
>   604 |         cancel_delayed_work(&vpu->watchdog_work);
>       |                              ^~~
>       |                              fpu
> drivers/media/platform/verisilicon/hantro_drv.c:604:30: note: each undeclared identifier is reported only once for each function it appears in
>
> You clearly didn't compile this patch!
>
Sorry for my mistake. I was hurried to report the issue and discuss
with developer to confirm it . I'll complete it and test it locally.

Best regards,
Zheng
  

On 13/03/2023 14:45, Zheng Hacker wrote:
> Hans Verkuil <hverkuil@xs4all.nl> 于2023年3月13日周一 21:17写道：
>>
>> On 07/03/2023 16:35, 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..cf00ccaa7829 100644
>>> --- a/drivers/media/platform/verisilicon/hantro_drv.c
>>> +++ b/drivers/media/platform/verisilicon/hantro_drv.c
>>> @@ -601,6 +601,7 @@ static int hantro_release(struct file *filp)
>>>        * No need for extra locking because this was the last reference
>>>        * to this file.
>>>        */
>>> +     cancel_delayed_work(&vpu->watchdog_work);
>>>       v4l2_m2m_ctx_release(ctx->fh.m2m_ctx);
>>>       v4l2_fh_del(&ctx->fh);
>>>       v4l2_fh_exit(&ctx->fh);
>>
>> drivers/media/platform/verisilicon/hantro_drv.c: In function ‘hantro_release’:
>> drivers/media/platform/verisilicon/hantro_drv.c:604:30: error: ‘vpu’ undeclared (first use in this function); did you mean ‘fpu’?
>>   604 |         cancel_delayed_work(&vpu->watchdog_work);
>>       |                              ^~~
>>       |                              fpu
>> drivers/media/platform/verisilicon/hantro_drv.c:604:30: note: each undeclared identifier is reported only once for each function it appears in
>>
>> You clearly didn't compile this patch!
>>
> Sorry for my mistake. I was hurried to report the issue and discuss
> with developer to confirm it . I'll complete it and test it locally.

Was this UAF actually observed, or is it theoretical?

Regards,

	Hans

> 
> Best regards,
> Zheng
  

Hans Verkuil <hverkuil@xs4all.nl> 于2023年3月13日周一 21:47写道：
>
> On 13/03/2023 14:45, Zheng Hacker wrote:
> > Hans Verkuil <hverkuil@xs4all.nl> 于2023年3月13日周一 21:17写道：
> >>
> >> On 07/03/2023 16:35, 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..cf00ccaa7829 100644
> >>> --- a/drivers/media/platform/verisilicon/hantro_drv.c
> >>> +++ b/drivers/media/platform/verisilicon/hantro_drv.c
> >>> @@ -601,6 +601,7 @@ static int hantro_release(struct file *filp)
> >>>        * No need for extra locking because this was the last reference
> >>>        * to this file.
> >>>        */
> >>> +     cancel_delayed_work(&vpu->watchdog_work);
> >>>       v4l2_m2m_ctx_release(ctx->fh.m2m_ctx);
> >>>       v4l2_fh_del(&ctx->fh);
> >>>       v4l2_fh_exit(&ctx->fh);
> >>
> >> drivers/media/platform/verisilicon/hantro_drv.c: In function ‘hantro_release’:
> >> drivers/media/platform/verisilicon/hantro_drv.c:604:30: error: ‘vpu’ undeclared (first use in this function); did you mean ‘fpu’?
> >>   604 |         cancel_delayed_work(&vpu->watchdog_work);
> >>       |                              ^~~
> >>       |                              fpu
> >> drivers/media/platform/verisilicon/hantro_drv.c:604:30: note: each undeclared identifier is reported only once for each function it appears in
> >>
> >> You clearly didn't compile this patch!
> >>
> > Sorry for my mistake. I was hurried to report the issue and discuss
> > with developer to confirm it . I'll complete it and test it locally.
>
> Was this UAF actually observed, or is it theoretical?
>

This is a bug identified during a static audit and referenced to other
similar issues. There's no PoC but some work has proven that the race
time window can be enlarge which make the UAF practical.

Best regards,
Zheng