[PATCH] media: davinci: vpif_capture: fix potential double free

[PATCH] media: davinci: vpif_capture: fix potential double free

[Evgeny Novikov]

In case of errors vpif_probe_complete() releases memory for vpif_obj.sd
and unregisters the V4L2 device. But then this is done again by
vpif_probe() itself. The patch removes the cleaning from
vpif_probe_complete().

Found by Linux Driver Verification project (linuxtesting.org).

Signed-off-by: Evgeny Novikov <novikov@ispras.ru>
---
 drivers/media/platform/davinci/vpif_capture.c | 2 --
 1 file changed, 2 deletions(-)

diff --git a/drivers/media/platform/davinci/vpif_capture.c b/drivers/media/platform/davinci/vpif_capture.c
index d9ec439faefa..72a0e94e2e21 100644
--- a/drivers/media/platform/davinci/vpif_capture.c
+++ b/drivers/media/platform/davinci/vpif_capture.c
@@ -1482,8 +1482,6 @@ static int vpif_probe_complete(void)
 		/* Unregister video device */
 		video_unregister_device(&ch->video_dev);
 	}
-	kfree(vpif_obj.sd);
-	v4l2_device_unregister(&vpif_obj.v4l2_dev);
 
 	return err;
 }
-- 
2.16.4

Re: [PATCH] media: davinci: vpif_capture: fix potential double free

[Lad, Prabhakar]

Hi Evgeny,

Thank you for the patch.

On Thu, Jul 23, 2020 at 6:04 PM Evgeny Novikov <novikov@ispras.ru> wrote:
>
> In case of errors vpif_probe_complete() releases memory for vpif_obj.sd
> and unregisters the V4L2 device. But then this is done again by
> vpif_probe() itself. The patch removes the cleaning from
> vpif_probe_complete().
>
> Found by Linux Driver Verification project (linuxtesting.org).
>
> Signed-off-by: Evgeny Novikov <novikov@ispras.ru>
> ---
>  drivers/media/platform/davinci/vpif_capture.c | 2 --
>  1 file changed, 2 deletions(-)
>
> diff --git a/drivers/media/platform/davinci/vpif_capture.c b/drivers/media/platform/davinci/vpif_capture.c
> index d9ec439faefa..72a0e94e2e21 100644
> --- a/drivers/media/platform/davinci/vpif_capture.c
> +++ b/drivers/media/platform/davinci/vpif_capture.c
> @@ -1482,8 +1482,6 @@ static int vpif_probe_complete(void)
>                 /* Unregister video device */
>                 video_unregister_device(&ch->video_dev);
>         }
> -       kfree(vpif_obj.sd);
> -       v4l2_device_unregister(&vpif_obj.v4l2_dev);
>
vpif_probe_complete() is a async callback and probe() should have
already completed by then.

Cheers,
--Prabhakar

>         return err;
>  }
> --
> 2.16.4
>

Re: [PATCH] media: davinci: vpif_capture: fix potential double free

[Evgeny Novikov]

Hi Lad,

Yet again I can not demonstrate you a nice error trace corresponding to the bug fixed by the patch. Indeed, there is a branch in vpif_probe() that explicitly invokes vpif_probe_complete() and the patch targets the possible issue that can happen during this. 

When I developed the patch I saw on vpif_display.ko. It looks very similar regarding things touched by the patch. In particular, it does not free vpif_obj.sd in its vpif_probe_complete(). But now I see that it does this in vpif_remove()!

Do you think that vpif_capture.ko should do the same? If so, I guess that I should fix the patch appropriately since likely it just replaces one (very rare) bug with another one (on a typical execution path).

-- 
Evgeny Novikov
Linux Verification Center, ISP RAS
http://linuxtesting.org



24.07.2020, 17:17, "Lad, Prabhakar" <prabhakar.csengg@gmail.com>:
> Hi Evgeny,
>
> Thank you for the patch.
>
> On Thu, Jul 23, 2020 at 6:04 PM Evgeny Novikov <novikov@ispras.ru> wrote:
>>  In case of errors vpif_probe_complete() releases memory for vpif_obj.sd
>>  and unregisters the V4L2 device. But then this is done again by
>>  vpif_probe() itself. The patch removes the cleaning from
>>  vpif_probe_complete().
>>
>>  Found by Linux Driver Verification project (linuxtesting.org).
>>
>>  Signed-off-by: Evgeny Novikov <novikov@ispras.ru>
>>  ---
>>   drivers/media/platform/davinci/vpif_capture.c | 2 --
>>   1 file changed, 2 deletions(-)
>>
>>  diff --git a/drivers/media/platform/davinci/vpif_capture.c b/drivers/media/platform/davinci/vpif_capture.c
>>  index d9ec439faefa..72a0e94e2e21 100644
>>  --- a/drivers/media/platform/davinci/vpif_capture.c
>>  +++ b/drivers/media/platform/davinci/vpif_capture.c
>>  @@ -1482,8 +1482,6 @@ static int vpif_probe_complete(void)
>>                  /* Unregister video device */
>>                  video_unregister_device(&ch->video_dev);
>>          }
>>  - kfree(vpif_obj.sd);
>>  - v4l2_device_unregister(&vpif_obj.v4l2_dev);
>
> vpif_probe_complete() is a async callback and probe() should have
> already completed by then.
>
> Cheers,
> --Prabhakar
>
>>          return err;
>>   }
>>  --
>>  2.16.4

Re: [PATCH] media: davinci: vpif_capture: fix potential double free

[Lad, Prabhakar]

Hi Evgeny,

On Fri, Jul 24, 2020 at 9:00 PM Evgeny Novikov <novikov@ispras.ru> wrote:
>
> Hi Lad,
>
> Yet again I can not demonstrate you a nice error trace corresponding to the bug fixed by the patch. Indeed, there is a branch in vpif_probe() that explicitly invokes vpif_probe_complete() and the patch targets the possible issue that can happen during this.
>
> When I developed the patch I saw on vpif_display.ko. It looks very similar regarding things touched by the patch. In particular, it does not free vpif_obj.sd in its vpif_probe_complete(). But now I see that it does this in vpif_remove()!
>
Makes sense.

> Do you think that vpif_capture.ko should do the same? If so, I guess that I should fix the patch appropriately since likely it just replaces one (very rare) bug with another one (on a typical execution path).
>
Yes it should.

Cheers,
--Prabhakar

> --
> Evgeny Novikov
> Linux Verification Center, ISP RAS
> http://linuxtesting.org
>
>
>
> 24.07.2020, 17:17, "Lad, Prabhakar" <prabhakar.csengg@gmail.com>:
> > Hi Evgeny,
> >
> > Thank you for the patch.
> >
> > On Thu, Jul 23, 2020 at 6:04 PM Evgeny Novikov <novikov@ispras.ru> wrote:
> >>  In case of errors vpif_probe_complete() releases memory for vpif_obj.sd
> >>  and unregisters the V4L2 device. But then this is done again by
> >>  vpif_probe() itself. The patch removes the cleaning from
> >>  vpif_probe_complete().
> >>
> >>  Found by Linux Driver Verification project (linuxtesting.org).
> >>
> >>  Signed-off-by: Evgeny Novikov <novikov@ispras.ru>
> >>  ---
> >>   drivers/media/platform/davinci/vpif_capture.c | 2 --
> >>   1 file changed, 2 deletions(-)
> >>
> >>  diff --git a/drivers/media/platform/davinci/vpif_capture.c b/drivers/media/platform/davinci/vpif_capture.c
> >>  index d9ec439faefa..72a0e94e2e21 100644
> >>  --- a/drivers/media/platform/davinci/vpif_capture.c
> >>  +++ b/drivers/media/platform/davinci/vpif_capture.c
> >>  @@ -1482,8 +1482,6 @@ static int vpif_probe_complete(void)
> >>                  /* Unregister video device */
> >>                  video_unregister_device(&ch->video_dev);
> >>          }
> >>  - kfree(vpif_obj.sd);
> >>  - v4l2_device_unregister(&vpif_obj.v4l2_dev);
> >
> > vpif_probe_complete() is a async callback and probe() should have
> > already completed by then.
> >
> > Cheers,
> > --Prabhakar
> >
> >>          return err;
> >>   }
> >>  --
> >>  2.16.4

Re: [PATCH] media: davinci: vpif_capture: fix potential double free

[Evgeny Novikov]

Hi Lad,

25.07.2020, 01:06, "Lad, Prabhakar" <prabhakar.csengg@gmail.com>:
> Hi Evgeny,
>
> On Fri, Jul 24, 2020 at 9:00 PM Evgeny Novikov <novikov@ispras.ru> wrote:
>>  Hi Lad,
>>
>>  Yet again I can not demonstrate you a nice error trace corresponding to the bug fixed by the patch. Indeed, there is a branch in vpif_probe() that explicitly invokes vpif_probe_complete() and the patch targets the possible issue that can happen during this.
>>
>>  When I developed the patch I saw on vpif_display.ko. It looks very similar regarding things touched by the patch. In particular, it does not free vpif_obj.sd in its vpif_probe_complete(). But now I see that it does this in vpif_remove()!
>
> Makes sense.
>
>>  Do you think that vpif_capture.ko should do the same? If so, I guess that I should fix the patch appropriately since likely it just replaces one (very rare) bug with another one (on a typical execution path).
>
> Yes it should.

vpif_remove() from vpif_capture.ko already both frees vpif_obj.sd and unregisters the V4L2 device. So, there is no need to fix the patch.

BTW, other drivers, e.g. drivers/media/platform/renesas-ceu.c, do not clean up memory allocated by probe within v4l2_async_notifier_operations.complete as well. 

--
Best regards,
Evgeny Novikov

> Cheers,
> --Prabhakar
>
>>  --
>>  Evgeny Novikov
>>  Linux Verification Center, ISP RAS
>>  http://linuxtesting.org
>>
>>  24.07.2020, 17:17, "Lad, Prabhakar" <prabhakar.csengg@gmail.com>:
>>  > Hi Evgeny,
>>  >
>>  > Thank you for the patch.
>>  >
>>  > On Thu, Jul 23, 2020 at 6:04 PM Evgeny Novikov <novikov@ispras.ru> wrote:
>>  >> In case of errors vpif_probe_complete() releases memory for vpif_obj.sd
>>  >> and unregisters the V4L2 device. But then this is done again by
>>  >> vpif_probe() itself. The patch removes the cleaning from
>>  >> vpif_probe_complete().
>>  >>
>>  >> Found by Linux Driver Verification project (linuxtesting.org).
>>  >>
>>  >> Signed-off-by: Evgeny Novikov <novikov@ispras.ru>
>>  >> ---
>>  >> drivers/media/platform/davinci/vpif_capture.c | 2 --
>>  >> 1 file changed, 2 deletions(-)
>>  >>
>>  >> diff --git a/drivers/media/platform/davinci/vpif_capture.c b/drivers/media/platform/davinci/vpif_capture.c
>>  >> index d9ec439faefa..72a0e94e2e21 100644
>>  >> --- a/drivers/media/platform/davinci/vpif_capture.c
>>  >> +++ b/drivers/media/platform/davinci/vpif_capture.c
>>  >> @@ -1482,8 +1482,6 @@ static int vpif_probe_complete(void)
>>  >> /* Unregister video device */
>>  >> video_unregister_device(&ch->video_dev);
>>  >> }
>>  >> - kfree(vpif_obj.sd);
>>  >> - v4l2_device_unregister(&vpif_obj.v4l2_dev);
>>  >
>>  > vpif_probe_complete() is a async callback and probe() should have
>>  > already completed by then.
>>  >
>>  > Cheers,
>>  > --Prabhakar
>>  >
>>  >> return err;
>>  >> }
>>  >> --
>>  >> 2.16.4

Re: [PATCH] media: davinci: vpif_capture: fix potential double free

[Markus Elfring]

> In case of errors vpif_probe_complete() releases memory for vpif_obj.sd
> and unregisters the V4L2 device. But then this is done again by
> vpif_probe() itself. The patch removes the cleaning from
> vpif_probe_complete().

* An imperative wording can be preferred for the change description,
  can't it?

* Would you like to add the tag “Fixes” to the commit message?

Regards,
Markus

Re: [PATCH] media: davinci: vpif_capture: fix potential double free

[Markus Elfring]

> In case of errors vpif_probe_complete() releases memory for vpif_obj.sd
> and unregisters the V4L2 device. But then this is done again by
> vpif_probe() itself. The patch removes the cleaning from
> vpif_probe_complete().

* An imperative wording can be preferred for the change description,
  can't it?

* Would you like to add the tag “Fixes” to the commit message?

Regards,
Markus

Re: [PATCH] media: davinci: vpif_capture: fix potential double free

[Evgeny Novikov]

Hi Markus,

It seems that Lad Prabhakar detected a critical issue introduced by the patch instead of a minor bug fixed by it. So, we should take care of this first, but I will address your notes as well. 

-- 
Evgeny Novikov
Linux Verification Center, ISP RAS
http://linuxtesting.org

23.07.2020, 23:15, "Markus Elfring" <markus.elfring@web.de>:
>>  In case of errors vpif_probe_complete() releases memory for vpif_obj.sd
>>  and unregisters the V4L2 device. But then this is done again by
>>  vpif_probe() itself. The patch removes the cleaning from
>>  vpif_probe_complete().
>
> * An imperative wording can be preferred for the change description,
>   can't it?
>
> * Would you like to add the tag “Fixes” to the commit message?
>
> Regards,
> Markus

Re: [PATCH] media: davinci: vpif_capture: fix potential double free

[Evgeny Novikov]

Hi Markus,

It seems that Lad Prabhakar detected a critical issue introduced by the patch instead of a minor bug fixed by it. So, we should take care of this first, but I will address your notes as well. 

-- 
Evgeny Novikov
Linux Verification Center, ISP RAS
http://linuxtesting.org

23.07.2020, 23:15, "Markus Elfring" <markus.elfring@web.de>:
>>  In case of errors vpif_probe_complete() releases memory for vpif_obj.sd
>>  and unregisters the V4L2 device. But then this is done again by
>>  vpif_probe() itself. The patch removes the cleaning from
>>  vpif_probe_complete().
>
> * An imperative wording can be preferred for the change description,
>   can't it?
>
> * Would you like to add the tag “Fixes” to the commit message?
>
> Regards,
> Markus