diff mbox series

[4.19,047/125] media: davinci: vpif_capture: fix potential double free

Message ID 20200901150936.857115610@linuxfoundation.org
State Superseded
Headers show
Series None | expand

Commit Message

Greg KH Sept. 1, 2020, 3:10 p.m. UTC
From: Evgeny Novikov <novikov@ispras.ru>

[ Upstream commit 602649eadaa0c977e362e641f51ec306bc1d365d ]

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>
Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
Signed-off-by: Sasha Levin <sashal@kernel.org>
---
 drivers/media/platform/davinci/vpif_capture.c | 2 --
 1 file changed, 2 deletions(-)

Comments

Pavel Machek Sept. 1, 2020, 6:42 p.m. UTC | #1
Hi!

> [ Upstream commit 602649eadaa0c977e362e641f51ec306bc1d365d ]
> 
> 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().

> Signed-off-by: Evgeny Novikov <novikov@ispras.ru>
> Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
> Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
> Signed-off-by: Sasha Levin <sashal@kernel.org>
> ---
>  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 a96f53ce80886..cf1d11e6dd8c4 100644
> --- a/drivers/media/platform/davinci/vpif_capture.c
> +++ b/drivers/media/platform/davinci/vpif_capture.c
> @@ -1489,8 +1489,6 @@ probe_out:
>  		/* Unregister video device */
>  		video_unregister_device(&ch->video_dev);
>  	}
> -	kfree(vpif_obj.sd);
> -	v4l2_device_unregister(&vpif_obj.v4l2_dev);
>  
>  	return err;
>  }

This one is wrong. Unlike mainline, 4.19 does check return value of
vpif_probe_complete(), and thus it will lead to memory leak in 4.19.

Furthermore, I believe mainline still has a problems after this
patch. There is sync and async path where vpif_probe_complete(), and
while this fixes the sync path in mainline, I believe it will cause
memory leak on the async path.

Best regards,
									Pavel
Evgeny Novikov Sept. 2, 2020, 8:42 a.m. UTC | #2
Hi Pavel,

Maybe I miss something, but it seems that in 4.19 vpif_probe() ignores
error codes from vpif_probe_complete() and returns 0 even if it fails.
That was fixed by commit 64f883cd98c6, but it was not backported to 4.19.
In addition, this commit contains a fix of one more bug.

Regarding your second note. I investigated other drivers that use the same
mechanism and did not find out any driver that performs clean up in the
complete handler. In particular, this is the case for very similar driver
vpif_display.c. I am not an expert in this subsystem, so, I can not reason
why this is so.

-- 
Best regards,
Evgeny Novikov



01.09.2020, 21:43, "Pavel Machek" <pavel@denx.de>:
> Hi!
>
>>  [ Upstream commit 602649eadaa0c977e362e641f51ec306bc1d365d ]
>>
>>  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().
>
>>  Signed-off-by: Evgeny Novikov <novikov@ispras.ru>
>>  Signed-off-by: Hans Verkuil <hverkuil-cisco@xs4all.nl>
>>  Signed-off-by: Mauro Carvalho Chehab <mchehab+huawei@kernel.org>
>>  Signed-off-by: Sasha Levin <sashal@kernel.org>
>>  ---
>>   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 a96f53ce80886..cf1d11e6dd8c4 100644
>>  --- a/drivers/media/platform/davinci/vpif_capture.c
>>  +++ b/drivers/media/platform/davinci/vpif_capture.c
>>  @@ -1489,8 +1489,6 @@ probe_out:
>>                   /* Unregister video device */
>>                   video_unregister_device(&ch->video_dev);
>>           }
>>  - kfree(vpif_obj.sd);
>>  - v4l2_device_unregister(&vpif_obj.v4l2_dev);
>>
>>           return err;
>>   }
>
> This one is wrong. Unlike mainline, 4.19 does check return value of
> vpif_probe_complete(), and thus it will lead to memory leak in 4.19.
>
> Furthermore, I believe mainline still has a problems after this
> patch. There is sync and async path where vpif_probe_complete(), and
> while this fixes the sync path in mainline, I believe it will cause
> memory leak on the async path.
>
> Best regards,
>                                                                         Pavel
>
> --
> (english) http://www.livejournal.com/~pavelmachek
> (cesky, pictures) http://atrey.karlin.mff.cuni.cz/~pavel/picture/horses/blog.html
diff mbox series

Patch

diff --git a/drivers/media/platform/davinci/vpif_capture.c b/drivers/media/platform/davinci/vpif_capture.c
index a96f53ce80886..cf1d11e6dd8c4 100644
--- a/drivers/media/platform/davinci/vpif_capture.c
+++ b/drivers/media/platform/davinci/vpif_capture.c
@@ -1489,8 +1489,6 @@  probe_out:
 		/* Unregister video device */
 		video_unregister_device(&ch->video_dev);
 	}
-	kfree(vpif_obj.sd);
-	v4l2_device_unregister(&vpif_obj.v4l2_dev);
 
 	return err;
 }