Re: [PATCH v14 12/12] ext4: Add EXT4_IOC_SET_LUFID ioctl for setting LUFID on directory entries

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 4 potential issue(s) to consider:
- [Critical] ext4: rollback in ext4_dirdata_set_lufid() can fail and lose directory entry
- [High] ext4: premature fscrypt_has_encryption_key() check in ext4_dirdata_set_lufid()
- [Medium] ext4: missing validation of looked-up inode in ext4_dirdata_set_lufid()
- [Medium] ext4: check_sticky() positive return value leaked in ext4_dirdata_set_lufid()
--

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

ext4: Add EXT4_IOC_SET_LUFID ioctl for setting LUFID on directory entries

This commit introduces the EXT4_IOC_SET_LUFID ioctl and
ext4_dirdata_set_lufid() to attach LUFID dirdata to existing directory
entries by looking up the entry, deleting it, and re-creating it with the
LUFID payload.

> diff --git a/fs/ext4/namei.c b/fs/ext4/namei.c
> index 8f1a6c000373e..68c5caff4707e 100644
> --- a/fs/ext4/namei.c
> +++ b/fs/ext4/namei.c

[ ... ]

> @@ -4825,6 +4858,245 @@ static int ext4_rename2(struct mnt_idmap *idmap,
>  	return ext4_rename(idmap, old_dir, old_dentry, new_dir, new_dentry, flags);
>  }
>  
> +/*
> + * ext4_dirdata_set_lufid() - Set LUFID data on an existing directory entry
> + * @dir:        parent directory inode
> + * @filename:   name of the file in the directory
> + * @namelen:    length of filename
> + * @edp:        pointer to initialized dentry param with LUFID data
> + *
> + * This function finds an existing directory entry, deletes it, and re-creates it
> + * with LUFID data attached. Used by the EXT4_IOC_SET_LUFID ioctl.
> + *
> + * Returns 0 on success, negative error code on failure.
> + */
> +int ext4_dirdata_set_lufid(struct mnt_idmap *idmap, struct inode *dir,
> +			   const char *filename, int namelen,
> +			   struct ext4_dentry_param *edp)
> +{
> +	struct super_block *sb = dir->i_sb;
> +	/* zero-init: safe to free on any path */
> +	struct ext4_filename new_fname = {}, old_fname = {};
> +	struct ext4_dir_entry_2 *de = NULL;
> +	struct buffer_head *bh = NULL;
> +	struct inode *inode = NULL;
> +	handle_t *handle = NULL;
> +	struct ext4_lufid_snap old_snap = {};
> +	struct qstr d_name = QSTR_INIT(filename, namelen);
> +	/* the name as stored on disk, for casefolded directories */
> +	struct fscrypt_str stored = FSTR_INIT(NULL, 0);
> +	struct qstr stored_name;
> +	const struct qstr *add_name = &d_name;
> +	__u32 ino;
> +	int err, err2;
> +
> +	if (!ext4_has_feature_dirdata(sb))
> +		return -EOPNOTSUPP;
> +
> +	if (namelen > EXT4_NAME_LEN)
> +		return -ENAMETOOLONG;
> +	if (namelen != strnlen(filename, namelen + 1))
> +		return -EINVAL;
> +
> +	/* the entry is re-created from its plaintext name */
> +	if (IS_ENCRYPTED(dir) && !fscrypt_has_encryption_key(dir))
> +		return -ENOKEY;

[Severity: High]
Won't this fscrypt_has_encryption_key(dir) check fail with -ENOKEY on an
encrypted directory even when the key is present in the keyring?

fscrypt_has_encryption_key() only checks whether dir->i_crypt_info has
already been populated:

include/linux/fscrypt.h:fscrypt_has_encryption_key() {
	return fscrypt_get_inode_info(inode) != NULL;
}

Opening a directory via ext4_dir_open() does not set up i_crypt_info, nor
does looking up a top-level encrypted directory in an unencrypted parent or
adding a key via FS_IOC_ADD_ENCRYPTION_KEY on an already-open directory fd.

Should ext4_dirdata_set_lufid() call fscrypt_prepare_readdir(dir) (or
fscrypt_get_encryption_info(dir)) before checking
fscrypt_has_encryption_key(dir)?

> +
> +	err = dquot_initialize(dir);
> +	if (err)
> +		return err;
> +
> +	/*
> +	 * The lookup and the change must both be under the directory lock.
> +	 * The entry is deleted and re-added in the same directory, so the
> +	 * target inode itself is not changed and is not locked.
> +	 */
> +	inode_lock_nested(dir, I_MUTEX_PARENT);
> +
> +	/*
> +	 * Look up the entry first to get the inode for the permission
> +	 * checks, without holding a journal handle.
> +	 */
> +	bh = ext4_find_entry(dir, &d_name, &de, NULL);
> +	if (IS_ERR(bh)) {
> +		err = PTR_ERR(bh);
> +		bh = NULL;
> +		goto out_unlock_dir;
> +	}
> +	if (!bh) {
> +		err = -ENOENT;
> +		goto out_unlock_dir;
> +	}
> +	ino = le32_to_cpu(de->inode);
> +	brelse(bh);
> +	bh = NULL;
> +	de = NULL;
> +
> +	inode = ext4_iget(sb, ino, EXT4_IGET_NORMAL);
> +	if (IS_ERR(inode)) {
> +		err = PTR_ERR(inode);
> +		inode = NULL;
> +		goto out_unlock_dir;
> +	}

[Severity: Medium]
Should this lookup validate ino and the returned inode the same way
ext4_lookup() does?

In ext4_lookup(), before and after calling ext4_iget(), it checks for
invalid inode numbers, entries pointing back to the parent directory itself,
and deleted inodes returning -ESTALE:

fs/ext4/namei.c:ext4_lookup() {
		if (!ext4_valid_inum(dir->i_sb, ino)) {
			EXT4_ERROR_INODE(dir, "bad inode number: %u", ino);
			return ERR_PTR(-EFSCORRUPTED);
		}
		if (unlikely(ino == dir->i_ino)) {
			EXT4_ERROR_INODE(dir, "'%pd' linked to parent dir",
					 dentry);
			return ERR_PTR(-EFSCORRUPTED);
		}
		inode = ext4_iget(dir->i_sb, ino, EXT4_IGET_NORMAL);
		if (inode == ERR_PTR(-ESTALE)) {
			EXT4_ERROR_INODE(dir,
					 "deleted inode referenced: %u",
					 ino);
			return ERR_PTR(-EFSCORRUPTED);
		}
}

Also, if a corrupted directory entry references an unlinked inode that is
still open in memory, ext4_iget() will return the cached inode with
inode->i_nlink == 0. Should ext4_dirdata_set_lufid() check for these cases
and report -EFSCORRUPTED instead of deleting and re-creating a corrupted
entry?

> +
> +	/* Enforce sticky-bit restriction: on a sticky directory only the
> +	 * directory owner, the file owner, or a privileged process may
> +	 * modify entries.  inode_permission(MAY_WRITE) alone does not cover
> +	 * this, so check it explicitly here as may_delete() does. */
> +	err = check_sticky(idmap, dir, inode);
> +	if (err)
> +		goto out_iput;

[Severity: Medium]
Does this leak a positive return value (+1) to userspace when sticky-bit
permission is denied?

check_sticky() calls __check_sticky(), which returns 0 when allowed and 1
(not -EPERM) when denied:

fs/namei.c:__check_sticky() {
	if (vfsuid_eq_kuid(i_uid_into_vfsuid(idmap, inode), fsuid))
		return 0;
	if (vfsuid_eq_kuid(i_uid_into_vfsuid(idmap, dir), fsuid))
		return 0;
	return !capable_wrt_inode_uidgid(idmap, inode, CAP_FOWNER);
}

Assigning that return value to err and jumping to out_iput causes
ext4_ioctl_set_lufid() to return 1 to userspace instead of -EPERM. Should
this set err = -EPERM when check_sticky(idmap, dir, inode) is non-zero?

[ ... ]

> +	err = ext4_delete_entry(handle, dir, de, bh);
> +	if (err)
> +		goto out_brelse;
> +
> +	brelse(bh);
> +	bh = NULL;
> +
> +	err = ext4_add_fname(handle, dir, &new_fname, inode);
> +	if (err) {
> +		/*
> +		 * The old entry fits in the space it was deleted from, so
> +		 * this needs no new block.
> +		 */
> +		err2 = ext4_add_fname(handle, dir, &old_fname, inode);
> +		if (err2)
> +			EXT4_ERROR_INODE(dir,
> +				"Failed to set LUFID on '%.*s' (err=%d) and failed to restore the original directory entry (err=%d); inode %llu may be orphaned",
> +				namelen, filename, err, err2,
> +				(unsigned long long)inode->i_ino);
> +		goto out_journal;
> +	}

[Severity: Critical]
Can the rollback call to ext4_add_fname(handle, dir, &old_fname, inode) fail
in an HTree-indexed directory and permanently lose the directory entry?

When looking up the existing entry, ext4_find_entry() ->
ext4_dx_find_entry() walks continuation leaf blocks via
ext4_htree_next_block() when a hash collision chain spans multiple blocks
(and __ext4_find_entry() can also fall back to a linear scan across all
directory blocks on casefolded directories or on ERR_BAD_DX_DIR).

However, when re-adding the entry, ext4_add_fname() -> ext4_dx_add_entry()
only probes the single primary leaf block returned by dx_probe() and never
walks continuation blocks:

fs/ext4/namei.c:ext4_dx_add_entry() {
	frame = dx_probe(fname, dir, NULL, frames);
	if (IS_ERR(frame))
		return PTR_ERR(frame);
	entries = frame->entries;
	at = frame->at;
	bh = ext4_read_dirblock(dir, dx_get_block(frame->at), DIRENT_HTREE);
	...
	err = add_dirent_to_buf(handle, fname, dir, inode, NULL, bh);
	if (err != -ENOSPC)
		goto cleanup;
	...
	de = do_split(handle, dir, &bh, frame, &fname->hinfo);
}

If the old entry resided in a continuation block (or was found via linear
fallback), ext4_delete_entry() frees space in that continuation block, while
the primary block returned by dx_probe() remains full. If splitting that
primary block in ext4_add_fname(&new_fname) then fails (for example due to
-ENOSPC, -EDQUOT, or reaching the maximum HTree level), the rollback call
ext4_add_fname(&old_fname) will probe the same full primary block, fail to
split it for the same reason, and commit the transaction with the entry
deleted and inode's link count leaked.

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