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

[Ngọc Thắng Nguyễn]

Hi Laurent,

Fair point. The patch only narrows the window: sd can still be
unregistered right after the mdev check, before internal_ops->open() runs,
so the race between v4l2_open() and v4l2_device_unregister_subdev() is not
actually closed.

What I see (syzbot repro, vimc bind/unbind vs. open of /dev/v4l-subdevN):
v4l2_device_unregister_subdev() clears sd->v4l2_dev and the entity's mdev
before video_unregister_device() on the node, while v4l2_open() only checks
video_is_registered() without any lock held across fops->open().

How would you prefer this to be fixed? For example:
  a) unregister the devnode first in v4l2_device_unregister_subdev(),
     and make open/unregister mutually exclusive somehow, or
  b) something else you have in mind.

Happy to rework it and test with the reproducer.

Thanks,
Nguyen Ngoc Thang

Vào CN, 20 thg 9, 2026 vào lúc 00:02 Laurent Pinchart
<laurent.pinchart@xxxxxxxxxxxxxxxx> đã viết:
>
> 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@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;
> >       }
>
> 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