Re: [PATCH v14 09/12] ext4: dirdata feature

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 5 potential issue(s) to consider:
- [Critical] ext4: integer underflow and buffer overflow in make_indexed_dir()
- [High] ext4: inconsistent dx_root_info offset in make_indexed_dir()
- [Medium] ext4: malformed dirdata ignored in ext4_dirent_get_data_len()
- [Medium] ext4: silent corruption ignore in ext4_inlinedir_to_tree()
- [Low] ext4: dead dirdata allocation and copy in ext4_htree_store_dirent()
--

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

ext4: dirdata feature

This commit enables the EXT4_FEATURE_INCOMPAT_DIRDATA feature flag and
updates directory entry sizing, insertion, and htree handling 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);
> +	}
> +
> +	/* exta_data contains +1 byte for NULL */
> +	new_fn = kzalloc_flex(*new_fn, name, ent_name->len +
> +			      (extra_data ? extra_data : 1));
>  	if (!new_fn)
>  		return -ENOMEM;
>  	new_fn->hash = hash;
>  	new_fn->minor_hash = minor_hash;
>  	new_fn->inode = le32_to_cpu(dirent->inode);
> -	new_fn->name_len = ent_name->len;
>  	new_fn->file_type = dirent->file_type;
> +	new_fn->name_len = ent_name->len;
>  	memcpy(new_fn->name, ent_name->name, ent_name->len);
> +	if (extra_data)
> +		/* Dirdata bytes lie past the __counted_by(name_len) bound but
> +		 * within the kzalloc_flex allocation; use unsafe_memcpy to
> +		 * avoid a FORTIFY_SOURCE false positive. */
> +		unsafe_memcpy(new_fn->name + ent_name->len,
> +			      dirent->name + dirent->name_len, extra_data,
> +			      /* within kzalloc_flex allocation */);

[Severity: Low]
Is there any consumer of the extra_data bytes copied into new_fn->name?

In fs/ext4/dir.c, struct fname is defined privately with:

	__u8		name_len;
	__u8		file_type;
	char		name[] __counted_by(name_len);

and its only reader is call_filldir(), which only passes fname->name and
fname->name_len to dir_emit():

	while (fname) {
		if (!dir_emit(ctx, fname->name,
				fname->name_len,
				fname->inode,
				get_dtype(sb, fname->file_type))) {
			info->extra_fname = fname;
			return 1;
		}
		fname = fname->next;
	}

In addition, kzalloc_flex() internally runs __set_flex_counter() to store
ent_name->len + extra_data into __u8 new_fn->name_len (which truncates if
the sum exceeds 255) before new_fn->name_len = ent_name->len overwrites it.

Since nothing reads the trailing dirdata bytes from struct fname, can the
extra_data allocation, unsafe_memcpy(), and the buf_size parameter to
ext4_htree_store_dirent() be dropped?

> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index e42e5bcc2f28f..4a7b36e1910ee 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -2369,6 +2369,7 @@ EXT4_FEATURE_INCOMPAT_FUNCS(casefold,		CASEFOLD)
>  					 EXT4_FEATURE_INCOMPAT_FLEX_BG| \
>  					 EXT4_FEATURE_INCOMPAT_EA_INODE| \
>  					 EXT4_FEATURE_INCOMPAT_MMP | \
> +					 EXT4_FEATURE_INCOMPAT_DIRDATA | \
>  					 EXT4_FEATURE_INCOMPAT_INLINE_DATA | \
>  					 EXT4_FEATURE_INCOMPAT_ENCRYPT | \
>  					 EXT4_FEATURE_INCOMPAT_CASEFOLD | \

[ ... ]

> @@ -4278,6 +4307,42 @@ static inline unsigned int ext4_dir_entry_len(struct ext4_dir_entry_2 *de,
>  		   ext4_dirent_rec_len(de->name_len, dir));
>  }

[Severity: Medium]
With EXT4_FEATURE_INCOMPAT_DIRDATA now enabled in EXT4_FEATURE_INCOMPAT_SUPP,
what happens when a directory entry has dirdata flags set in de->file_type
(~EXT4_FT_MASK), but a dirdata header is truncated, zero-length, or exceeds
the entry's current rec_len?

In ext4_dirent_get_data_len():

	if (offset >= rec_len)
		return 0;
	...
	while (extra_data_flags) {
		if (extra_data_flags & 1) {
			if (offset + sizeof(*ddh) > rec_len)
				return 0;
			{
				u8 ddh_len = READ_ONCE(ddh->ddh_length);

				if (ddh_len == 0 || ddh_len > rec_len - offset)
					return 0;

returning 0 on malformed dirdata causes ext4_dir_entry_len() to return the
bare filename record length, so __ext4_check_dir_entry() accepts the entry:

	else if (unlikely(rlen < ext4_dir_entry_len(de, size, fake ? NULL : dir)))
		error_msg = "rec_len is too small for name_len";

This seems to allow two forms of corruption on subsequent directory
modifications:

1. Because ext4_dirent_get_data_len() depends on the entry's current
rec_len, expanding rec_len later (for example, in make_indexed_dir(),
do_split(), or ext4_generic_delete_entry()) can cause a previously
out-of-bounds ddh_length byte in trailing space to become <= rec_len - offset,
increasing ext4_dir_entry_len(de, ...) after the entry was already validated.

2. If an entry has multiple dirdata flags set (such as EXT4_DIRENT_LUFID |
EXT4_DIRENT_CFHASH) where the first record is valid and the second has an
invalid ddh_length, ext4_dirent_get_data_len() returns 0 and drops the first
record's length as well. When ext4_insert_dentry_data(), dx_move_dirents(),
or dx_pack_dirents() then splits or compacts the entry using
ext4_dir_entry_len(), can it shrink de->rec_len to the bare filename length
and overwrite the valid first record while leaving the dirdata flags set in
de->file_type?

> diff --git a/fs/ext4/inline.c b/fs/ext4/inline.c
> index bdf6f45a3de48..81e0ef1c88337 100644
> --- a/fs/ext4/inline.c
> +++ b/fs/ext4/inline.c

[ ... ]

> @@ -1337,14 +1339,24 @@ int ext4_inlinedir_to_tree(struct file *dir_file,
>  			de = &fake;
>  			pos = EXT4_INLINE_DOTDOT_SIZE;
>  		} else {
> +			if (pos + EXT4_BASE_DIR_LEN > inline_size) {
> +				ret = count;
> +				goto out;
> +			}
>  			de = (struct ext4_dir_entry_2 *)(dir_buf + pos);
> -			pos += ext4_rec_len_from_disk(de->rec_len, inline_size);
> +			de_len = ext4_rec_len_from_disk(de->rec_len, inline_size);
> +			if (de_len < EXT4_BASE_DIR_LEN ||
> +			    pos + de_len > (unsigned int)inline_size) {
> +				ret = count;
> +				goto out;
> +			}
>  			if (ext4_check_dir_entry(inode, dir_file, de,
>  					 iloc.bh, dir_buf,
>  					 inline_size, pos)) {
>  				ret = count;
>  				goto out;
>  			}
> +			pos += de_len;
>  		}

[Severity: Medium]
Does checking de_len < EXT4_BASE_DIR_LEN and pos + de_len > inline_size
before ext4_check_dir_entry() suppress filesystem corruption reporting in
ext4_inlinedir_to_tree()?

In __ext4_check_dir_entry(), rlen < ext4_dirent_rec_len(1, ...),
rlen % 4 != 0, and next_offset > size are already checked before calling
ext4_dir_entry_len(), and any failure logs the error via ext4_error_file()
and triggers the filesystem's error policy:

	if (unlikely(rlen < ext4_dirent_rec_len(1, fake ? NULL : dir)))
		error_msg = "rec_len is smaller than minimal";
	else if (unlikely(rlen % 4 != 0))
		error_msg = "rec_len % 4 != 0";
	else if (unlikely(next_offset > size))
		error_msg = "directory entry overrun";
	else if (unlikely(rlen < ext4_dir_entry_len(de, size, fake ? NULL : dir)))
		error_msg = "rec_len is too small for name_len";

Because ext4_rec_len_from_disk(de->rec_len, inline_size) maps rec_len == 0
or 0xffff to inline_size (making pos + de_len > inline_size since pos >= 4)
and shifts unaligned low bits by 16 (making de_len >= 65536 > inline_size),
jumping to out on de_len < EXT4_BASE_DIR_LEN or pos + de_len > inline_size
before calling ext4_check_dir_entry() causes corrupted inline directory
entries (such as rec_len == 0, rec_len == 4, unaligned rec_len, or entry
overruns) to silently return a partial entry count without calling
ext4_check_dir_entry().

> diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
> index 0c91e4da00fbc..4f0465d498877 100644
> --- a/fs/ext4/namei.c
> +++ b/fs/ext4/namei.c
> @@ -402,23 +402,33 @@ 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;
> -		root = (struct dx_root_info *)(((void *)de + 12));
> +		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;
> +		/* 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) +
> +		    sizeof(struct dx_countlimit) > 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]
Can make_indexed_dir() and get_dx_countlimit() disagree on the offset of
dx_root_info when converting a single-block directory to an htree-indexed
directory?

In __ext4_add_entry() -> make_indexed_dir(), dx_get_dx_info(dir, dot_de) is
first called before dotdot_de->rec_len is expanded:

	/* Find room for the hashing info before the directory is changed */
	dx_info = dx_get_dx_info(dir, dot_de);
	...
	memcpy(data2, de, len);
	memset(de, 0, len); /* wipe old data */
	...
	/* Initialize the root; the dot dirents already exist */
	dotdot_de->rec_len =
		ext4_rec_len_to_disk(blocksize - le16_to_cpu(dot_de->rec_len),
				     blocksize);
	...
	entries = (void *)dx_info + dx_info->info_length;
	dx_set_block(entries, 1);
	dx_set_count(entries, 1);
	dx_set_limit(entries, dx_root_limit(dir, dot_de));

If dotdot_de initially has rec_len == 12, a dirdata flag set in
dotdot_de->file_type, and a byte at offset 11 of dotdot_de (ddh_length) > 1,
then during the first dx_get_dx_info(dir, dot_de) call,
ext4_dirent_get_data_len(dotdot_de, 12) sees ddh_len > 12 - 11 and returns 0.
Thus dotdot_rec_len is 12 and dx_info is placed immediately after dotdot_de
(at offset 24), while memset(de, 0, len) only wipes from offset 24 onwards,
leaving byte 11 of dotdot_de untouched.

Once dotdot_de->rec_len is expanded to blocksize - le16_to_cpu(dot_de->rec_len),
calling dx_root_limit(dir, dot_de) -> dx_get_dx_info(dir, dot_de) and
ext4_handle_dirty_dx_node() -> get_dx_countlimit() evaluates
ext4_dir_entry_len(dotdot_de, blocksize, NULL) with the expanded rec_len.
Now ddh_len <= rec_len - 11 is satisfied, so ext4_dir_entry_len() returns a
larger dotdot_rec_len and looks for dx_root_info and dx_countlimit at a
shifted offset instead of where make_indexed_dir() just wrote dx_info and
entries.

[ ... ]

> @@ -2467,7 +2490,8 @@ static int make_indexed_dir(handle_t *handle, struct ext4_filename *fname,
>   */
>  static int __ext4_add_entry(handle_t *handle, struct inode *dir,
>  			  const struct qstr *d_name,
> -			  struct inode *inode)
> +			  struct inode *inode,
> +			  struct ext4_dentry_param *edp)

[Severity: Critical]
Can len underflow in make_indexed_dir() when metadata_csum is enabled and
dot_de / dotdot_de carry dirdata or large rec_len values?

In __ext4_add_entry(), when add_dirent_to_buf() -> ext4_find_dest_de()
searches block 0 of a 1-block directory with metadata_csum enabled
(csum_size = 12), top is set to:

	top = buf + buf_size - reclen;

where buf_size is blocksize - csum_size. If dot_de at offset 0 has a large
rec_len filled by dirdata (for example, blocksize = 1024, buf_size = 1012,
dot_de rec_len = 808 with dirdata nlen = 808) and a new entry has
reclen = 208 (so top = buf + 804), ext4_find_dest_de() checks dot_de on the
first iteration, advances de to buf + 808 > top, and returns -ENOSPC without
ever validating dotdot_de at offset 808.

__ext4_add_entry() then calls make_indexed_dir(), where ext4_check_dx_root()
and dx_get_dx_info() check dotdot_de against blocksize without accounting for
csum_size at the end of the block:

	de = ext4_next_entry(dotdot_de, blocksize);
	if ((char *)de >= (((char *)dot_de) + blocksize)) {
		error_msg = "invalid rec_len for '..'";
		goto corrupted;
	}

If dotdot_de ends in the checksum tail region between blocksize - csum_size
and blocksize (for example, offset 808 + rec_len 212 = 1020 > 1012), both
ext4_check_dx_root() and dx_get_dx_info() succeed, and make_indexed_dir()
computes an unsigned len:

	/* The 0th block becomes the root, move the dirents out */
	len = ((char *)dot_de) + (blocksize - csum_size) - (char *)de;
	...
	memcpy(data2, de, len);
	memset(de, 0, len); /* wipe old data */

Because (char *)de > (char *)dot_de + (blocksize - csum_size), unsigned len
underflows (e.g. 1012 - 1020 = 0xfffffff8) and overflows data2 (bh2->b_data)
in memcpy() and de (bh->b_data) in memset(). Even when (char *)de equals
(char *)dot_de + (blocksize - csum_size), len becomes 0, leaving bh2 with no
directory entries for do_split().

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