⬢ DragonFlyBSD Kernel Audit
DF-5378 / fix.diff
← back to finding ↓ download raw
Apply with: git apply -p0   (from /usr/src)
or:           patch -p0 < fix.diff

Fix DF-5378: use-after-free of the UVC control graph in
uvc_query_v4l2_ctrl()/uvc_query_v4l2_menu().

uvc_drv_detach() freed the entire control graph (uvc_drv_destroy_ctrl(),
which also frees sc->ctrl itself) while the /dev/videoN cdev was still live
(destroy_dev() only ran later, in uvc_v4l2_unreg()).  An unprivileged
VIDIOC_QUERYCTRL/VIDIOC_QUERYMENU issued around a device disconnect then:
  - took video->ctrl->mtx on freed memory          (uvc_ctrls.c:1107)
  - walked the freed topo_nodes STAILQ             (uvc_ctrls.c:1090)
  - wrote ctrl->info.cached_sub_info_idx           (uvc_ctrls.c:1065)
  - strlcpy'd freed sub_info->v4l2_name into the
    user-visible queryctrl/querymenu name          (uvc_ctrls.c:1001,1150)

Fix layers:
 1. uvc_drv_detach(): destroy the cdev (drains in-flight ioctls) and
    NULL video->ctrl under the ctrl lock BEFORE sc->data/sc->ctrl are freed;
 2. uvc_query_v4l2_ctrl()/uvc_query_v4l2_menu(): bail out with ENODEV when
    video->ctrl is gone;
 3. uvc_drv_destroy_ctrl(): take the ctrl mutex while tearing the graph
    down, and lockuninit() it (also fixes the missing uninit).

--- sys/bus/u4b/uvc/uvc_drv.c
+++ sys/bus/u4b/uvc/uvc_drv.c
@@ -2456,6 +2456,7 @@
 	uint8_t i = 0;
 
 	if (c) {
+		UVC_LOCK(&c->mtx);
 		STAILQ_FOREACH_SAFE(topo_node, &c->topo_nodes, link, tmp) {
 			for (i = 0; i < topo_node->controls_num; ++i) {
 				struct uvc_control *ctrl = &topo_node->controls[i];
@@ -2469,6 +2470,8 @@
 
 			uvc_free_topo_node(topo_node);
 		}
+		UVC_UNLOCK(&c->mtx);
+		lockuninit(&c->mtx);
 		kfree(c, M_UVC);
 	}
 }
@@ -2499,6 +2502,20 @@
 	//delete sysfs
 	//sysfs_video_del_device(sc->udev, sc->iface);
 
+	/* DF-5378: quiesce the /dev/videoN entry points FIRST.  destroy_dev()
+	 * drains in-flight ioctls, so after uvc_v4l2_unreg() no
+	 * VIDIOC_QUERYCTRL/QUERYMENU can be running against the graphs we
+	 * are about to free.  video->ctrl is also NULLed under its lock so
+	 * late query callers (uvc_ctrls.c) observe the teardown. */
+	if (sc->video) {
+		if (sc->video->ctrl != NULL && sc->ctrl != NULL) {
+			UVC_LOCK(&sc->video->ctrl->mtx);
+			sc->video->ctrl = NULL;
+			UVC_UNLOCK(&sc->ctrl->mtx);
+		}
+		uvc_v4l2_unreg(sc->video);
+	}
+
 	/* destroy video hard code only one */
 	uvc_drv_destroy_video(sc->video);
 
@@ -2507,8 +2524,6 @@
 	uvc_drv_destroy_ctrl(sc->ctrl);
 
 	if (sc->video) {
-		/* entry close */
-		uvc_v4l2_unreg(sc->video);
 		/* data free */
 		kfree(sc->video, M_UVC);
 		sc->video = NULL;
--- sys/bus/u4b/uvc/uvc_ctrls.c
+++ sys/bus/u4b/uvc/uvc_ctrls.c
@@ -1104,6 +1104,9 @@
 	struct uvc_control *ctrl;
 	int ret = EINVAL;
 
+	if (video->ctrl == NULL)
+		return (ENODEV);
+
 	UVC_LOCK(&video->ctrl->mtx);
 
 	ctrl = uvc_search_control(video->ctrl, query->id);
@@ -1128,6 +1131,9 @@
 	int id = qm->id;
 	int index = qm->index;
 
+	if (video->ctrl == NULL)
+		return (ENODEV);
+
 	memset(qm, 0, sizeof(*qm));
 	qm->id = id;
 	qm->index = index;