[PATCH 0/3] media: Two oopses and a hang when unbinding a streaming sensor

[PATCH 0/3] media: Two oopses and a hang when unbinding a streaming sensor

[Nicola Fiorillo]

Unbinding a camera sensor while a capture is running is a scenario that
nothing in the IPU6 path handles: the kernel oopses twice, corrupts memory
once, and leaves the application blocked forever. None of this is caused by
the sensor drivers themselves, and all four failures are present in
mainline today.

They were found while testing two new sensor drivers on a CHUWI Hi10 X1
(Intel N100, Alder Lake-N, IPU6) on a kernel built with KASAN, UBSAN,
KMEMLEAK, PROVE_LOCKING and DETECT_HUNG_TASK. Three of them are fixed here;
the fourth, a use-after-free in the media controller, is sent separately
because it belongs to a different subsystem.

Patch 1 is a NULL pointer dereference in ipu6_isys_csi2_get_remote_desc().
The remote pad is dereferenced without being checked, and unbinding the
sensor mid-stream makes it NULL. Present since May 2024.

Patch 2 is a second NULL pointer dereference, in subdev_open().
v4l2_device_unregister_subdev() clears sd->v4l2_dev before the device node
goes away, so anything opening /dev/v4l-subdevN in that window oopses. This
one was not provoked deliberately: udev's v4l_id walked into it on its own.
The window has been open since 2011.

Patch 3 is the hang. isys_async_ops has no .unbind() callback, so nothing
tells the video nodes that the sensor is gone, and a DQBUF already waiting
in vb2_core_dqbuf() never returns. The sleep is interruptible, so
DETECT_HUNG_TASK stays quiet and the process is simply stuck until it is
killed. Reproduced 10 times out of 10 on both sensors of the machine; with
the patch, all 10 return -EIO and exit.

How each one was verified, since the three differ:

  - patch 1: the oops was provoked deliberately on the unpatched kernel
    before the fix was written
  - patch 2: reproduced itself, unprompted, with udev alone; after the fix,
    150 cycles of a reproducer with four concurrent openers left no oops
    and no leaked minors
  - patch 3: rebuilt both ways, same kernel and same test. Without it,
    3 attempts out of 3 hang; with it, 10 out of 10 wake up and return
    -EIO

The full test cycle with the three fixes in place is clean: no KASAN or
UBSAN reports, no KMEMLEAK findings, and lockdep still enabled at the end
of the run. That last detail matters: lockdep disables itself on its first
complaint and silently invalidates everything measured afterwards.

The reproducer is a shell script that streams with v4l2-ctl, unbinds the
sensor after two seconds, then rebinds it, in a loop. I am happy to post it
if that would be useful.

Nicola Fiorillo (3):
  media: ipu6: Check the remote pad before dereferencing it
  media: v4l2-subdev: Check v4l2_dev before dereferencing it in open()
  media: ipu6: Signal the video queues when a sensor is unbound

 drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c | 14 ++++++-
 drivers/media/pci/intel/ipu6/ipu6-isys.c      | 41 +++++++++++++++++++
 drivers/media/v4l2-core/v4l2-subdev.c         | 31 +++++++++++---
 3 files changed, 78 insertions(+), 8 deletions(-)

-- 
2.47.3

[PATCH 1/3] media: ipu6: Check the remote pad before dereferencing it

[Nicola Fiorillo]

Unbinding a sensor driver while a capture is running oopses the kernel:

  BUG: kernel NULL pointer dereference, address: 0000000000000020
  RIP: 0010:ipu6_isys_csi2_disable_streams+0x3c/0x70 [intel_ipu6_isys]
  Call Trace:
   v4l2_subdev_disable_streams+0x1b7/0x370 [videodev]
   ipu6_isys_video_set_streaming+0x20f/0x930 [intel_ipu6_isys]
   stop_streaming+0x102/0x110 [intel_ipu6_isys]
   __vb2_queue_cancel+0x2a/0x2d0 [videobuf2_common]
   vb2_core_queue_release+0x22/0x80 [videobuf2_common]
   _vb2_fop_release+0x58/0xb0 [videobuf2_v4l2]
   v4l2_release+0xbd/0xd0 [videodev]
   __fput+0xde/0x2a0

media_pad_remote_pad_first() returns NULL once the sensor is gone and the
link with it, but both the enable and the disable path dereference the
result unconditionally. The faulting address is the offset of the entity
member in struct media_pad.

Check it. On enable there is nothing to stream from, so refuse with
-ENOLINK. On disable the receiver still has to be stopped, so stop it and
skip only the call towards the sensor that is no longer there.

Reproduced on a CHUWI Hi10 X1 (Alder Lake-N, IPU6) running 6.12.86, with
the CSI-2 port of a sensor being unbound mid capture. The code is
unchanged in 7.2-rc7.

Fixes: 3a5c59ad926b ("media: ipu6: Rework CSI-2 sub-device streaming control")
Signed-off-by: Nicola Fiorillo <nicfio@gmail.com>
---
 drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c | 14 ++++++++++++--
 1 file changed, 12 insertions(+), 2 deletions(-)

diff --git a/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c b/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c
index 7e539a0c6..c00a82eb8 100644
--- a/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c
+++ b/drivers/media/pci/intel/ipu6/ipu6-isys-csi2.c
@@ -356,6 +356,9 @@ static int ipu6_isys_csi2_enable_streams(struct v4l2_subdev *sd,
 	int ret;
 
 	remote_pad = media_pad_remote_pad_first(&sd->entity.pads[CSI2_PAD_SINK]);
+	if (!remote_pad)
+		return -ENOLINK;
+
 	remote_sd = media_entity_to_v4l2_subdev(remote_pad->entity);
 
 	sink_streams =
@@ -392,10 +395,17 @@ static int ipu6_isys_csi2_disable_streams(struct v4l2_subdev *sd,
 		v4l2_subdev_state_xlate_streams(state, pad, CSI2_PAD_SINK,
 						&streams_mask);
 
+	ipu6_isys_csi2_set_stream(sd, NULL, 0, false);
+
+	/*
+	 * The link is gone if the sensor driver was unbound while streaming.
+	 * Stop the receiver anyway, there is just no one left to tell.
+	 */
 	remote_pad = media_pad_remote_pad_first(&sd->entity.pads[CSI2_PAD_SINK]);
-	remote_sd = media_entity_to_v4l2_subdev(remote_pad->entity);
+	if (!remote_pad)
+		return 0;
 
-	ipu6_isys_csi2_set_stream(sd, NULL, 0, false);
+	remote_sd = media_entity_to_v4l2_subdev(remote_pad->entity);
 
 	v4l2_subdev_disable_streams(remote_sd, remote_pad->index, sink_streams);
 
-- 
2.47.3

[PATCH 2/3] media: v4l2-subdev: Check v4l2_dev before dereferencing it in open()

[Nicola Fiorillo]

Unbinding a sensor driver while something opens its /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, and only then unregisters the device node. Until the node is
gone userspace can still open it, and subdev_open() dereferences
sd->v4l2_dev unconditionally. The faulting address is the offset of the
mdev member in struct v4l2_device.

The window is not a narrow one: media_device_unregister_entity() sleeps,
and the first oops seen here was not provoked at all, it was hit by v4l_id,
run by udev on the very node that was appearing and disappearing.

The same window leaves sd->entity.graph_obj.mdev NULL while
sd->v4l2_dev->mdev is not, and the second dereference on that line goes
through it. That one was found by reading the teardown path, not by
crashing on it; it arrived later, with commit 218bf10e39ed ("media:
v4l2-subdev: handle module refcounting here").

Unregistering the device node before clearing the pointers would narrow the
window but not close it, because v4l2_open() drops videodev_lock before it
calls fops->open() and the whole of v4l2_device_unregister_subdev() can run
in between. Check the pointers in subdev_open() instead.

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*. The code is unchanged in 7.2-rc7.

Fixes: 61f5db549dde ("[media] v4l: Make v4l2_subdev inherit from media_entity")
Signed-off-by: Nicola Fiorillo <nicfio@gmail.com>
---
 drivers/media/v4l2-core/v4l2-subdev.c | 31 +++++++++++++++++++++------
 1 file changed, 25 insertions(+), 6 deletions(-)

diff --git a/drivers/media/v4l2-core/v4l2-subdev.c b/drivers/media/v4l2-core/v4l2-subdev.c
index e9f81b9be..2a47b9730 100644
--- a/drivers/media/v4l2-core/v4l2-subdev.c
+++ b/drivers/media/v4l2-core/v4l2-subdev.c
@@ -97,8 +97,19 @@ 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 v4l2_device *v4l2_dev;
 	int ret;
 
+	/*
+	 * v4l2_device_unregister_subdev() clears sd->v4l2_dev and unregisters
+	 * the entity before it unregisters the device node, so an open() that
+	 * races with the sub-device going away lands here with those pointers
+	 * already gone.
+	 */
+	v4l2_dev = READ_ONCE(sd->v4l2_dev);
+	if (!v4l2_dev)
+		return -ENODEV;
+
 	subdev_fh = kzalloc_obj(*subdev_fh);
 	if (subdev_fh == NULL)
 		return -ENOMEM;
@@ -112,15 +123,23 @@ 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;
+	if (v4l2_dev->mdev) {
+		struct media_device *mdev = READ_ONCE(sd->entity.graph_obj.mdev);
 
-		owner = sd->entity.graph_obj.mdev->dev->driver->owner;
-		if (!try_module_get(owner)) {
-			ret = -EBUSY;
+		if (!mdev) {
+			ret = -ENODEV;
 			goto err;
 		}
-		subdev_fh->owner = owner;
+
+		if (mdev->dev) {
+			struct module *owner = mdev->dev->driver->owner;
+
+			if (!try_module_get(owner)) {
+				ret = -EBUSY;
+				goto err;
+			}
+			subdev_fh->owner = owner;
+		}
 	}
 
 	if (sd->internal_ops && sd->internal_ops->open) {
-- 
2.47.3

[PATCH 3/3] media: ipu6: Signal the video queues when a sensor is unbound

[Nicola Fiorillo]

isys_async_ops implements .bound() and .complete() but not .unbind(), so
nothing tells the ISYS video nodes that the sensor feeding them has gone
away. A capture that is streaming when the sensor is unbound stays
blocked in vb2_core_dqbuf() forever, waiting for a frame that can no
longer arrive:

  [<0>] vb2_core_dqbuf+0x362/0x1190 [videobuf2_common]
  [<0>] vb2_dqbuf+0xb4/0x210 [videobuf2_v4l2]
  [<0>] __video_do_ioctl+0x894/0xb30
  [<0>] video_usercopy+0x479/0xde0
  [<0>] v4l2_ioctl+0x198/0x220
  [<0>] __x64_sys_ioctl+0x134/0x1c0

The wait in __vb2_wait_for_done_vb() ends on a new buffer, on
!q->streaming, or on q->error. Tearing the sensor down sets none of the
three. The sleep is interruptible, so DETECT_HUNG_TASK stays quiet as
well and the process is simply stuck until something kills it.

Add the missing .unbind() and mark the queues of the CSI-2 receiver the
departing sensor was attached to, which is enough for DQBUF to return
-EIO. Only streaming queues are flagged: q->error is cleared by
__vb2_queue_cancel(), so flagging an idle queue would leave it in error
until the next VIDIOC_STREAMOFF.

Reproduced on a CHUWI Hi10 X1 (Alder Lake-N, IPU6) by unbinding the
sensor while v4l2-ctl was streaming, with both sensors of the machine.
Without this patch 3 attempts out of 3 hang; with it, 10 out of 10 wake
up, report "VIDIOC_DQBUF: failed: Input/output error" and exit. The same
run under KASAN reports nothing.

Fixes: f50c4ca0a820 ("media: intel/ipu6: add the main input system driver")
Signed-off-by: Nicola Fiorillo <nicfio@gmail.com>
---
 drivers/media/pci/intel/ipu6/ipu6-isys.c | 41 ++++++++++++++++++++++++
 1 file changed, 41 insertions(+)

diff --git a/drivers/media/pci/intel/ipu6/ipu6-isys.c b/drivers/media/pci/intel/ipu6/ipu6-isys.c
index c9cdeb705..8055ae169 100644
--- a/drivers/media/pci/intel/ipu6/ipu6-isys.c
+++ b/drivers/media/pci/intel/ipu6/ipu6-isys.c
@@ -31,6 +31,7 @@
 #include <media/v4l2-async.h>
 #include <media/v4l2-device.h>
 #include <media/v4l2-fwnode.h>
+#include <media/videobuf2-core.h>
 
 #include "ipu6-bus.h"
 #include "ipu6-cpd.h"
@@ -700,6 +701,45 @@ static int isys_notifier_bound(struct v4l2_async_notifier *notifier,
 	return v4l2_device_register_subdev_nodes(&isys->v4l2_dev);
 }
 
+/* The .unbind() notifier callback when a sub-device goes away */
+static void isys_notifier_unbind(struct v4l2_async_notifier *notifier,
+				 struct v4l2_subdev *sd,
+				 struct v4l2_async_connection *asc)
+{
+	struct ipu6_isys *isys =
+		container_of(notifier, struct ipu6_isys, notifier);
+	struct sensor_async_sd *s_asd =
+		container_of(asc, struct sensor_async_sd, asc);
+	struct ipu6_isys_csi2 *csi2;
+	unsigned int i;
+
+	if (s_asd->csi2.port >= isys->pdata->ipdata->csi2.nports)
+		return;
+
+	/*
+	 * The sensor is gone, so no more frames will ever arrive on the video
+	 * nodes fed by it. Tell videobuf2, or a DQBUF already blocked in
+	 * vb2_core_dqbuf() would sleep forever: nothing else in the teardown
+	 * path wakes that queue up.
+	 *
+	 * Only queues that are actually streaming are marked. The error flag
+	 * is only cleared by __vb2_queue_cancel(), so flagging an idle queue
+	 * would leave it poisoned until the next STREAMOFF.
+	 */
+	csi2 = &isys->csi2[s_asd->csi2.port];
+	for (i = 0; i < NR_OF_CSI2_SRC_PADS; i++) {
+		struct vb2_queue *q = &csi2->av[i].aq.vbq;
+
+		if (!vb2_is_streaming(q))
+			continue;
+
+		dev_dbg(&isys->adev->auxdev.dev,
+			"%s went away while streaming on %s\n", sd->name,
+			csi2->av[i].vdev.name);
+		vb2_queue_error(q);
+	}
+}
+
 static int isys_notifier_complete(struct v4l2_async_notifier *notifier)
 {
 	struct ipu6_isys *isys =
@@ -710,6 +750,7 @@ static int isys_notifier_complete(struct v4l2_async_notifier *notifier)
 
 static const struct v4l2_async_notifier_operations isys_async_ops = {
 	.bound = isys_notifier_bound,
+	.unbind = isys_notifier_unbind,
 	.complete = isys_notifier_complete,
 };
 
-- 
2.47.3