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

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

On Sun, Sep 20, 2026 at 12:12:11AM +0700, Ngọc Thắng Nguyễn wrote:
> 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:

By actually reasoning about it without the use of an LLM.

>   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 đã 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@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