[PATCH v3 0/2] media: v4l2-subdev: Fix NULL dereferences racing with unbind

[PATCH v3 0/2] media: v4l2-subdev: Fix NULL dereferences racing with unbind

[Nicola Fiorillo]

Hi,

v4l2_device_unregister_subdev() clears sd->v4l2_dev and, through
media_gobj_destroy(), sd->entity.graph_obj.mdev before it unregisters
the sub-device node, and drivers that call media_device_unregister()
first, as vimc does, clear the latter even earlier. subdev_open()
dereferences both pointers, and the EXT_CTRLS ioctls the first one, so a
driver unbind racing with an open or an ioctl on the node oopses. syzbot
hits the open() case by unbinding vimc.

Patch 2/3 of v2, now patch 1, checked the two pointers in subdev_open().
That only narrows the race: the sub-device can be unregistered right
after the check. A patch along the same lines was also posted in [1].
This version takes a different approach: it stops reading the
sub-device's registration state in these file operations and uses the
node's own vdev->v4l2_dev instead, which unregistration never clears.
subdev_open() and the EXT_CTRLS ioctls no longer read a pointer that the
V4L2 core or the driver core clears when a driver is unbound.

What this does not fix is the lifetime of the sub-device and of the
media device when a driver goes away with file handles open, which is
the known limitation Sakari mentioned on v2 [2]. The commit messages say
so.

Changes since v2:
- Patch 1 (was 2/3): use vdev->v4l2_dev->mdev instead of checking
  sd->v4l2_dev and sd->entity.graph_obj.mdev, and fail with -ENODEV if
  the media device's driver has been unbound meanwhile.
- Patch 2 is new: the same dereference of sd->v4l2_dev in the EXT_CTRLS
  ioctls, found by reading the code.
- Dropped the two ipu6 patches of v2 (1/3 and 3/3), as I said I would
  in [3].
- Rebased on media next.

Testing:
- Built with W=1 (allmodconfig) at each patch of the series on media
  next 8e26d4c20ed2.
- Run in QEMU/KVM (x86_64, 4 vCPUs, KASAN generic inline, vimc built
  in) on media next 2dcdfb625c3b, which has the same v4l2-subdev.c,
  one boot per 60 s run. 16 threads either open and close vimc's
  sub-device nodes, or loop on VIDIOC_G_EXT_CTRLS on handles they keep
  open across unbinds, while another thread unbinds and rebinds vimc
  through sysfs every 1 or 30 ms. Each oops kills the thread that hits
  it; the test re-creates up to 100 per run, so the counts are capped:

    kernel              open() runs       EXT_CTRLS runs
    media next          116 + 116 oops    -
    + patch 1           0 + 0             116 + 116 oops
    + patches 1 and 2   0 + 0             0 + 0

  The oopses are KASAN null-ptr-derefs in subdev_open() and
  subdev_do_ioctl() respectively. The EXT_CTRLS runs are not done on
  plain media next, since they open the nodes too and would oops in
  subdev_open() as well. The open() runs with patch 1 and all the runs
  with both patches showed no oops, KASAN report or warning at all:
  about 8 million successful opens, 1.1 million opens refused with
  -ENODEV while vimc was being unbound (the test does not tell
  v4l2_open()'s own check from the new one), and, with both patches,
  880 million EXT_CTRLS ioctls.
- Run on the IPU6 tablet where I first hit the open() oops (Intel N100,
  Chuwi Hi10 X1), with two GalaxyCore sensors, GC5035 and GC8034, whose
  drivers I have not posted yet. Kernel: media next 9cfc1aca0781 plus
  those drivers and their ipu-bridge and int3472 changes, with KASAN
  (generic inline), lockdep and UBSAN. 4 processes either open and
  close every sub-device node, or loop on VIDIOC_G_EXT_CTRLS on the
  sensor's node, keeping it open across unbinds, while the sensor's
  I2C driver is unbound and rebound 300 times through sysfs. One run
  per sensor and test:

    kernel              open() runs          EXT_CTRLS runs
    media next          oops at the 4th      -
                        unbind of the first
    + patches 1 and 2   0 + 0                0 + 0

  On media next the oops is the KASAN null-ptr-deref in subdev_open().
  The test stops at the first oops, so the other runs were not done
  there, and the tablet gives no before and after for patch 2. With
  both patches: 2.9 million successful opens, 1048 refused with
  -ENODEV (again from either check), 81 million EXT_CTRLS ioctls, and
  v4l2-compliance and a 10-frame capture from each sensor still pass
  after the runs. One warning in the open() run on GC5035:
  DEBUG_LOCKS_WARN_ON(lock->magic != lock), from
  __v4l2_subdev_state_alloc() in subdev_open(). The GC5035 driver uses
  its control handler's mutex as the state lock, and the open took it
  after the unbind had destroyed it in v4l2_ctrl_handler_free(): that
  is the lifetime limitation above, which these patches do not touch.
- syzbot has not tested the series: its report has no public
  reproducer.
- I can post the test program if that helps.

[1] https://lore.kernel.org/linux-media/20260919162709.314464-1-ngocthang2710.1999@gmail.com/
[2] https://lore.kernel.org/linux-media/aqUoLr0JFGEBiJIf@kekkonen.localdomain/
[3] https://lore.kernel.org/linux-media/178921203630.98144.13238972861597227362@gmail.com/

Nicola Fiorillo (2):
  media: v4l2-subdev: Fix NULL pointer dereference in subdev_open()
  media: v4l2-subdev: Fix NULL pointer dereference in the EXT_CTRLS
    ioctls

 drivers/media/v4l2-core/v4l2-subdev.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)


base-commit: 8e26d4c20ed218db6c3123480a4a4e2545680566
-- 
2.47.3

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

[Nicola Fiorillo]

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
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@syzkaller.appspotmail.com
Closes: https://syzkaller.appspot.com/bug?extid=74de6401dbdd377b5746
Fixes: 61f5db549dde ("[media] v4l: Make v4l2_subdev inherit from media_entity")
Cc: stable@vger.kernel.org
Assisted-by: LLM
Signed-off-by: Nicola Fiorillo <nicfio@gmail.com>
---
 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;
 		if (!try_module_get(owner)) {
 			ret = -EBUSY;
 			goto err;
-- 
2.47.3

[PATCH v3 2/2] media: v4l2-subdev: Fix NULL pointer dereference in the EXT_CTRLS ioctls

[Nicola Fiorillo]

VIDIOC_G_EXT_CTRLS, VIDIOC_S_EXT_CTRLS and VIDIOC_TRY_EXT_CTRLS on a
sub-device node pass sd->v4l2_dev->mdev to the control framework.
v4l2_device_unregister_subdev() clears sd->v4l2_dev before it
unregisters the device node, and nothing serialises the
video_is_registered() checks in v4l2_ioctl() and subdev_do_ioctl_lock()
against it: sub-device nodes have no vdev->lock, and unregistration
would not take it anyway. An EXT_CTRLS ioctl on a file handle opened
before a driver is unbound can therefore dereference NULL:

  KASAN: null-ptr-deref in range [0x0000000000000008-0x000000000000000f]
  RIP: 0010:subdev_do_ioctl+0x1926/0x2550
  Call Trace:
   subdev_do_ioctl_lock+0x246/0x5c0
   video_usercopy+0x47b/0xcc0
   v4l2_ioctl+0x187/0x1f0
   __x64_sys_ioctl+0x134/0x1c0

Use vdev->v4l2_dev->mdev instead, as the previous patch does in
subdev_open(), and as the video node ioctls already do.

Found by reading the code, then reproduced in QEMU with KASAN: with the
previous patch applied, threads looping on VIDIOC_G_EXT_CTRLS on vimc's
sub-device nodes while vimc is unbound and rebound hit the oops above;
with this patch the same test shows no oops, KASAN report or warning.

Fixes: c41e9cff704a ("media: v4l2-ctrls: support g/s_ext_ctrls for requests")
Cc: stable@vger.kernel.org
Assisted-by: LLM
Signed-off-by: Nicola Fiorillo <nicfio@gmail.com>
---
 drivers/media/v4l2-core/v4l2-subdev.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index c56ca328f..1a2ae7c13 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -775,19 +775,19 @@ static long subdev_do_ioctl(struct file *file, unsigned int cmd, void *arg,
 		if (!vfh->ctrl_handler)
 			return -ENOTTY;
 		return v4l2_g_ext_ctrls(vfh->ctrl_handler,
-					vdev, sd->v4l2_dev->mdev, arg);
+					vdev, vdev->v4l2_dev->mdev, arg);
 
 	case VIDIOC_S_EXT_CTRLS:
 		if (!vfh->ctrl_handler)
 			return -ENOTTY;
 		return v4l2_s_ext_ctrls(vfh, vfh->ctrl_handler,
-					vdev, sd->v4l2_dev->mdev, arg);
+					vdev, vdev->v4l2_dev->mdev, arg);
 
 	case VIDIOC_TRY_EXT_CTRLS:
 		if (!vfh->ctrl_handler)
 			return -ENOTTY;
 		return v4l2_try_ext_ctrls(vfh->ctrl_handler,
-					  vdev, sd->v4l2_dev->mdev, arg);
+					  vdev, vdev->v4l2_dev->mdev, arg);
 
 	case VIDIOC_DQEVENT:
 		if (!(sd->flags & V4L2_SUBDEV_FL_HAS_EVENTS))
-- 
2.47.3

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@syzkaller.appspotmail.com
> Closes: https://syzkaller.appspot.com/bug?extid=74de6401dbdd377b5746
> Fixes: 61f5db549dde ("[media] v4l: Make v4l2_subdev inherit from media_entity")
> Cc: stable@vger.kernel.org
> Assisted-by: LLM
> Signed-off-by: Nicola Fiorillo <nicfio@gmail.com>
> ---
>  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

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

[Nicola Fiorillo]

Hi Laurent,

Thank you for looking at it.

On 6.12.86: agreed, that line does not belong in the commit message.
The runs that matter are the ones on media next described in the cover
letter, and those are what the commit message should have referred to.

On the fix: agreed as well, it does not fix the problem. subdev_open()
now goes through vdev->v4l2_dev->mdev, which is only valid as long as
the driver keeps the v4l2_device and the media_device alive. vimc does,
through its v4l2_device release callback, but ipu6-isys embeds both in
struct ipu6_isys, allocated with devm_kzalloc(), so they are freed when
the driver is unbound.

I checked this on the IPU6 tablet (media next 9cfc1aca0781 plus the
sensor drivers mentioned in the cover letter, without this series):
with the media device, a video node and a CSI-2 sub-device node of
isys held open, I unbound isys, then issued one ioctl on each node and
closed them. KASAN reports use-after-free in v4l2_ioctl() on the video
node, then on close in v4l2_release(), v4l2_prio_close() and
v4l2_device_release() on the struct ipu6_isys allocated in
isys_probe() and freed by devres at unbind, and more in subdev_close()
and __vb2_queue_free().

Patch 2/2 rests on the same vdev->v4l2_dev assumption, so I am
withdrawing the whole series rather than keeping half of it. The
underlying issue is the lifetime of the objects in the driver, and
that needs a proper fix along the lines of what Hans did for em28xx,
not another check in the file operations.

Thanks,
Nicola

Re: [PATCH v3 2/2] media: v4l2-subdev: Fix NULL pointer dereference in the EXT_CTRLS ioctls

[Sakari Ailus]

Hi Nicola,

On Thu, Oct 08, 2026 at 06:26:00AM +0200, Nicola Fiorillo wrote:
> VIDIOC_G_EXT_CTRLS, VIDIOC_S_EXT_CTRLS and VIDIOC_TRY_EXT_CTRLS on a
> sub-device node pass sd->v4l2_dev->mdev to the control framework.
> v4l2_device_unregister_subdev() clears sd->v4l2_dev before it
> unregisters the device node, and nothing serialises the
> video_is_registered() checks in v4l2_ioctl() and subdev_do_ioctl_lock()
> against it: sub-device nodes have no vdev->lock, and unregistration
> would not take it anyway. An EXT_CTRLS ioctl on a file handle opened
> before a driver is unbound can therefore dereference NULL:

Unregistering a sub-device node isn't doable safely currently. Addressing
this properly requires much more than this patch does, and also I'm afraid
this patch isn't part of properly addressing this either.

-- 
Kind regards,

Sakari Ailus