Re: [PATCH v13 07/11] ext4: rename ext4_dir_rec_len() and clarify dirdata usage

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Critical] ext4: mutable ext4_dir_entry_len() causes OOB write in ext4_insert_dentry()
--

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

ext4: rename ext4_dir_rec_len() and clarify dirdata usage

This commit renames ext4_dir_rec_len() to ext4_dirent_rec_len(), widens its
name_len parameter from __u8 to unsigned int, and updates several callers to
use ext4_dir_entry_len() or pass an explicit dirdata length to
ext4_find_dest_de().

> diff --git a/fs/ext4/dir.c b/fs/ext4/dir.c
> index 8d7b81e6948e7..38911ae2bcfc0 100644
> --- a/fs/ext4/dir.c
> +++ b/fs/ext4/dir.c
> @@ -89,16 +89,15 @@ int __ext4_check_dir_entry(const char *function, unsigned int line,
>  	bool fake = is_fake_dir_entry(de);
>  	bool has_csum = ext4_has_feature_metadata_csum(dir->i_sb);
>  
> -	if (unlikely(rlen < ext4_dir_rec_len(1, fake ? NULL : dir)))
> +	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(rlen < ext4_dir_rec_len(de->name_len,
> -							fake ? NULL : dir)))
> -		error_msg = "rec_len is too small for name_len";
>  	else if (unlikely(next_offset > size))
>  		error_msg = "directory entry overrun";
> -	else if (unlikely(next_offset > size - ext4_dir_rec_len(1,
> +	else if (unlikely(rlen < ext4_dir_entry_len(de, size, fake ? NULL : dir)))
> +		error_msg = "rec_len is too small for name_len";
> +	else if (unlikely(next_offset > size - ext4_dirent_rec_len(1,
>  						  has_csum ? NULL : dir) &&
>  			  next_offset != size))
>  		error_msg = "directory entry too close to block end";

[ ... ]

> diff --git a/fs/ext4/ext4.h b/fs/ext4/ext4.h
> index 2d6d3be4ef7df..839274c4d7fcd 100644
> --- a/fs/ext4/ext4.h
> +++ b/fs/ext4/ext4.h
> @@ -2649,11 +2649,16 @@ struct ext4_dirent_hash {
>   * casefolded and encrypted need to store the hash as well, so we add room for
>   * ext4_extended_dir_entry_2. For all entries related to '.' or '..' you should
>   * pass NULL for dir, as those entries do not use the extra fields.
> + *
> + * For directories with the dirdata feature, extra data may follow the filename.
> + * Use ext4_dir_entry_len() to compute the length of a directory entry
> + * including any dirdata, or ext4_dirent_rec_len() directly when the total
> + * name_len (including dirdata length) is already known.
>   */
> -static inline unsigned int ext4_dir_rec_len(__u8 name_len,
> +static inline unsigned int ext4_dirent_rec_len(unsigned int name_len,
>  						const struct inode *dir)
>  {
> -	int rec_len = (name_len + 8 + EXT4_DIR_ROUND);
> +	unsigned int rec_len = (name_len + 8 + EXT4_DIR_ROUND);
>  
>  	/*
>  	 * Without dirdata, the casefold+fscrypt hash lives at a fixed position

[ ... ]

> @@ -4266,7 +4272,7 @@ static inline unsigned int ext4_dir_entry_len(struct ext4_dir_entry_2 *de,
>  	unsigned int rec_len = ext4_rec_len_from_disk(de->rec_len, blocksize);
>  	unsigned int dirdata = ext4_dirent_get_data_len(de, rec_len);
>  
> -	return ext4_dir_rec_len(de->name_len + dirdata, dir);
> +	return ext4_dirent_rec_len(de->name_len + dirdata, dir);
>  }

[ ... ]

> diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
> index b3acab0689c93..e54608950fb34 100644
> --- a/fs/ext4/namei.c
> +++ b/fs/ext4/namei.c

[ ... ]

> @@ -1902,7 +1902,7 @@ dx_move_dirents(struct inode *dir, char *from, char *to,
>  	while (count--) {
>  		struct ext4_dir_entry_2 *de = (struct ext4_dir_entry_2 *)
>  						(from + (map->offs<<2));
> -		rec_len = ext4_dir_rec_len(de->name_len, dir);
> +		rec_len = ext4_dir_entry_len(de, blocksize, dir);
>  
>  		memcpy (to, de, rec_len);
>  		((struct ext4_dir_entry_2 *) to)->rec_len =
> @@ -1935,7 +1935,7 @@ static struct ext4_dir_entry_2 *dx_pack_dirents(struct inode *dir, char *base,
>  	while ((char*)de < base + blocksize) {
>  		next = ext4_next_entry(de, blocksize);
>  		if (de->inode && de->name_len) {
> -			rec_len = ext4_dir_rec_len(de->name_len, dir);
> +			rec_len = ext4_dir_entry_len(de, blocksize, dir);
>  			if (de > to)
>  				memmove(to, de, rec_len);
>  			to->rec_len = ext4_rec_len_to_disk(rec_len, blocksize);

[ ... ]

> @@ -2126,7 +2127,7 @@ void ext4_insert_dentry(struct inode *dir,
>  
>  	int nlen, rlen;
>  
> -	nlen = ext4_dir_rec_len(de->name_len, dir);
> +	nlen = ext4_dir_entry_len(de, buf_size, dir);
>  	rlen = ext4_rec_len_from_disk(de->rec_len, buf_size);
>  	if (de->inode) {
>  		struct ext4_dir_entry_2 *de1 =

[Severity: Critical]
Can switching to ext4_dir_entry_len() here and widening ext4_dirent_rec_len()
cause an out-of-bounds write past bh->b_data or trigger the BUG_ON() in
ext4_rec_len_to_disk() when splitting a directory block via
do_split() -> add_dirent_to_buf() -> ext4_insert_dentry()?

In fs/ext4/ext4.h, ext4_dir_entry_len() computes the entry length using the
entry's current de->rec_len via ext4_dirent_get_data_len():

fs/ext4/ext4.h:ext4_dirent_get_data_len() {
    ...
			{
				u8 ddh_len = READ_ONCE(ddh->ddh_length);

				if (ddh_len == 0 || ddh_len > rec_len - offset)
					return 0;
				dlen += ddh_len + (dlen == 0);
    ...
}

Because ext4_dirent_get_data_len() returns 0 whenever ddh_len exceeds
rec_len - offset (and does not check ext4_has_feature_dirdata()),
__ext4_check_dir_entry() in fs/ext4/dir.c will not reject an entry whose
on-disk rec_len is smaller than its dirdata length.

During a directory leaf split in fs/ext4/namei.c:do_split():

fs/ext4/namei.c:do_split() {
    ...
	de2 = dx_move_dirents(dir, data1, data2, map + split, count - split,
			      blocksize);
	de = dx_pack_dirents(dir, data1, blocksize);
	de->rec_len = ext4_rec_len_to_disk(data1 + (blocksize - csum_size) -
					   (char *) de,
					   blocksize);
	de2->rec_len = ext4_rec_len_to_disk(data2 + (blocksize - csum_size) -
					    (char *) de2,
					    blocksize);
    ...
}

If the final entry de in the target split half has a small rec_len (for
example, rec_len = 12, name_len = 1, file_type = 0x11) with a larger
ddh_length byte stored at offset 10 (still inside the 12-byte entry),
ext4_dirent_get_data_len(de, 12) returns 0 during __ext4_check_dir_entry(),
dx_move_dirents(), and dx_pack_dirents(), copying all 12 bytes.

Then do_split() expands de->rec_len to cover the remainder of the block and
passes de to add_dirent_to_buf(), which skips ext4_find_dest_de() when de is
non-NULL and calls ext4_insert_dentry() directly:

fs/ext4/namei.c:ext4_insert_dentry() {
	int nlen, rlen;

	nlen = ext4_dir_entry_len(de, buf_size, dir);
	rlen = ext4_rec_len_from_disk(de->rec_len, buf_size);
	if (de->inode) {
		struct ext4_dir_entry_2 *de1 =
			(struct ext4_dir_entry_2 *)((char *)de + nlen);
		de1->rec_len = ext4_rec_len_to_disk(rlen - nlen, buf_size);
		de->rec_len = ext4_rec_len_to_disk(nlen, buf_size);
		de = de1;
	}
    ...
	memcpy(de->name, fname_name(fname), fname_len(fname));
}

When ext4_insert_dentry() recomputes nlen = ext4_dir_entry_len(de, buf_size,
dir) with the expanded de->rec_len, ddh_len <= rec_len - offset can now
succeed, growing nlen up to rlen (or rlen + 8 if ext4_hash_in_dirent(dir) &&
!ext4_has_feature_dirdata(dir->i_sb)). In addition, because
ext4_dirent_rec_len() now takes unsigned int name_len, an entry with multiple
valid dirdata fields can be up to 1284 bytes, which on 1024-byte or 2048-byte
block filesystems can also leave rlen - nlen in do_split() smaller than the
new entry being inserted.

Since ext4_insert_dentry() does not check that rlen - nlen is large enough to
hold the new entry, wouldn't nlen > rlen underflow rlen - nlen and hit BUG_ON
in ext4_rec_len_to_disk(), or nlen <= rlen advance de1 to the end of the
block buffer so that memcpy(de->name, ...) writes past the end of bh->b_data?

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