Re: [PATCH v13 10/11] ext4: add dirdata set/get helpers

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] ext4: out-of-bounds read in EXT4_DIRENT_HASH() fallback paths
- [High] ext4: LUFID and CFHASH corruption in ext4_setent() on whiteout rollback
--

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

ext4: add dirdata set/get helpers

This commit adds ext4_dirdata_get() and ext4_dirdata_set() helpers to read and
write directory entry extension payloads, and wires them into directory lookup,
readdir, htree indexing, insertion, and rename paths for LUFID and
casefold+encryption hashes.

> 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 d3dda1d65d067..700a0282278b0 100644
> --- a/fs/ext4/inline.c
> +++ b/fs/ext4/inline.c
> @@ -1364,14 +1364,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;
> +				}
>  			}
>  		}

[ ... ]

> diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
> index 433bee757ea4c..5a118320eef9b 100644
> --- a/fs/ext4/namei.c
> +++ b/fs/ext4/namei.c
> @@ -1142,22 +1142,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);
> +				} else {

[ ... ]

> +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)) {
> +		hinfo->hash = EXT4_DIRENT_HASH(de);
> +		hinfo->minor_hash = EXT4_DIRENT_MINOR_HASH(de);
> +		ret |= EXT4_DIRENT_CFHASH;
> +		return ret;
> +	}

[ ... ]

> @@ -1360,13 +1568,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);
> +				} else {

[ ... ]

> @@ -1495,10 +1712,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);
> +			}
> +			if (fname->hinfo.hash != de_hash ||
> +			    fname->hinfo.minor_hash != de_minor_hash)
> +				return false;
> +		}
>  		/*
>  		 * Treat comparison errors as not a match.  The
>  		 * only case where it happens is on a disk
> @@ -1529,11 +1765,18 @@ int ext4_search_dir(struct buffer_head *bh, char *search_buf, int buf_size,
>  	dlimit = search_buf + buf_size;
>  	while ((char *) de < dlimit - EXT4_BASE_DIR_LEN) {
>  		/* this code is executed quadratically often */
> -		/* do minimal checking `by hand' */
> +		/* Decode rec_len before ext4_match() so that ext4_dirdata_get()
> +		 * inside ext4_match() never uses an unvalidated rec_len as a
> +		 * read bound.  A crafted entry with rec_len > buf_size would
> +		 * let ext4_dirdata_get() read past the block buffer. */
> +		de_len = ext4_rec_len_from_disk(de->rec_len,
> +						dir->i_sb->s_blocksize);
> +		if (de_len <= 0 || (char *)de + de_len > dlimit)
> +			return -EFSCORRUPTED;
>  		if (de->name + de->name_len <= dlimit &&
>  		    (!ext4_hash_in_dirent(dir) ||
>  		     (char *)de + ext4_dirent_rec_len(de->name_len, dir) <= dlimit) &&
> -		    ext4_match(dir, fname, de)) {
> +		    ext4_match(dir, fname, de, buf_size)) {

[Severity: High]
Can the EXT4_DIRENT_HASH(de) and EXT4_DIRENT_MINOR_HASH(de) fallback paths
read 8 bytes past the end of bh->b_data or dir_buf?

In fs/ext4/ext4.h:ext4_dirent_rec_len(), when EXT4_FEATURE_INCOMPAT_DIRDATA
is enabled on a casefolded and encrypted directory, sizeof(struct
ext4_dir_entry_hash) is not added to rec_len:

	if (dir && ext4_hash_in_dirent(dir) &&
	    !ext4_has_feature_dirdata(dir->i_sb))
		rec_len += sizeof(struct ext4_dir_entry_hash);
	return (rec_len & ~EXT4_DIR_ROUND);

Because of this, both __ext4_check_dir_entry() and the pre-check in
fs/ext4/namei.c:ext4_search_dir() accept a trailing entry without
EXT4_DIRENT_CFHASH (or with a malformed CFHASH header where
ext4_dirent_get_data_len() and ext4_dirdata_get() return 0) whose rec_len is
((de->name_len + 8 + EXT4_DIR_ROUND) & ~EXT4_DIR_ROUND) right at the end of
the buffer.

However, EXT4_DIRENT_HASHES(de) starts at offset ((8 + de->name_len +
EXT4_DIR_ROUND) & ~EXT4_DIR_ROUND) and reads 8 bytes from that offset. When
fs/ext4/dir.c:ext4_readdir(), fs/ext4/inline.c:ext4_inlinedir_to_tree(),
fs/ext4/namei.c:htree_dirblock_to_tree(), fs/ext4/namei.c:dx_make_map(), or
fs/ext4/namei.c:ext4_match() falls back to EXT4_DIRENT_HASH(de) and
EXT4_DIRENT_MINOR_HASH(de) without checking that those 8 bytes fit within
rec_len, doesn't that read 8 bytes past de + rec_len and past the end of
bh->b_data or the kmalloc(inline_size, GFP_NOFS) buffer dir_buf?

Similarly, in fs/ext4/namei.c:ext4_dirdata_get(), the non-dirdata
compatibility branch only checks:

	if (EXT4_BASE_DIR_LEN + data_offset > rec_len)
		return ret;

which verifies 8 + de->name_len + 1 <= rec_len before reading 8 bytes at
((8 + de->name_len + EXT4_DIR_ROUND) & ~EXT4_DIR_ROUND). And in
fs/ext4/inline.c:ext4_inlinedir_to_tree(), the synthesized "." and ".."
entries use an uninitialized stack variable fake with rec_len == 12 and no
EXT4_DIRENT_CFHASH bit, causing both ext4_dirdata_get() and the fallback to
read 8 bytes of uninitialized kernel stack from fake.name[4..11] and pass
them to ext4_htree_store_dirent().

Should all legacy EXT4_DIRENT_HASH(de) reads verify that
((EXT4_BASE_DIR_LEN + de->name_len + EXT4_DIR_ROUND) & ~EXT4_DIR_ROUND) +
sizeof(struct ext4_dir_entry_hash) <= rec_len first?

[ ... ]

> @@ -3746,6 +3974,43 @@ static int ext4_rename_dir_finish(handle_t *handle, struct ext4_renament *ent,
>  	return 0;
>  }
>  
> +/*
> + * When LUFID is cleared from an entry that also carries CFHASH, the CFHASH
> + * bytes are stranded: LUFID precedes CFHASH in the layout, so removing LUFID
> + * shifts the expected CFHASH offset.  Slide CFHASH into the vacated slot so
> + * the parser finds it at the right position.  If the headers are malformed,
> + * clear CFHASH instead of risking a corrupt read.
> + */
> +static void ext4_setent_compact_exts(struct ext4_renament *ent,
> +				     struct ext4_dirent_data_header *lddh,
> +				     unsigned int lufid_off,
> +				     unsigned int rec_len)
> +{
> +	struct ext4_dirent_data_header *cddh;
> +	unsigned int coff;
> +
> +	if (!(ent->de->file_type & EXT4_DIRENT_CFHASH))
> +		return;
> +	if (lufid_off + sizeof(*lddh) > rec_len ||
> +	    !lddh->ddh_length ||
> +	    lufid_off + lddh->ddh_length > rec_len) {
> +		ent->de->file_type &= ~EXT4_DIRENT_CFHASH;
> +		return;
> +	}
> +
> +	coff = lufid_off + lddh->ddh_length;
> +	cddh = (struct ext4_dirent_data_header *)((char *)ent->de + coff);
> +
> +	if (coff + sizeof(*cddh) > rec_len ||
> +	    !cddh->ddh_length ||
> +	    coff + cddh->ddh_length > rec_len) {
> +		ent->de->file_type &= ~EXT4_DIRENT_CFHASH;
> +		return;
> +	}
> +
> +	memmove(lddh, cddh, cddh->ddh_length);
> +}
> +
>  static int ext4_setent(handle_t *handle, struct ext4_renament *ent,
>  		       unsigned ino, unsigned file_type,
>  		       const struct ext4_dirent_fid *src_fid)
> @@ -3786,13 +4051,25 @@ static int ext4_setent(handle_t *handle, struct ext4_renament *ent,
>  				if (ddh_off + sizeof(*ddh) > rec_len ||
>  				    ddh->ddh_length != copy_len ||
>  				    ddh_off + copy_len > rec_len) {
> -					/* Cannot copy: clear the flag so the
> -					 * slot does not advertise a stale LUFID
> -					 * from the old inode. */
> +					/* Cannot copy in-place (size mismatch).
> +					 * If CFHASH is also present, slide it
> +					 * into the LUFID slot so the parser
> +					 * finds it at the correct offset after
> +					 * the LUFID flag is cleared. */
> +					ext4_setent_compact_exts(ent, ddh,
> +							ddh_off, rec_len);
>  					ent->de->file_type &= ~EXT4_DIRENT_LUFID;
>  				} else {
>  					memcpy(ddh, src_fid, copy_len);
>  				}
> +			} else if (src_fid) {
> +				/* Destination has no LUFID slot; cannot
> +				 * propagate in-place.  Clear the flag from
> +				 * the on-disk entry directly — clearing only
> +				 * the local file_type variable has no effect
> +				 * because the final assignment takes the high
> +				 * bits from ent->de->file_type, not file_type. */
> +				ent->de->file_type &= ~EXT4_DIRENT_LUFID;
>  			} else if (!src_fid) {
>  				/* Sync the LUFID flag with what file_type requests.
>  				 * For normal rename (source has no LUFID) and for
> @@ -3800,7 +4077,24 @@ static int ext4_setent(handle_t *handle, struct ext4_renament *ent,
>  				 * so we clear the stale flag.  For ext4_resetent
>  				 * (error recovery), file_type is the original
>  				 * file_type with LUFID=1, so we restore the flag —
> -				 * the LUFID bytes are still on disk untouched. */
> +				 * the LUFID bytes are still on disk untouched.
> +				 * When clearing LUFID and CFHASH is present, slide
> +				 * CFHASH bytes into the LUFID slot first so the
> +				 * parser finds CFHASH at the expected offset. */
> +				if (!(file_type & EXT4_DIRENT_LUFID) &&
> +				    (ent->de->file_type & EXT4_DIRENT_LUFID)) {
> +					unsigned int loff =
> +						EXT4_BASE_DIR_LEN +
> +						ent->de->name_len + 1;
> +					unsigned int rlen =
> +						ext4_rec_len_from_disk(
> +							ent->de->rec_len,
> +							ent->dir->i_sb->s_blocksize);
> +					ext4_setent_compact_exts(ent,
> +						(struct ext4_dirent_data_header *)
> +						((char *)ent->de + loff),
> +						loff, rlen);
> +				}
>  				ent->de->file_type =
>  					(ent->de->file_type & ~EXT4_DIRENT_LUFID) |
>  					(file_type & EXT4_DIRENT_LUFID);

[Severity: High]
Does calling ext4_setent_compact_exts() here corrupt old.de when a
RENAME_WHITEOUT rename fails and rolls back via ext4_resetent()?

In fs/ext4/namei.c:ext4_rename(), when whiteout is non-NULL and old.de has
both EXT4_DIRENT_LUFID and EXT4_DIRENT_CFHASH set, ext4_rename() first
updates old.de to point to the whiteout inode before adding the new entry:

	if (whiteout) {
		retval = ext4_setent(handle, &old, whiteout->i_ino,
				     EXT4_FT_CHRDEV, NULL);
		if (retval)
			goto end_rename;
	}
	if (!new.de) {
		retval = ext4_add_entry(handle, new.dentry, old.inode,
					old_fid);

Inside that first ext4_setent() call, src_fid is NULL and file_type is
EXT4_FT_CHRDEV (which does not have EXT4_DIRENT_LUFID set), so
ext4_setent_compact_exts() memmoves the 9-byte CFHASH extension over the
on-disk LUFID slot at loff = EXT4_BASE_DIR_LEN + old.de->name_len + 1.

If ext4_add_entry() (or a subsequent step) fails, execution jumps to
end_rename:

end_rename:
	kfree(edp);
	ext4_lufid_snap_release(&old_snap);
	if (whiteout) {
		if (retval) {
			ext4_resetent(handle, &old,
				      old.inode->i_ino, old_file_type);

ext4_resetent() calls ext4_setent(handle, &old, old.inode->i_ino,
old_file_type, NULL). The comment in the !src_fid branch assumes the LUFID
bytes are still on disk untouched, and sets EXT4_DIRENT_LUFID back to 1 in
old.de->file_type. However, the LUFID slot was already overwritten with the
CFHASH bytes during the whiteout step, leaving old.de with both
EXT4_DIRENT_LUFID and EXT4_DIRENT_CFHASH set in file_type, the CFHASH sitting
in the LUFID slot, and stale bytes at the post-LUFID offset where
ext4_dirdata_get() will look for CFHASH.

Also, in ext4_setent_compact_exts(), should coff account for
EXT4_DIRENT_INO64 if that flag is set between EXT4_DIRENT_LUFID and
EXT4_DIRENT_CFHASH, matching the layout in ext4_dirdata_get()?

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