[PATCH] media: zoran: fix video_device double free

[PATCH] media: zoran: fix video_device double free

[Guangshuo Li]

zoran allocates zr->video_dev with video_device_alloc() and installs
zoran_vdev_release() as the video_device release callback. That callback
frees the video_device.

After video_register_device() succeeds, the video_device lifetime is
managed by the V4L2 core. video_unregister_device() eventually invokes
the registered release callback, so the explicit kfree() after
video_unregister_device() frees the same object again.

There is also a registration failure path with the same risk. If
device_register() fails inside video_register_device(), the V4L2 core
calls put_device(&vdev->dev), which may run v4l2_device_release() and
invoke zoran_vdev_release(). Control then returns to
zoran_init_video_devices(), which frees zr->video_dev again.

Fix the successful unregister path by dropping the extra kfree(). For
registration failures, temporarily use video_device_release_empty while
calling video_register_device(), so that any release callback run from
the V4L2 core's registration failure path does not free the zoran-owned
video_device. zoran_init_video_devices() then remains responsible for
freeing the allocation exactly once.

This issue was found by a static analysis tool I am developing.

Fixes: 82e3a496eb56 ("media: staging: media: zoran: move videodev alloc")
Signed-off-by: Guangshuo Li <lgs201920130244@gmail.com>
---
 drivers/media/pci/zoran/zoran_card.c | 29 +++++++++++++++++++++++++---
 1 file changed, 26 insertions(+), 3 deletions(-)

diff --git a/drivers/media/pci/zoran/zoran_card.c b/drivers/media/pci/zoran/zoran_card.c
index d81facf735d9..e7268718f137 100644
--- a/drivers/media/pci/zoran/zoran_card.c
+++ b/drivers/media/pci/zoran/zoran_card.c
@@ -863,10 +863,12 @@ int zoran_check_jpg_settings(struct zoran *zr,
 
 static int zoran_init_video_device(struct zoran *zr, struct video_device *video_dev, int dir)
 {
+	void (*release)(struct video_device *vdev);
 	int err;
 
 	/* Now add the template and register the device unit. */
 	*video_dev = zoran_template;
+	release = video_dev->release;
 	video_dev->v4l2_dev = &zr->v4l2_dev;
 	video_dev->lock = &zr->lock;
 	video_dev->device_caps = V4L2_CAP_STREAMING | dir;
@@ -875,17 +877,36 @@ static int zoran_init_video_device(struct zoran *zr, struct video_device *video_
 	video_dev->vfl_dir = VFL_DIR_RX;
 	zoran_queue_init(zr, &zr->vq, V4L2_BUF_TYPE_VIDEO_CAPTURE);
 
+	/*
+	 * zr->video_dev is allocated by zoran_init_video_devices() and the
+	 * caller frees it again if registration fails. If device_register()
+	 * fails inside video_register_device(), the V4L2 core may still run
+	 * vdev->release() from its put_device() cleanup path. Use an empty
+	 * release callback while registering, so that this failure path cannot
+	 * free zr->video_dev before control returns to the caller.
+	 *
+	 * Restore zoran_vdev_release() after successful registration, since
+	 * the V4L2 core owns the video_device lifetime from that point on.
+	 */
+	video_dev->release = video_device_release_empty;
+
 	err = video_register_device(video_dev, VFL_TYPE_VIDEO, video_nr[zr->id]);
-	if (err < 0)
+	if (err < 0) {
+		video_dev->release = release;
 		return err;
+	}
+
+	video_dev->release = release;
 	video_set_drvdata(video_dev, zr);
 	return 0;
 }
 
 static void zoran_exit_video_devices(struct zoran *zr)
 {
+	if (!zr->video_dev)
+		return;
 	video_unregister_device(zr->video_dev);
-	kfree(zr->video_dev);
+	zr->video_dev = NULL;
 }
 
 static int zoran_init_video_devices(struct zoran *zr)
@@ -897,8 +918,10 @@ static int zoran_init_video_devices(struct zoran *zr)
 		return -ENOMEM;
 
 	err = zoran_init_video_device(zr, zr->video_dev, V4L2_CAP_VIDEO_CAPTURE);
-	if (err)
+	if (err) {
 		kfree(zr->video_dev);
+		zr->video_dev = NULL;
+	}
 	return err;
 }
 
-- 
2.43.0