DF-5765 / fix.diff
1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 16 17 18 19 20 21 22 23 24 25 26 27 28 29 30 31 32 33 34 35 36 37 38 39 40 41 42 43 44 45 46 47 48 49 50 51 52 53 54 55 56 57 58 59 60 61 62 63 64 65 66 67 68 69 70 71 72 73 74 75 76 77 78 79 80 81 82 83 84 85 86 87 88 89 90 91 92 93 94 95 96 97 98 99 100 101 102 103 104 105 106 107 108 109 110 111 112 113 114 115 116 117 118 119 120 121 122 123 124 125 126 127 128 129 130 131 132 133 134 135 136 137 138 139 140 141 142 143 144 145 146 147 148 149 150 151 152 | 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; } |