⬢ DragonFlyBSD Kernel Audit
DF-3168 / fix.diff
← back to finding ↓ download raw
--- a/sys/vfs/hammer/hammer_rebalance.c
+++ b/sys/vfs/hammer/hammer_rebalance.c
@@ -146,6 +146,34 @@
 		}
 
 		/*
+		 * SECURITY: The on-disk node count is CRC-validated when
+		 * the node is loaded but is never bounds-validated.  A
+		 * corrupted or crafted filesystem image can present an
+		 * arbitrarily large (or negative) count which would be
+		 * consumed below (leaf index skip at count-1 and the
+		 * elms[cursor.index] extraction) and by rebalance_node()
+		 * (element copy bound and avg_elms pack bound), causing
+		 * out-of-bounds kernel reads and writes.
+		 */
+		{
+			int maxcnt;
+
+			maxcnt = hammer_node_max_elements(
+					cursor.node->ondisk->type);
+			if (maxcnt < 0 ||
+			    cursor.node->ondisk->count < 0 ||
+			    cursor.node->ondisk->count > maxcnt) {
+				hdkprintf("rebalance: bad node %016jx "
+					  "type %c count %d\n",
+					(intmax_t)cursor.node->node_offset,
+					cursor.node->ondisk->type,
+					cursor.node->ondisk->count);
+				error = EIO;
+				break;
+			}
+		}
+
+		/*
 		 * We only care about internal nodes visited for the last
 		 * time on the way up... that is, a trailing scan of the
 		 * internal node after all of its children have been recursed
@@ -327,6 +355,26 @@
 	TAILQ_FOREACH(item, &lockroot.list, entry) {
 		if (hammer_debug_general & 0x1000)
 			hdkprintf("add count %d\n", item->count);
+		/*
+		 * SECURITY: item->count comes straight from the on-disk
+		 * child node header (set in hammer_btree_lock_children)
+		 * and is used both as the element copy bound in the pack
+		 * loop and, through `count'/avg_elms, as the write index
+		 * into the fixed-size kmalloc'd node copies below.
+		 * Without this check a crafted image drives avg_elms past
+		 * the 63-element copy array and overflows the copies.
+		 */
+		if (item->count < 0 ||
+		    item->count > hammer_node_max_elements(
+					item->node->ondisk->type)) {
+			hdkprintf("rebalance: bad child node %016jx "
+				  "type %c count %d\n",
+				(intmax_t)item->node->node_offset,
+				item->node->ondisk->type,
+				item->count);
+			error = EIO;
+			goto done;
+		}
 		count += item->count;
 		KKASSERT(item->node->ondisk->type == type1);
 	}