Re: [PATCH v3 1/2] media: v4l2-subdev: Fix NULL pointer dereference in subdev_open()

[Laurent Pinchart]

On Thu, Oct 08, 2026 at 06:25:59AM +0200, Nicola Fiorillo wrote:
> Unbinding a driver while something opens a /dev/v4l-subdevN node oopses
> the kernel:
> 
>   BUG: kernel NULL pointer dereference, address: 0000000000000008
>   RIP: 0010:subdev_open+0x8a/0x190 [videodev]
>   Call Trace:
>    v4l2_open+0xa9/0x100 [videodev]
>    chrdev_open+0xb2/0x230
>    do_dentry_open+0x14c/0x440
>    vfs_open+0x2e/0xe0
>    path_openat+0x82e/0x12d0
>    do_filp_open+0xc4/0x170
>    do_sys_openat2+0xae/0xe0
>    __x64_sys_openat+0x55/0xa0
> 
> v4l2_device_unregister_subdev() clears sd->v4l2_dev, then unregisters
> the media entity, which clears sd->entity.graph_obj.mdev, and only then
> unregisters the device node. Drivers that call media_device_unregister()
> before v4l2_device_unregister(), as vimc does, clear
> sd->entity.graph_obj.mdev even earlier, while the sub-device nodes are
> still registered. subdev_open() dereferences both pointers. v4l2_open()
> has checked that the node is registered, but it drops videodev_lock
> before calling fops->open(), so the whole unregistration can run in
> between. Reordering v4l2_device_unregister_subdev() would not help in
> the second case, and checking the two pointers in subdev_open() would
> only narrow the window.
> 
> Neither pointer is needed in subdev_open(). Both record that the
> sub-device is registered, and are cleared on purpose when it goes away,
> while open() runs on behalf of the device node. The node has its own
> pointer to the same v4l2_device, vdev->v4l2_dev, which is set when the
> node is registered and never cleared, and the V4L2 core already relies
> on it for as long as the node exists: v4l2_release() dereferences it on
> every close. The sub-device's entity is registered with
> vdev->v4l2_dev->mdev, so that is also the media device to take the
> module reference through.
> 
> That reference goes through mdev->dev->driver, which the driver core
> clears once the media device's own driver has been unbound, as when
> syzbot unbinds vimc. Read it once with READ_ONCE(), as
> dev_driver_string() does, and fail the open with -ENODEV if it is gone:
> unbinding does not unload the module, so the pointer read is either
> still valid or NULL.
> 
> With this, subdev_open() no longer reads a pointer that the V4L2 core or
> the driver core clears when a driver is unbound. It can still run on a
> sub-device that has just been unregistered, exactly like a file handle
> opened an instant earlier. The lifetime of the sub-device and of the
> media device when a driver goes away with file handles open, and module
> unloading, are a separate, known limitation that this does not address.
> 
> Reproduced on a CHUWI Hi10 X1 (Alder Lake-N, IPU6) running 6.12.86, at

We don't develop or test patcheson 6.12.86.

> cycle 7 of a loop unbinding and rebinding a sensor while four processes
> opened every /dev/v4l-subdev*. syzbot hit the second dereference on the
> same line by unbinding vimc, and so does a test in QEMU with KASAN that
> opens vimc's sub-device nodes while vimc is unbound and rebound; with
> this patch the same test shows no oops, KASAN report or warning.
> 
> Reported-by: syzbot+74de6401dbdd377b5746@xxxxxxxxxxxxxxxxxxxxxxxxx
> Closes: https://syzkaller.appspot.com/bug?extid=74de6401dbdd377b5746
> Fixes: 61f5db549dde ("[media] v4l: Make v4l2_subdev inherit from media_entity")
> Cc: stable@xxxxxxxxxxxxxxx
> Assisted-by: LLM
> Signed-off-by: Nicola Fiorillo <nicfio@xxxxxxxxx>
> ---
>  drivers/media/v4l2-core/v4l2-subdev.c | 12 ++++++++++--
>  1 file changed, 10 insertions(+), 2 deletions(-)
> 
> diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
> index a07d77e58..c56ca328f 100644
> --- a/drivers/media/v4l2-core/v4l2-subdev.c
> +++ b/drivers/media/v4l2-core/v4l2-subdev.c
> @@ -96,6 +96,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 media_device *mdev = vdev->v4l2_dev->mdev;
>  	struct v4l2_subdev_fh *subdev_fh;
>  	int ret;
>  
> @@ -112,10 +113,17 @@ 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) {
> +	if (mdev && mdev->dev) {
> +		struct device_driver *drv = READ_ONCE(mdev->dev->driver);
>  		struct module *owner;
>  
> -		owner = sd->entity.graph_obj.mdev->dev->driver->owner;
> +		/* The media device's driver has been unbound meanwhile. */
> +		if (!drv) {
> +			ret = -ENODEV;
> +			goto err;
> +		}
> +
> +		owner = drv->owner;

All of this is a hack that may reduce a race window but it doesn't fix
the problem.

>  		if (!try_module_get(owner)) {
>  			ret = -EBUSY;
>  			goto err;

-- 
Regards,

Laurent Pinchart