[PATCH] xfs: fix out-of-bounds da node read with a crafted record count

[PATCH] xfs: fix out-of-bounds da node read with a crafted record count

[Henry Martin]

xfs_da3_node_verify() bounds the on-disk record count with:

	/* we don't know if the node is for and attribute or directory
	 * tree, so only fail if the count is outside both bounds
	 */
	if (ichdr.count > mp->m_dir_geo->node_ents &&
	    ichdr.count > mp->m_attr_geo->node_ents)
		return __this_address;

The "fail only outside both bounds" conjunction is forced weak
because the verify pass cannot tell directory from attribute nodes.
On filesystems with a multi-fsb directory block size (mkfs.xfs -n
size=64k, sb_dirblklog > 0), m_dir_geo->node_ents is much larger than
m_attr_geo->node_ents (~8184 vs ~504 for 64k/4k), so an attribute
tree node with a count in (attr_ents, dir_ents] passes verification.
Consumers then index the in-block btree array with that count and
read out of bounds:

  xfs_da3_node_lookup_int()   btree[max - 1].hashval  (max = count)
  xfs_da3_path_shift()        btree[nodehdr.count - 1].hashval / .before

with count=8184 the read lands ~61KB past the 4K buffer, and the
out-of-bounds "before" value is then used as a child block number.
The read path is reachable by an unprivileged user via
getxattr/listxattr once the malicious image is mounted.

Tightening the verify conjunction would falsely reject legitimate
large directory nodes whose counts live in the same interval, so
enforce the per-fork bound instead where the geometry is known: at
the two consumers that index the array with the on-disk count.

This issue was discovered by Tencent CodeBuddy Security.

Fixes: f5ea110044fa ("xfs: add CRCs to dir2/da node blocks")
Cc: stable@vger.kernel.org
Signed-off-by: Henry Martin <bsdhenrymartin@gmail.com>
---
 fs/xfs/libxfs/xfs_da_btree.c | 16 ++++++++++++++++
 1 file changed, 16 insertions(+)

diff --git a/fs/xfs/libxfs/xfs_da_btree.c b/fs/xfs/libxfs/xfs_da_btree.c
index 3d02a0d7ba44a..ab3deff11111e 100644
--- a/fs/xfs/libxfs/xfs_da_btree.c
+++ b/fs/xfs/libxfs/xfs_da_btree.c
@@ -1787,6 +1787,18 @@ xfs_da3_node_lookup_int(
 		} else
 			expected_level--;
 
+		/* The verify pass cannot tell directory from attribute
+		 * nodes, so its count bound has to allow both geometries.
+		 * Here we know the fork; enforce its geometry so crafted
+		 * counts cannot make the array indexing run out of the
+		 * block.
+		 */
+		if (nodehdr.count > args->geo->node_ents) {
+			xfs_buf_mark_corrupt(blk->bp);
+			xfs_da_mark_sick(args);
+			return -EFSCORRUPTED;
+		}
+
 		max = nodehdr.count;
 		blk->hashval = be32_to_cpu(btree[max - 1].hashval);
 
@@ -2161,6 +2173,11 @@ xfs_da3_path_shift(
 		blk = &path->blk[level];
 		xfs_da3_node_hdr_from_disk(dp->i_mount, &nodehdr,
 					   blk->bp->b_addr);
+		if (nodehdr.count > args->geo->node_ents) {
+			xfs_buf_mark_corrupt(blk->bp);
+			xfs_da_mark_sick(args);
+			return -EFSCORRUPTED;
+		}
 
 		if (forward && (blk->index < nodehdr.count - 1)) {
 			blk->index++;
@@ -2227,6 +2244,11 @@ xfs_da3_path_shift(
 			blk->magic = XFS_DA_NODE_MAGIC;
 			xfs_da3_node_hdr_from_disk(dp->i_mount, &nodehdr,
 						   bp->b_addr);
+			if (nodehdr.count > args->geo->node_ents) {
+				xfs_buf_mark_corrupt(blk->bp);
+				xfs_da_mark_sick(args);
+				return -EFSCORRUPTED;
+			}
 			btree = nodehdr.btree;
 			blk->hashval = be32_to_cpu(btree[nodehdr.count - 1].hashval);
 			if (forward)
-- 
2.43.7