Re: [PATCH v14 11/12] ext4: add dirdata set/get helpers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [High] ext4: unchecked EXT4_DIRENT_HASH fallback read after ext4_dirdata_get()
--

commit b0e6360d0312737d9514b8c174a474f487aed85e
Author: Artem Blagodarenko <artem.blagodarenko@gmail.com>

ext4: add dirdata set/get helpers

This commit introduces ext4_dirdata_get() and ext4_dirdata_set() to read and
write directory entry payload extensions such as LUFID and casefold hashes,
and wires them into directory lookup, insertion, and readdir paths.

> diff --git a/fs/ext4/dir.c b/fs/ext4/dir.c
> index 15816f2a5f472..12df2d2a09978 100644
> --- a/fs/ext4/dir.c
> +++ b/fs/ext4/dir.c
> @@ -302,8 +302,19 @@ static int ext4_readdir(struct file *file, struct dir_context *ctx)
>  					u32 minor_hash;
>  
>  					if (IS_CASEFOLDED(inode)) {
> -						hash = EXT4_DIRENT_HASH(de);
> -						minor_hash = EXT4_DIRENT_MINOR_HASH(de);
> +						if (ext4_has_feature_dirdata(sb) &&
> +						    (de->file_type &
> +						     EXT4_DIRENT_CFHASH)) {
> +							struct dx_hash_info hi = {};
> +							ext4_dirdata_get(de, inode,
> +								sb->s_blocksize,
> +								NULL, &hi);
> +							hash = hi.hash;
> +							minor_hash = hi.minor_hash;
> +						} else {
> +							hash = EXT4_DIRENT_HASH(de);
> +							minor_hash = EXT4_DIRENT_MINOR_HASH(de);
> +						}
>  					} else {
>  						hash = 0;
>  						minor_hash = 0;
> diff --git a/fs/ext4/inline.c b/fs/ext4/inline.c
> index 81e0ef1c88337..801fa6555ea96 100644
> --- a/fs/ext4/inline.c
> +++ b/fs/ext4/inline.c
> @@ -1359,14 +1359,20 @@ int ext4_inlinedir_to_tree(struct file *dir_file,
>  			pos += de_len;
>  		}
>  
> -		if (ext4_hash_in_dirent(dir)) {
> -			hinfo->hash = EXT4_DIRENT_HASH(de);
> -			hinfo->minor_hash = EXT4_DIRENT_MINOR_HASH(de);
> -		} else {
> -			err = ext4fs_dirhash(dir, de->name, de->name_len, hinfo);
> -			if (err) {
> -				ret = err;
> -				goto out;
> +		if (!(ext4_dirdata_get(de, dir, inline_size, NULL, hinfo) &
> +							EXT4_DIRENT_CFHASH)) {
> +			if (ext4_hash_in_dirent(dir)) {
> +				/* Un-migrated entry: hash at legacy fixed
> +				 * offset. */
> +				hinfo->hash = EXT4_DIRENT_HASH(de);
> +				hinfo->minor_hash = EXT4_DIRENT_MINOR_HASH(de);
> +			} else {
> +				err = ext4fs_dirhash(dir, de->name,
> +						     de->name_len, hinfo);
> +				if (err) {
> +					ret = err;
> +					goto out;
> +				}
>  			}
>  		}
>  		if ((hinfo->hash < start_hash) ||
> diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
> index bd0f4760f1e31..8f1a6c000373e 100644
> --- a/fs/ext4/namei.c
> +++ b/fs/ext4/namei.c
> @@ -1178,22 +1178,34 @@ static int htree_dirblock_to_tree(struct file *dir_file,
>  			/* silently ignore the rest of the block */
>  			break;
>  		}
> -		if (ext4_hash_in_dirent(dir)) {
> -			if (de->name_len && de->inode) {
> -				hinfo->hash = EXT4_DIRENT_HASH(de);
> -				hinfo->minor_hash = EXT4_DIRENT_MINOR_HASH(de);
> -			} else {
> -				hinfo->hash = 0;
> -				hinfo->minor_hash = 0;
> -			}
> +		if (de->name_len && de->inode) {
> +			/* check for saved hash first, or generate it from name */
> +			if (!(ext4_dirdata_get(de, dir, dir->i_sb->s_blocksize,
> +					       NULL, hinfo) &
> +			      EXT4_DIRENT_CFHASH)) {
> +				if (ext4_hash_in_dirent(dir)) {
> +					/* Un-migrated entry: hash is at the
> +					 * legacy fixed offset, not in a CFHASH
> +					 * extension.  Read it directly rather
> +					 * than hashing the encrypted name. */
> +					hinfo->hash = EXT4_DIRENT_HASH(de);
> +					hinfo->minor_hash =
> +						EXT4_DIRENT_MINOR_HASH(de);

[ ... ]

> +unsigned char ext4_dirdata_get(struct ext4_dir_entry_2 *de, struct inode *dir,
> +			       int buf_size,
> +			       struct ext4_dirent_fid *dfid,
> +			       struct dx_hash_info *hinfo)
> +{
> +	unsigned char ret = 0;
> +	unsigned int data_offset = de->name_len + 1;
> +	unsigned int rec_len = ext4_rec_len_from_disk(de->rec_len, buf_size);
> +
> +	/* data_offset is relative to de->name, which itself starts
> +	 * EXT4_BASE_DIR_LEN bytes into the entry -- rec_len is relative to
> +	 * the start of the entry, so add the header size before comparing,
> +	 * or this lets reads run EXT4_BASE_DIR_LEN bytes past the entry. */
> +	if (EXT4_BASE_DIR_LEN + data_offset > rec_len)
> +		return ret;
> +
> +	/* compatibility: hash stored inline after filename (no dirdata) */
> +	if (hinfo && !ext4_has_feature_dirdata(dir->i_sb) &&
> +	    ext4_hash_in_dirent(dir)) {
> +		if (ext4_dirent_rec_len(de->name_len, dir) > rec_len)
> +			return ret;
> +		hinfo->hash = EXT4_DIRENT_HASH(de);
> +		hinfo->minor_hash = EXT4_DIRENT_MINOR_HASH(de);
> +		ret |= EXT4_DIRENT_CFHASH;
> +		return ret;
> +	}

[ ... ]

> @@ -1396,13 +1606,21 @@ static int dx_make_map(struct inode *dir, struct buffer_head *bh,
>  					 ((char *)de) - base))
>  			return -EFSCORRUPTED;
>  		if (de->name_len && de->inode) {
> -			if (ext4_hash_in_dirent(dir))
> -				h.hash = EXT4_DIRENT_HASH(de);
> -			else {
> -				int err = ext4fs_dirhash(dir, de->name,
> -						     de->name_len, &h);
> -				if (err < 0)
> -					return err;
> +			if (!(ext4_dirdata_get(de, dir, dir->i_sb->s_blocksize,
> +					       NULL, &h) &
> +						EXT4_DIRENT_CFHASH)) {
> +				if (ext4_hash_in_dirent(dir)) {
> +					/* Un-migrated entry: hash at legacy
> +					 * fixed offset. */
> +					h.hash = EXT4_DIRENT_HASH(de);
> +					h.minor_hash =
> +						EXT4_DIRENT_MINOR_HASH(de);

[ ... ]

> @@ -1531,10 +1750,29 @@ static bool ext4_match(struct inode *parent,
>  		 * considering the calculated hash.
>  		 */
>  		if (sb_no_casefold_compat_fallback(parent->i_sb) &&
> -		    IS_ENCRYPTED(parent) && fname->cf_name.name &&
> -		    (fname->hinfo.hash != EXT4_DIRENT_HASH(de) ||
> -		     fname->hinfo.minor_hash != EXT4_DIRENT_MINOR_HASH(de)))
> -			return false;
> +		    IS_ENCRYPTED(parent) && fname->cf_name.name) {
> +			__u32 de_hash, de_minor_hash;
> +
> +			if (ext4_has_feature_dirdata(parent->i_sb) &&
> +			    (de->file_type & EXT4_DIRENT_CFHASH)) {
> +				/* Hash is in a CFHASH extension at a variable
> +				 * offset (past any LUFID bytes). Read it via
> +				 * ext4_dirdata_get() to get the correct offset. */
> +				struct dx_hash_info dirent_hinfo = {};
> +
> +				ext4_dirdata_get(de, parent,
> +						 buf_size,
> +						 NULL, &dirent_hinfo);
> +				de_hash = dirent_hinfo.hash;
> +				de_minor_hash = dirent_hinfo.minor_hash;
> +			} else {
> +				de_hash = EXT4_DIRENT_HASH(de);
> +				de_minor_hash = EXT4_DIRENT_MINOR_HASH(de);
> +			}

[Severity: High]
As reported by Sashiko on v13 [1], can the fallback reads of
EXT4_DIRENT_HASH(de) and EXT4_DIRENT_MINOR_HASH(de) read 8 bytes past rec_len
and out of bounds of dir_buf or bh->b_data?

In fs/ext4/namei.c:ext4_dirdata_get(), the legacy compatibility branch checks
that rec_len is large enough to hold the inline hash before reading it:

	if (hinfo && !ext4_has_feature_dirdata(dir->i_sb) &&
	    ext4_hash_in_dirent(dir)) {
		if (ext4_dirent_rec_len(de->name_len, dir) > rec_len)
			return ret;
		hinfo->hash = EXT4_DIRENT_HASH(de);
		hinfo->minor_hash = EXT4_DIRENT_MINOR_HASH(de);
		ret |= EXT4_DIRENT_CFHASH;
		return ret;
	}

When ext4_has_feature_dirdata(dir->i_sb) is enabled, ext4_dirdata_get() also
verifies that any EXT4_DIRENT_LUFID, EXT4_DIRENT_INO64, and
EXT4_DIRENT_CFHASH records fit within rec_len, returning without
EXT4_DIRENT_CFHASH set in the return mask if the flag is absent or if the
entry fails the bounds checks.

However, when ext4_dirdata_get() returns without EXT4_DIRENT_CFHASH set, the
callers in fs/ext4/inline.c:ext4_inlinedir_to_tree(),
fs/ext4/namei.c:htree_dirblock_to_tree(), and fs/ext4/namei.c:dx_make_map()
unconditionally fall back to reading EXT4_DIRENT_HASH(de) and
EXT4_DIRENT_MINOR_HASH(de) when ext4_hash_in_dirent(dir) is true, without
checking that ext4_dirent_rec_len(de->name_len, dir) <= rec_len. Similarly,
the else branches in fs/ext4/dir.c:ext4_readdir() and
fs/ext4/namei.c:ext4_match() (when called from ext4_find_dest_de()) read
EXT4_DIRENT_HASH(de) and EXT4_DIRENT_MINOR_HASH(de) without checking rec_len.

On a valid casefolded and encrypted inline directory in
ext4_inlinedir_to_tree(), the synthesized "." and ".." entries in fake have
fake.rec_len = ext4_dirent_rec_len(fake.name_len, NULL) = 12, which is
smaller than ext4_dirent_rec_len(fake.name_len, dir) = 20. ext4_dirdata_get()
returns 0, and the fallback reads 8 bytes at offset 12..19 past fake.rec_len.
Likewise, on a 1-block casefolded and encrypted directory in
htree_dirblock_to_tree(), the "." entry has rec_len = 12, so after
ext4_dirdata_get() returns 0, the fallback reads 8 bytes from the adjacent
".." entry header.

On a crafted or corrupted casefolded and encrypted directory image,
__ext4_check_dir_entry() sets fake = is_fake_dir_entry(de) (which is true for
"." or ".." or when de->file_type == EXT4_FT_DIR_CSUM) and validates rlen
against ext4_dirent_rec_len(1, NULL) (12 bytes) and
ext4_dir_entry_len(de, size, NULL) (12 bytes for name_len <= 4). This allows
a 12-byte fake entry with de->inode != 0 at offset = size - 12 at the end of
the kmalloc(inline_size) buffer dir_buf or block buffer bh->b_data. When
ext4_dirdata_get() rejects the entry and returns 0, the unchecked fallback in
ext4_inlinedir_to_tree(), htree_dirblock_to_tree(), dx_make_map(),
ext4_readdir(), and ext4_match() reads 8 bytes at (char *)de + 12, past the
end of dir_buf or bh->b_data.

Furthermore, on a dirdata-enabled filesystem where de->file_type &
EXT4_DIRENT_CFHASH is set, if the CFHASH extension fails the bounds check in
ext4_dirdata_get(), htree_dirblock_to_tree(), dx_make_map(), and
ext4_inlinedir_to_tree() still fall back to reading EXT4_DIRENT_HASH(de) at
the legacy fixed offset, misinterpreting LUFID or out-of-bounds bytes as the
casefold hash.

[1] https://lore.kernel.org/r/20261029122413.230409-11-ablagodarenko@thelustrecollective.com

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/20261103134921.610047-1-ablagodarenko@thelustrecollective.com?part=11