Re: [PATCH v13 08/11] ext4: dirdata feature

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] ext4: missing legacy hash size in ext4_dirent_rec_len() with dirdata
- [High] ext4: out-of-bounds read of htree root dirents in ext4_htree_store_dirent()
- [High] ext4: out-of-bounds read in ext4_dx_csum_verify() via get_dx_countlimit()
- [Medium] ext4: uninitialized stack read of fake dirent in ext4_htree_store_dirent()
--

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

ext4: dirdata feature

This commit enables the EXT4_FEATURE_INCOMPAT_DIRDATA feature flag and updates
directory entry handling across htree, inline directory, and directory entry
insertion paths to account for variable-length dirdata records stored after
the filename.

> diff --git a/fs/ext4/dir.c b/fs/ext4/dir.c
> index 38911ae2bcfc0..15816f2a5f472 100644
> --- a/fs/ext4/dir.c
> +++ b/fs/ext4/dir.c
> @@ -483,27 +483,43 @@ void ext4_htree_free_dir_info(struct dir_private_info *p)
>   * The decrypted filename is passed in via ent_name.  parameter.
>   */
>  int ext4_htree_store_dirent(struct file *dir_file, __u32 hash,
> -			     __u32 minor_hash,
> +			     __u32 minor_hash, int buf_size,
>  			    struct ext4_dir_entry_2 *dirent,
>  			    struct fscrypt_str *ent_name)
>  {
>  	struct rb_node **p, *parent = NULL;
>  	struct fname *fname, *new_fn;
>  	struct dir_private_info *info;
> +	int extra_data = 0;
>  
>  	info = dir_file->private_data;
>  	p = &info->root.rb_node;
>  
>  	/* Create and allocate the fname structure */
> -	new_fn = kzalloc_flex(*new_fn, name, ent_name->len + 1);
> +	if (dirent->file_type & ~EXT4_FT_MASK) {
> +		unsigned int rec_len =
> +			ext4_rec_len_from_disk(dirent->rec_len, buf_size);
> +		extra_data = ext4_dirent_get_data_len(dirent, rec_len);
> +	}

[Severity: Medium]
Can this read uninitialized stack memory from fake.file_type and fake.name[]
when called from ext4_inlinedir_to_tree()?

In fs/ext4/inline.c, ext4_inlinedir_to_tree() declares struct ext4_dir_entry_2
fake on the stack without initializing it, and populates it for the "." and
".." entries:

fs/ext4/inline.c:ext4_inlinedir_to_tree() {
    struct ext4_dir_entry_2 fake;
    ...
        if (pos == 0) {
            fake.inode = cpu_to_le32(inode->i_ino);
            fake.name_len = 1;
            memcpy(fake.name, ".", 2);
            fake.rec_len = ext4_rec_len_to_disk(
                      ext4_dirent_rec_len(fake.name_len, NULL),
                      inline_size);
            ext4_set_de_type(inode->i_sb, &fake, S_IFDIR);
            de = &fake;
            pos = EXT4_INLINE_DOTDOT_OFFSET;
}

However, ext4_set_de_type() only writes de->file_type when the filetype
feature is enabled:

fs/ext4/ext4.h:ext4_set_de_type() {
    if (ext4_has_feature_filetype(sb))
        de->file_type = ext4_type_by_mode[(mode & S_IFMT)>>S_SHIFT];
}

On a filesystem with inline_data and dir_index enabled but without
EXT4_FEATURE_INCOMPAT_FILETYPE, fake.file_type and fake.name[2..3] remain
uninitialized on the stack when ext4_inlinedir_to_tree() passes de = &fake to
ext4_htree_store_dirent().

If any upper bit of the uninitialized fake.file_type is non-zero,
ext4_dirent_get_data_len(&fake, 12) reads ddh->ddh_length from uninitialized
stack bytes fake.name[2..3] and unsafe_memcpy() copies those uninitialized
bytes into new_fn->name.

The later commit in the series ("ext4: add dirdata set/get helpers") gates
this check on ext4_has_feature_dirdata() and also calls ext4_dirdata_get() on
de in ext4_inlinedir_to_tree(), but neither ext4_feature_set_ok() nor
ext4_dirdata_get() requires EXT4_FEATURE_INCOMPAT_FILETYPE when
EXT4_FEATURE_INCOMPAT_DIRDATA is enabled, and fake.file_type is still left
uninitialized before ext4_set_de_type().

Should ext4_inlinedir_to_tree() initialize fake.file_type = EXT4_FT_UNKNOWN
before calling ext4_set_de_type(), matching ext4_insert_dentry_data()?

[ ... ]

> diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
> index e54608950fb34..30bbf6a56bf64 100644
> --- a/fs/ext4/namei.c
> +++ b/fs/ext4/namei.c
> @@ -402,23 +402,32 @@ static struct dx_countlimit *get_dx_countlimit(struct inode *inode,
>  {
>  	struct ext4_dir_entry_2 *de;
>  	struct dx_root_info *root;
> -	int count_offset;
> +	int count_offset, dotdot_rec_len;
>  	int blocksize = EXT4_BLOCK_SIZE(inode->i_sb);
>  	unsigned int rlen = ext4_rec_len_from_disk(dirent->rec_len, blocksize);
>  
> -	if (rlen == blocksize)
> +	if (rlen == blocksize) {
>  		count_offset = sizeof(struct dx_node);
> -	else if (rlen == 12) {
> -		de = (struct ext4_dir_entry_2 *)(((void *)dirent) + 12);
> -		if (ext4_rec_len_from_disk(de->rec_len, blocksize) != blocksize - 12)
> +	} else {
> +		if (rlen < EXT4_BASE_DIR_LEN ||
> +		    rlen + EXT4_BASE_DIR_LEN > blocksize)
> +			return NULL;
> +		de = (struct ext4_dir_entry_2 *)(((char *)dirent) + rlen);
> +		if (ext4_rec_len_from_disk(de->rec_len, blocksize) !=
> +		    (unsigned int)(blocksize - rlen))
>  			return NULL;
> -		root = (struct dx_root_info *)(((void *)de + 12));
> +		/* de->rec_len covers whole dx_root block, calculate actual length.
> +		 * This is the '..' entry, which never carries the casefold+fscrypt
> +		 * hash, so pass NULL for dir regardless of the directory's flags */
> +		dotdot_rec_len = ext4_dir_entry_len(de, blocksize, NULL);
> +		if (rlen + dotdot_rec_len + sizeof(struct dx_root_info) > blocksize)
> +			return NULL;
> +		root = (struct dx_root_info *)(((char *)de + dotdot_rec_len));
>  		if (root->reserved_zero ||
>  		    root->info_length != sizeof(struct dx_root_info))
>  			return NULL;
> -		count_offset = 32;
> -	} else
> -		return NULL;
> +		count_offset = root->info_length + rlen + dotdot_rec_len;
> +	}
>  
>  	if (offset)
>  		*offset = count_offset;

[Severity: High]
Does get_dx_countlimit() allow a 4-byte out-of-bounds read past the end of
bh->b_data in ext4_dx_csum_verify() and ext4_dx_csum_set()?

The bounds check in get_dx_countlimit() checks:

    if (rlen + dotdot_rec_len + sizeof(struct dx_root_info) > blocksize)
        return NULL;

without accounting for sizeof(struct dx_countlimit) (unlike dx_get_dx_info(),
which includes sizeof(struct dx_entry) in its upper-bound check).

On a corrupted or crafted image where rlen + dotdot_rec_len +
sizeof(struct dx_root_info) == blocksize, get_dx_countlimit() sets
count_offset = blocksize and returns (struct dx_countlimit *)(((void *)dirent)
+ blocksize), which points directly past the end of the directory block
buffer.

Both ext4_dx_csum_verify() and ext4_dx_csum_set() immediately dereference
c->limit and c->count before validating count_offset against blocksize:

fs/ext4/namei.c:ext4_dx_csum_verify() {
    c = get_dx_countlimit(inode, dirent, &count_offset);
    if (!c) {
        EXT4_ERROR_INODE(inode, "dir seems corrupt?  Run e2fsck -D.");
        return 0;
    }
    limit = le16_to_cpu(c->limit);
    count = le16_to_cpu(c->count);
    if (count_offset + (limit * sizeof(struct dx_entry)) >
        EXT4_BLOCK_SIZE(inode->i_sb) - sizeof(struct ext4_dir_entry_tail))
        ...
}

Should the check in get_dx_countlimit() include sizeof(struct dx_countlimit)
so that the returned struct dx_countlimit pointer is guaranteed to lie within
the block?

[ ... ]

> @@ -1252,6 +1264,7 @@ int ext4_htree_fill_tree(struct file *dir_file, __u32 start_hash,
>  		tmp_str.name = de->name;
>  		tmp_str.len = de->name_len;
>  		err = ext4_htree_store_dirent(dir_file, 0, 0,
> +					      dir->i_sb->s_blocksize,
>  					      de, &tmp_str);
>  		if (err != 0)
>  			goto errout;
> @@ -1263,6 +1276,7 @@ int ext4_htree_fill_tree(struct file *dir_file, __u32 start_hash,
>  		tmp_str.name = de->name;
>  		tmp_str.len = de->name_len;
>  		err = ext4_htree_store_dirent(dir_file, 2, 0,
> +					      dir->i_sb->s_blocksize,
>  					      de, &tmp_str);
>  		if (err != 0)
>  			goto errout;

[Severity: High]
Could ext4_htree_fill_tree() and ext4_htree_store_dirent() read past the end
of frames[0].bh->b_data when storing the "." and ".." entries from an htree
root block?

In dx_probe() -> dx_get_dx_info(), the code steps from "." to ".." using
ext4_dir_entry_len():

fs/ext4/namei.c:dx_get_dx_info() {
    if (ext4_rec_len_from_disk(((struct ext4_dir_entry_2 *)de_buf)->rec_len,
                   blocksize) < EXT4_BASE_DIR_LEN)
        return ERR_PTR(-EFSCORRUPTED);
    de_buf += ext4_dir_entry_len(de_buf, blocksize, NULL);
}

By contrast, get_dx_countlimit() and ext4_htree_fill_tree() advance from "."
to ".." using dot_de->rec_len (via ext4_next_entry(de, dir->i_sb->s_blocksize))
without verifying that dot_de->rec_len matches ext4_dir_entry_len(dot_de,
blocksize, NULL).

Even with the additional rlen checks added in the later commit ("ext4: Add
EXT4_IOC_SET_LUFID ioctl for setting LUFID on directory entries"),
dx_get_dx_info() permits rlen <= blocksize on "." and validates the ".." entry
and dx_root_info at offset ext4_dir_entry_len(dot_de) (for example, offset 12)
rather than at offset rlen.

As a result, on a crafted image where dot_de has ext4_dir_entry_len(dot_de) ==
12 (with valid ".." and dx_root_info at offsets 12 and 24) but dot_de->rec_len
decodes to rlen == blocksize (or blocksize - 4), dx_probe() succeeds and
ext4_htree_fill_tree() then advances de = ext4_next_entry(de, s_blocksize) to
frames[0].bh->b_data + rlen, reading de->name_len, de->inode, de->file_type,
and de->rec_len out of bounds.

Similarly, if dot_de->rec_len decodes to an offset near the end of the block
(such as 4000), ext4_htree_fill_tree() passes "." and ".." to
ext4_htree_store_dirent() without calling ext4_check_dir_entry(). In
ext4_htree_store_dirent(), rec_len = ext4_rec_len_from_disk(dirent->rec_len,
buf_size) is passed directly to ext4_dirent_get_data_len(dirent, rec_len)
without checking that (char *)dirent + rec_len or dirent->name +
dirent->name_len fits within frames[0].bh->b_data + s_blocksize, allowing
memcpy() and unsafe_memcpy() to read past the end of the block buffer.

[ ... ]

> @@ -2118,13 +2136,10 @@ int ext4_find_dest_de(struct inode *dir, struct buffer_head *bh,
>  	return 0;
>  }
>  
> -void ext4_insert_dentry(struct inode *dir,
> -			struct inode *inode,
> -			struct ext4_dir_entry_2 *de,
> -			int buf_size,
> -			struct ext4_filename *fname)
> +void ext4_insert_dentry_data(struct inode *dir, struct inode *inode,
> +			     struct ext4_dir_entry_2 *de, int buf_size,
> +			     struct ext4_filename *fname, void *data)
>  {
> -
>  	int nlen, rlen;
>  
>  	nlen = ext4_dir_entry_len(de, buf_size, dir);
> @@ -2141,6 +2156,10 @@ void ext4_insert_dentry(struct inode *dir,
>  	ext4_set_de_type(inode->i_sb, de, inode->i_mode);
>  	de->name_len = fname_len(fname);
>  	memcpy(de->name, fname_name(fname), fname_len(fname));
> +	/* 'data' (the LUFID payload) and the CFHASH are written by
> +	 * ext4_dirdata_set(), which is introduced in the next patch that adds
> +	 * the dirdata set/get helpers.  Until then, only the casefold hash is
> +	 * stored at the legacy fixed offset for encrypted+casefolded dirs. */
>  	if (ext4_hash_in_dirent(dir)) {
>  		struct dx_hash_info *hinfo = &fname->hinfo;
>  

[Severity: Critical]
Does enabling EXT4_FEATURE_INCOMPAT_DIRDATA in EXT4_FEATURE_INCOMPAT_SUPP
while ext4_dirent_rec_len() omits sizeof(struct ext4_dir_entry_hash) cause
hash corruption and 8-byte out-of-bounds reads in encrypted, casefolded
directories when EXT4_DIRENT_CFHASH is not set on an entry?

In fs/ext4/ext4.h, ext4_dirent_rec_len() only reserves
sizeof(struct ext4_dir_entry_hash) when dirdata is not enabled on the
superblock:

fs/ext4/ext4.h:ext4_dirent_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);
}

This commit adds EXT4_FEATURE_INCOMPAT_DIRDATA to EXT4_FEATURE_INCOMPAT_SUPP,
making ext4_has_feature_dirdata(dir->i_sb) true on mounted dirdata
filesystems.

At this commit, ext4_insert_dentry_data() still writes the casefold hash at
EXT4_DIRENT_HASHES(de) without setting EXT4_DIRENT_CFHASH in de->file_type.
Because ext4_dirent_get_data_len() returns 0 and ext4_dirent_rec_len() omits
sizeof(struct ext4_dir_entry_hash), ext4_dir_entry_len(de, buf_size, dir)
returns a length 8 bytes too small, and splitting the entry at nlen =
ext4_dir_entry_len() overwrites the 8-byte hash.

Even after the follow-up commit ("ext4: add dirdata set/get helpers") adds
ext4_dirdata_set() and ext4_dirdata_get(), all hash readers
(ext4_inlinedir_to_tree(), ext4_search_dir() -> ext4_match(), ext4_readdir(),
htree_dirblock_to_tree(), and dx_make_map()) fall back to reading
EXT4_DIRENT_HASH(de) and EXT4_DIRENT_MINOR_HASH(de) at the legacy fixed offset
whenever EXT4_DIRENT_CFHASH is not set in de->file_type (or when
ext4_dirdata_get() returns an error).

However, __ext4_check_dir_entry() and ext4_search_dir() validate entry bounds
using ext4_dir_entry_len() and ext4_dirent_rec_len():

fs/ext4/dir.c:__ext4_check_dir_entry() {
    else if (unlikely(rlen < ext4_dir_entry_len(de, size, fake ? NULL : dir)))
        error_msg = "rec_len is too small for name_len";
}

fs/ext4/namei.c:ext4_search_dir() {
    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)) {
}

When ext4_has_feature_dirdata(dir->i_sb) is true and an entry in an
encrypted, casefolded directory does not have a valid EXT4_DIRENT_CFHASH
extension (for example, an un-migrated entry or a crafted entry with rec_len =
12 and name_len = 4 at the end of dir_buf in ext4_inlinedir_to_tree() or at
dlimit - 12 in ext4_search_dir()), bounds validation passes without requiring
the 8-byte hash, and the fallback to EXT4_DIRENT_HASHES(de) reads 8 bytes past
the end of the kmalloc(inline_size) buffer or directory block buffer.
Likewise, ext4_dir_entry_len() omits the 8-byte legacy hash for un-migrated
entries lacking EXT4_DIRENT_CFHASH, causing ext4_insert_dentry_data(),
dx_move_dirents(), and dx_pack_dirents() to overwrite their hashes.

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