[PATCH] media: v4l2-subdev: fix NULL deref in subdev_open() racing with unbind

[Nguyen Ngoc Thang]

subdev_open() dereferences sd->v4l2_dev->mdev and then
sd->entity.graph_obj.mdev->dev->driver->owner. v4l2_open() only checks
that the node is still registered, while
v4l2_device_unregister_subdev() clears sd->v4l2_dev and the entity's mdev
before it unregisters the node. Opening a sub-device node while the
driver is being unbound (e.g. vimc through sysfs) can therefore see a
NULL entity mdev, or a NULL dev->driver once remove() has finished, and
oops:

  Oops: general protection fault, probably for non-canonical address 0xdffffc0000000000
  KASAN: null-ptr-deref in range [0x0000000000000000-0x0000000000000007]
  RIP: 0010:subdev_open+0x193/0x510 drivers/media/v4l2-core/v4l2-subdev.c:115
  Call Trace:
   v4l2_open+0x1d2/0x490 drivers/media/v4l2-core/v4l2-dev.c:433
   chrdev_open+0x234/0x6a0 fs/char_dev.c:411

Read the entity's mdev once and treat NULL as "no media device". Take
the driver module reference under device_lock(), which is what unbind
holds while it clears dev->driver, and fail with -ENODEV if the driver
is already gone.

Reported-by: syzbot+74de6401dbdd377b5746@xxxxxxxxxxxxxxxxxxxxxxxxx
Closes: https://syzkaller.appspot.com/bug?extid=74de6401dbdd377b5746
Fixes: 218bf10e39ed ("media: v4l2-subdev: handle module refcounting here")
Cc: stable@xxxxxxxxxxxxxxx
Signed-off-by: Nguyen Ngoc Thang <ngocthang2710.1999@xxxxxxxxx>
---
Root cause
  v4l2_open() checks video_is_registered() and then calls subdev_open().
  v4l2_device_unregister_subdev() clears sd->v4l2_dev and the entity's
  graph_obj.mdev *before* video_unregister_device() on the node, so an open
  in that window sees a NULL mdev. After remove() returns, dev->driver is
  also NULL. subdev_open() dereferences both unchecked (v4l2-subdev.c:115).

Fix
  Snapshot entity mdev once (NULL == no media device), and take the driver
  module reference under device_lock(dev), the lock unbind holds while it
  clears dev->driver; return -ENODEV if the driver is gone. Errors go
  through the existing "err" label.

Testing (QEMU x86_64, KASAN, vimc built in, syzbot's C reproducer:
16 threads opening /dev/v4l-subdevN vs. one thread doing vimc bind/unbind,
vivid.n_devs=1 to avoid minor exhaustion at boot):
  before: 32 x "RIP: subdev_open+0x193/0x510" GPF (same as syzbot report)
  after : 0 oopses, 0 KASAN reports, reproducer runs to completion
  (checked with 1 ms and 30 ms bind/unbind period)

Unrelated finding (not addressed here)
  If the sub-device node registration in vimc_probe() fails (e.g.
  "videodev: could not get a free minor"), the error path in
  vimc_register_devices() frees the entities and then
  v4l2_device_unregister() hits a slab-use-after-free. Easy to trigger
  with the same reproducer if minors are exhausted. I'll look at it
  separately.

 drivers/media/v4l2-core/v4l2-subdev.c | 20 ++++++++++++++------
 1 file changed, 14 insertions(+), 6 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index e9f81b9be9e2..cec63694a658 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -97,6 +97,7 @@ static int subdev_open(struct file *file)
 	struct video_device *vdev = video_devdata(file);
 	struct v4l2_subdev *sd = vdev_to_v4l2_subdev(vdev);
 	struct v4l2_subdev_fh *subdev_fh;
+	struct media_device *mdev;
 	int ret;
 
 	subdev_fh = kzalloc_obj(*subdev_fh);
@@ -112,15 +113,22 @@ static int subdev_open(struct file *file)
 	v4l2_fh_init(&subdev_fh->vfh, vdev);
 	v4l2_fh_add(&subdev_fh->vfh, file);
 
-	if (sd->v4l2_dev->mdev && sd->entity.graph_obj.mdev->dev) {
-		struct module *owner;
+	/* Unregistration clears the entity's mdev without waiting for open. */
+	mdev = READ_ONCE(sd->entity.graph_obj.mdev);
+	if (mdev && mdev->dev) {
+		struct device *dev = mdev->dev;
 
-		owner = sd->entity.graph_obj.mdev->dev->driver->owner;
-		if (!try_module_get(owner)) {
+		/* Unbind clears dev->driver under the device lock. */
+		device_lock(dev);
+		if (!dev->driver)
+			ret = -ENODEV;
+		else if (!try_module_get(dev->driver->owner))
 			ret = -EBUSY;
+		else
+			subdev_fh->owner = dev->driver->owner;
+		device_unlock(dev);
+		if (ret)
 			goto err;
-		}
-		subdev_fh->owner = owner;
 	}
 
 	if (sd->internal_ops && sd->internal_ops->open) {
-- 
2.43.0