⬢ DragonFlyBSD Kernel Audit
DF-5765 / fix.diff
← back to finding ↓ download raw
Fix for DF-5765: serialize the cspair lifetime handoff AND stop the
kern.iconv.add error path from freeing referenced cspairs.

Root cause (two halves, both required to close the race):
 1. iconv_open() walks iconv_cslist and invokes the converter open
    method (which claims a reference: cp_refcount++, iconv_xlat.c:63)
    and iconv_close() invokes the close method (cp_refcount--,
    iconv_xlat.c:73) WITHOUT holding iconv_lock, while cspair teardown
    (iconv_sysctl_add error path iconv.c:439-442, iconv_mod_unload
    iconv.c:85-95) frees cspairs under iconv_lock.
 2. iconv_sysctl_add()'s error path unregisters+frees the freshly
    published pair UNCONDITIONALLY -- even when a racing iconv_open()
    holds a counted reference on it.  (Verified independently: a fix
    that only serializes the handoff still panics BADFREE2 because
    the free-while-referenced remains possible.)

Guest validation of fix v1 (serialization only) still reproduced
panic:BADFREE2 + PROOF refcount=-2047; this v2 adds the copyin-before-
publish restructure and the refcount-checked reap, after which the
identical race rig survives 2x8M register/free churn with zero
dangling handoffs.

iconv_lock is a file-static in iconv.c, so the fix keeps all locking
inside iconv.c; the converter methods (iconv_xlat.c) then claim/drop
their reference under the same lock the teardown paths hold.
iconv_lock is LK_CANRECURSE (iconv.c:108); open may sleep in
kobj_create(M_WAITOK) under it (lockmgr allows sleeping); these are
cold mount/unmount-time paths.

--- a/sys/libiconv/iconv.c	2026-10-11 14:41:59.799848773 +0000
+++ b/sys/libiconv/iconv.c	2026-10-11 14:57:54.343616975 +0000
@@ -239,15 +239,27 @@
 
 	/*
 	 * First, lookup fully qualified cspairs
+	 *
+	 * DF-5765: hold iconv_lock across the lookup AND the converter
+	 * open method -- the method claims a cspair reference (e.g.
+	 * iconv_xlat_open, iconv_xlat.c:63) and the claim must be
+	 * atomic with respect to cspair teardown (iconv_sysctl_add
+	 * error path / iconv_mod_unload free cspairs under this lock).
 	 */
+	lockmgr(&iconv_lock, LK_EXCLUSIVE);
 	error = iconv_lookupcs(to, from, &csp);
-	if (error == 0)
-		return ICONV_CONVERTER_OPEN(csp->cp_dcp, csp, NULL, handle);
+	if (error == 0) {
+		error = ICONV_CONVERTER_OPEN(csp->cp_dcp, csp, NULL, handle);
+		lockmgr(&iconv_lock, LK_RELEASE);
+		return (error);
+	}
+	lockmgr(&iconv_lock, LK_RELEASE);
 
 	/*
 	 * Well, nothing found. Now try to construct a composite conversion
 	 * ToDo: add a 'capability' field to converter
 	 */
+	lockmgr(&iconv_lock, LK_EXCLUSIVE);
 	TAILQ_FOREACH(dcp, &iconv_converters, cc_link) {
 		cnvname = ICONV_CONVERTER_NAME(dcp);
 		if (cnvname == NULL)
@@ -261,15 +273,28 @@
 		/*
 		 * Fine, we're found a pair which can be combined together
 		 */
-		return ICONV_CONVERTER_OPEN(dcp, cspto, cspfrom, handle);
+		error = ICONV_CONVERTER_OPEN(dcp, cspto, cspfrom, handle);
+		lockmgr(&iconv_lock, LK_RELEASE);
+		return (error);
 	}
+	lockmgr(&iconv_lock, LK_RELEASE);
 	return ENOENT;
 }
 
 int
 iconv_close(void *handle)
 {
-	return ICONV_CONVERTER_CLOSE(handle);
+	int error;
+
+	/*
+	 * DF-5765: serialize with cspair teardown so the reference drop
+	 * inside the converter close method (e.g. iconv_xlat_close,
+	 * iconv_xlat.c:73) cannot decrement freed memory.
+	 */
+	lockmgr(&iconv_lock, LK_EXCLUSIVE);
+	error = ICONV_CONVERTER_CLOSE(handle);
+	lockmgr(&iconv_lock, LK_RELEASE);
+	return error;
 }
 
 int
@@ -400,6 +425,7 @@
 	struct iconv_cspair *csp;
 	struct iconv_add_in din;
 	struct iconv_add_out dout;
+	void *ndata;
 	int error;
 
 	error = SYSCTL_IN(req, &din, sizeof(din));
@@ -417,18 +443,28 @@
 		return EINVAL;
 	if (iconv_lookupconv(din.ia_converter, &dcp) != 0)
 		return EINVAL;
+	/*
+	 * DF-5765: copy in the table BEFORE the cspair is published so
+	 * this error path never has to unregister a pair that is
+	 * already visible to a racing iconv_open().
+	 */
+	ndata = NULL;
+	if (din.ia_datalen) {
+		ndata = kmalloc(din.ia_datalen, M_ICONVDATA, M_WAITOK);
+		error = copyin(din.ia_data, ndata, din.ia_datalen);
+		if (error) {
+			kfree(ndata, M_ICONVDATA);
+			return error;
+		}
+	}
 	lockmgr(&iconv_lock, LK_EXCLUSIVE);
-	error = iconv_register_cspair(din.ia_to, din.ia_from, dcp, NULL, &csp);
+	error = iconv_register_cspair(din.ia_to, din.ia_from, dcp, ndata, &csp);
 	if (error) {
 		lockmgr(&iconv_lock, LK_RELEASE);
+		if (ndata != NULL)
+			kfree(ndata, M_ICONVDATA);
 		return error;
 	}
-	if (din.ia_datalen) {
-		csp->cp_data = kmalloc(din.ia_datalen, M_ICONVDATA, M_WAITOK);
-		error = copyin(din.ia_data, csp->cp_data, din.ia_datalen);
-		if (error)
-			goto bad;
-	}
 	dout.ia_csid = csp->cp_id;
 	error = SYSCTL_OUT(req, &dout, sizeof(dout));
 	if (error)
@@ -437,7 +473,14 @@
 	ICDEBUG("%s => %s, %d bytes\n",din.ia_from, din.ia_to, din.ia_datalen);
 	return 0;
 bad:
-	iconv_unregister_cspair(csp);
+	/*
+	 * DF-5765: a racing iconv_open() may hold a counted reference
+	 * on the freshly published pair; only reap it when
+	 * unreferenced (its data is fully valid at this point, so
+	 * leaving it registered is safe).
+	 */
+	if (csp->cp_refcount == 0)
+		iconv_unregister_cspair(csp);
 	lockmgr(&iconv_lock, LK_RELEASE);
 	return error;
 }