linux-kernel - Re: [PATCH] xfs: Verify DA node btree hash order

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Message-ID: <dyo3cdnqg3zocge2ygspovdlrjjo2dbwefbvq6w5mcbjgs3bdj@diwkyidcrpjg>
Date: Wed, 30 Apr 2025 11:23:57 +0200
From: Carlos Maiolino <cem@...nel.org>
To: Charalampos Mitrodimas <charmitro@...teo.net>
Cc: linux-xfs@...r.kernel.org, linux-kernel@...r.kernel.org
Subject: Re: [PATCH] xfs: Verify DA node btree hash order

On Sat, Apr 12, 2025 at 08:03:57PM +0000, Charalampos Mitrodimas wrote:
> The xfs_da3_node_verify() function checks the integrity of directory
> and attribute B-tree node blocks. However, it was missing a check to
> ensure that the hash values of the btree entries within the node are
> strictly increasing, as required by the B-tree structure.
> 
> Add a loop to iterate through the btree entries and verify that each
> entry's hash value is greater than the previous one. If an
> out-of-order hash value is detected, return failure to indicate
> corruption.
> 
> This addresses the "XXX: hash order check?" comment and improves
> corruption detection for DA node blocks.
> 
> Signed-off-by: Charalampos Mitrodimas <charmitro@...teo.net>
> ---
>  fs/xfs/libxfs/xfs_da_btree.c | 11 ++++++++++-
>  1 file changed, 10 insertions(+), 1 deletion(-)
> 
> diff --git a/fs/xfs/libxfs/xfs_da_btree.c b/fs/xfs/libxfs/xfs_da_btree.c
> index 17d9e6154f1978ce5a5cb82176eea4d6b9cd768d..6c748911e54619c3ceae9b81f55cf61da6735f01 100644
> --- a/fs/xfs/libxfs/xfs_da_btree.c
> +++ b/fs/xfs/libxfs/xfs_da_btree.c
> @@ -247,7 +247,16 @@ xfs_da3_node_verify(
>  	    ichdr.count > mp->m_attr_geo->node_ents)
>  		return __this_address;
> 
> -	/* XXX: hash order check? */
> +	/* Check hash order */
> +	uint32_t prev_hash = be32_to_cpu(ichdr.btree[0].hashval);
> +
> +	for (int i = 1; i < ichdr.count; i++) {
> +		uint32_t curr_hash = be32_to_cpu(ichdr.btree[i].hashval);
> +
> +		if (curr_hash <= prev_hash)
> +			return __this_address;
> +		prev_hash = curr_hash;
> +	}

Hmmm. Do you have any numbers related to the performance impact of this patch?

IIRC for very populated directories we can end up having many entries here. It's
not uncommon to have filesystems with millions of entries in a single directory.
Now we'll be looping over all those entries here during verification, which could
scale to many interactions on this loop.
I'm not sure if I'm right here, but this seems to add a big performance penalty
for directory writes, so I'm curious about the performance implications of this
patch.

> 
>  	return NULL;
>  }
> 
> ---
> base-commit: ecd5d67ad602c2c12e8709762717112ef0958767
> change-id: 20250412-xfs-hash-check-be7397881a2c
> 
> Best regards,
> --
> Charalampos Mitrodimas <charmitro@...teo.net>
>