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 --- 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