⬢ DragonFlyBSD Kernel Audit
DF-2620 / fix.diff
← back to finding ↓ download raw 
 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
Fix DF-2620: hammer2 unclamped on-disk pfs_nmasters drives OOB thread-array
access in hammer2_xop_helper_cleanup() and leaves live xop threads inside a
freed array.

Root cause (two defects, both verified by PoC on the stock INVARIANTS
kernel):

(1) sys/vfs/hammer2/hammer2_vfsops.c hammer2_pfsalloc(): pmp->pfs_nmasters
    is copied verbatim from the on-disk PFS inode field
    ripdata->meta.pfs_nmasters (uint8_t) with no clamp to
    HAMMER2_MAXCLUSTER (= 8).  A crafted image with pfs_nmasters = 255
    makes hammer2_xop_helper_cleanup() iterate thrs[0..254] over
    xop_groups[] whose groups are only thrs[HAMMER2_MAXCLUSTER] wide,
    reading .td far past the allocation -> page fault in
    hammer2_xop_helper_cleanup+0x5a (reproduced; also: garbage .td leads
    to hammer2_thr_delete() on out-of-bounds memory: bit-set writes to
    garbage flags words, thr->pmp = NULL writes, kfree(thr->scratch) of
    a wild pointer, KKASSERT(TAILQ_EMPTY(&thr->xopq)) on OOB data).

(2) sys/vfs/hammer2/hammer2_admin.c hammer2_xop_helper_cleanup(): the
    teardown loop is bounded by pmp->pfs_nmasters instead of the actual
    cluster width.  When pfs_nmasters < iroot->cluster.nchains (e.g. a
    MASTER + SLAVE cluster of one pfs_clid mounted from two devices,
    where visible masters = 1 but nchains = 2), the loop deletes only
    the thrs[0] column and then kfrees xop_groups while the thrs[1..]
    columns' live kernel threads are still running inside the freed
    array; the later per-chain cleanup in hammer2_pfsdealloc() /
    hammer2_pfsfree() is skipped because pmp->xop_groups is already
    NULL.  Reproduced: page fault in hammer2_primary_xops_thread+0x2d9
    during umount -f.

Fix:
- Clamp the ingested value to HAMMER2_MAXCLUSTER at the single place it
  enters the pmp from disk (the informational "masters not currently
  discoverable" semantics are preserved up to the array width limit).
- Bound the cleanup loop by the real cluster width (capped at
  HAMMER2_MAXCLUSTER), never by pfs_nmasters: slots with td == NULL are
  skipped by hammer2_thr_delete(), so iterating the full width is safe
  and guarantees no live thread remains inside the array being freed.

Not addressed here (related robustness, separate issue): with a crafted
pfs_nmasters >= 2 on a single-chain mount, nquorum can never be met and
hammer2_vfs_root() loops forever at sys/vfs/hammer2/hammer2_vfsops.c:1966
(the "h2root" tsleep loop) -- mount succeeds but the first access to the
mountpoint hangs.  That infinite loop deserves its own error path.

--- a/sys/vfs/hammer2/hammer2_vfsops.c
+++ b/sys/vfs/hammer2/hammer2_vfsops.c
@@ -520,6 +520,12 @@ hammer2_pfsalloc(hammer2_chain_t *chain,
 	 *
 	 * (This informs us of masters that might not currently be
 	 *  discoverable by this mount).
+	 *
+	 * DF-2620: the on-disk value is untrusted and must never exceed
+	 * the maximum cluster width (it is used as an index bound by
+	 * hammer2_xop_helper_cleanup() over xop_groups[].thrs[]).
 	 */
 	if (ripdata && pmp->pfs_nmasters < ripdata->meta.pfs_nmasters) {
 		pmp->pfs_nmasters = ripdata->meta.pfs_nmasters;
+		if (pmp->pfs_nmasters > HAMMER2_MAXCLUSTER)
+			pmp->pfs_nmasters = HAMMER2_MAXCLUSTER;
 	}

 	/*
--- a/sys/vfs/hammer2/hammer2_admin.c
+++ b/sys/vfs/hammer2/hammer2_admin.c
@@ -451,13 +451,25 @@ hammer2_xop_helper_cleanup(hammer2_pfs_t *pmp)
 {
 	int i;
 	int j;
+	int nchains;

 	if (pmp->xop_groups == NULL) {
 		KKASSERT(pmp->has_xop_threads == 0);
 		return;
 	}

-	for (i = 0; i < pmp->pfs_nmasters; ++i) {
+	/*
+	 * DF-2620: pfs_nmasters originates from disk and may be larger
+	 * than the array width (out-of-bounds access below) or smaller
+	 * than the number of chains with live threads (threads left
+	 * running inside the array we are about to free).  Iterate the
+	 * actual cluster width instead; hammer2_thr_delete() silently
+	 * skips slots with td == NULL.
+	 */
+	nchains = pmp->iroot ? pmp->iroot->cluster.nchains : 0;
+	if (nchains > HAMMER2_MAXCLUSTER)
+		nchains = HAMMER2_MAXCLUSTER;
+	for (i = 0; i < nchains; ++i) {
 		for (j = 0; j < hammer2_xop_nthreads; ++j) {
 			if (pmp->xop_groups[j].thrs[i].td)
 				hammer2_thr_delete(&pmp->xop_groups[j].thrs[i]);
 		}
 	}