Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
From:   Soumya Negi <soumya.negi97@gmail.com>
To:     Mauro Carvalho Chehab <mchehab@kernel.org>,
        Shuah Khan <skhan@linuxfoundation.org>
Cc:     Soumya Negi <soumya.negi97@gmail.com>,
        linux-kernel-mentees@lists.linuxfoundation.org,
        linux-media@vger.kernel.org, linux-kernel@vger.kernel.org
Subject: [RFT PATCH] media: em28xx: Enable v4l2 file ops at the end of em28xx_v4l2_init()
Date:   Mon, 29 Aug 2022 06:25:48 -0700
Message-Id: <20220829132548.23520-1-soumya.negi97@gmail.com>
Precedence: bulk

Fix syzbot bug KASAN: use-after-free Read in v4l2_fh_open
Syzbot link:
https://syzkaller.appspot.com/bug?id=0e3c97f1c4112e102c9988afd5eff079350eab7f
Repro: https://syzkaller.appspot.com/text?tag=ReproC&x=12663ebdd00000

The bug arises because the em28xx driver registers a v4l2 video
device(struct video_device) with the V4L2 interface in
em28xx_v4l2_init() but the v4l2 extension initialization eventually
fails and the video device is unregistered. v4l2_open() in the V4L2
interface views the device as registered and allows calls to
em28xx_v4l2_open(). Due to race between video_unregister_device() and
v4l2_open(), em28xx_v4l2_open() is accessing video device after it has
been freed(by the release callback) and is passing it on to
v4l2_fh_open().

In em28xx_v4l2_init(), temporarily disable v4l2 file operations on
struct video_device devices(video, vbi, radio) before registering them
and enable the file ops at the end when v4l2 extension initialization
succeeds.

Reported-by: syzbot+b2391895514ed9ef4a8e@syzkaller.appspotmail.com
Signed-off-by: Soumya Negi <soumya.negi97@gmail.com>
---
 drivers/media/usb/em28xx/em28xx-video.c | 45 +++++++++++++++++++++----
 1 file changed, 39 insertions(+), 6 deletions(-)

diff --git a/drivers/media/usb/em28xx/em28xx-video.c b/drivers/media/usb/em28xx/em28xx-video.c
index 8181c0e6a25b..900a1eacb1c7 100644
--- a/drivers/media/usb/em28xx/em28xx-video.c
+++ b/drivers/media/usb/em28xx/em28xx-video.c
@@ -2320,6 +2320,19 @@ static int em28xx_v4l2_close(struct file *filp)
 	return 0;
 }
 
+/* Used to temporarily disable file operations on video_device until successful
+ * initialization in em28xx_v4l2_init().
+ */
+static const struct v4l2_file_operations em28xx_v4l_fops_empty = {
+	.owner         = THIS_MODULE,
+	.open          = NULL,
+	.release       = NULL,
+	.read          = NULL,
+	.poll          = NULL,
+	.mmap          = NULL,
+	.unlocked_ioctl = NULL,
+};
+
 static const struct v4l2_file_operations em28xx_v4l_fops = {
 	.owner         = THIS_MODULE,
 	.open          = em28xx_v4l2_open,
@@ -2375,12 +2388,22 @@ static const struct v4l2_ioctl_ops video_ioctl_ops = {
 };
 
 static const struct video_device em28xx_video_template = {
-	.fops		= &em28xx_v4l_fops,
+	.fops		= &em28xx_v4l_fops_empty,
 	.ioctl_ops	= &video_ioctl_ops,
 	.release	= video_device_release_empty,
 	.tvnorms	= V4L2_STD_ALL,
 };
 
+/* Used to temporarily disable file operations on video_device until successful
+ * initialization in em28xx_v4l2_init().
+ */
+static const struct v4l2_file_operations radio_fops_empty = {
+	.owner         = THIS_MODULE,
+	.open          = NULL,
+	.release       = NULL,
+	.unlocked_ioctl = NULL,
+};
+
 static const struct v4l2_file_operations radio_fops = {
 	.owner         = THIS_MODULE,
 	.open          = em28xx_v4l2_open,
@@ -2404,7 +2427,7 @@ static const struct v4l2_ioctl_ops radio_ioctl_ops = {
 };
 
 static struct video_device em28xx_radio_template = {
-	.fops		= &radio_fops,
+	.fops		= &radio_fops_empty,
 	.ioctl_ops	= &radio_ioctl_ops,
 	.release	= video_device_release_empty,
 };
@@ -2833,9 +2856,6 @@ static int em28xx_v4l2_init(struct em28xx *dev)
 				"can't register radio device\n");
 			goto unregister_dev;
 		}
-		dev_info(&dev->intf->dev,
-			 "Registered radio device as %s\n",
-			 video_device_node_name(&v4l2->radio_dev));
 	}
 
 	/* Init entities at the Media Controller */
@@ -2851,14 +2871,27 @@ static int em28xx_v4l2_init(struct em28xx *dev)
 	}
 #endif
 
+	/* Enable v4l2 file operations for v4l2 video video_device */
+	v4l2->vdev.fops = &em28xx_v4l_fops;
 	dev_info(&dev->intf->dev,
 		 "V4L2 video device registered as %s\n",
 		 video_device_node_name(&v4l2->vdev));
 
-	if (video_is_registered(&v4l2->vbi_dev))
+	if (video_is_registered(&v4l2->vbi_dev)) {
+		/* Enable v4l2 file operations for v4l2 vbi video_device */
+		v4l2->vbi_dev.fops = &em28xx_v4l_fops;
 		dev_info(&dev->intf->dev,
 			 "V4L2 VBI device registered as %s\n",
 			 video_device_node_name(&v4l2->vbi_dev));
+	}
+
+	if (video_is_registered(&v4l2->radio_dev)) {
+		/* Enable v4l2 file operations for v4l2 radio video_device */
+		v4l2->radio_dev.fops = &radio_fops;
+		dev_info(&dev->intf->dev,
+			 "Registered radio device as %s\n",
+			 video_device_node_name(&v4l2->radio_dev));
+	}
 
 	/* Save some power by putting tuner to sleep */
 	v4l2_device_call_all(&v4l2->v4l2_dev, 0, tuner, standby);
-- 
2.17.1