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

[Nicola Fiorillo <nicfio@gmail.com>]

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