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

[Laurent Pinchart <laurent.pinchart@ideasonboard.com>]

On Sat, Sep 19, 2026 at 11:27:09PM +0700, Nguyen Ngoc Thang wrote:
> 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@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=74de6401dbdd377b5746
> Fixes: 218bf10e39ed ("media: v4l2-subdev: handle module refcounting here")
> Cc: stable@vger.kernel.org
> Signed-off-by: Nguyen Ngoc Thang <ngocthang2710.1999@gmail.com>
> ---
> 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;
>  	}

This seems the kind of completely wrong fix that would be generated by
an LLM.

>  
>  	if (sd->internal_ops && sd->internal_ops->open) {

-- 
Regards,

Laurent Pinchart