DF-3168 / fix.diff
--- 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); } |