[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

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

[Dave Chinner]

On Tue, Oct 27, 2026 at 11:17:54PM +0800, Henry Martin wrote:
> 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.

No, it's pretty obvious that we knew about it a long, long time ago.
The bounds check we currently have catches gross corruptions, and
for the types of corruption we see in the real world (i.e. media
errors trashing entire sectors) this has been sufficient to avoid
issues. Hence there's been no priority/urgency to fix stuff like
this, because malicious images are not a threat model we are able to
defend against.

> 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;
> +		}

Again, look at the code. The node header is verified a few lines
above via xfs_da3_node_header_check(). We know that the read
verifier cannot verify everything in IO context, so we have checks
that are always done immediately after the buffer has been read
before we start using the node header.

If the new check is added there, then no other code needs to be
touched. i.e. every place we first read a da-node can be correctly
bounds checked because args->geo is available at all those call
sites...

-Dave.

-- 
Dave Chinner
dgc@kernel.org