Re: [RFC PATCH v2 18/41] fs: keep UUID and GUID owners off filesystems that can't store them

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 2 potential issue(s) to consider:
- [High] fs: vfs_rename() with RENAME_WHITEOUT skips fsuidgid_has_mapping() when target exists
- [Medium] fs: UUID/GUID owner check bypassed in ocfs2_may_create() and f2fs atomic write
--

commit 3d0ae6a640c537e68d1d11095789815bcc76e4cd
Author: Jan Koester <jan.koester@tuxist.de>

fs: keep UUID and GUID owners off filesystems that can't store them

This commit adds the SB_I_UID_R superblock flag for filesystems that can
store UUID and GUID inode owners, and updates notify_change() and
fsuidgid_has_mapping() to reject UUID and GUID owners on filesystems
without that flag.

> diff --git a/include/linux/fs.h b/include/linux/fs.h
> index deb411e866617..05c4611c87744 100644
> --- a/include/linux/fs.h
> +++ b/include/linux/fs.h
> @@ -1563,14 +1563,35 @@ static inline void inode_fsgid_set(struct inode *inode,
>  	inode->i_gid = mapped_fsgid(idmap, i_user_ns(inode));
>  }
>  
> +/**
> + * sb_can_own_kuid() - check whether a filesystem can store an owner
> + * @sb: the superblock
> + * @kuid: the owner
> + *
> + * Classic ids fit any filesystem; UUIDs and GUIDs only those that set
> + * SB_I_UID_R. Elsewhere i_uid_read() would turn them into -1.
> + *
> + * Return: true if @sb can store @kuid.
> + */
> +static inline bool sb_can_own_kuid(const struct super_block *sb, kuid_t kuid)
> +{
> +	return uid_is_legacy(kuid) || (sb->s_iflags & SB_I_UID_R);
> +}
> +
> +static inline bool sb_can_own_kgid(const struct super_block *sb, kgid_t kgid)
> +{
> +	return gid_is_legacy(kgid) || (sb->s_iflags & SB_I_UID_R);
> +}
> +
>  /**
>   * fsuidgid_has_mapping() - check whether caller's fsuid/fsgid is mapped
>   * @sb: the superblock we want a mapping in
>   * @idmap: idmap of the relevant mount
>   *
>   * Check whether the caller's fsuid and fsgid have a valid mapping in the
> - * s_user_ns of the superblock @sb. If the caller is on an idmapped mount map
> - * the caller's fsuid and fsgid according to the @idmap first.
> + * s_user_ns of the superblock @sb, and whether @sb can store them as owner
> + * of a new inode. If the caller is on an idmapped mount map the caller's
> + * fsuid and fsgid according to the @idmap first.
>   *
>   * Return: true if fsuid and fsgid is mapped, false if not.
>   */
> @@ -1582,10 +1603,10 @@ static inline bool fsuidgid_has_mapping(struct super_block *sb,
>  	kgid_t kgid;
>  
>  	kuid = mapped_fsuid(idmap, fs_userns);
> -	if (!uid_valid(kuid))
> +	if (!uid_valid(kuid) || !sb_can_own_kuid(sb, kuid))
>  		return false;

[Severity: High]
Can a caller with a UUID or GUID fsuid or fsgid bypass fsuidgid_has_mapping()
during a whiteout rename when the target dentry already exists?

In fs/namei.c, sys_renameat2() -> filename_renameat2() -> vfs_rename() only
calls may_create_dentry() (which checks fsuidgid_has_mapping()) when target
is NULL:

fs/namei.c:vfs_rename() {
    ...
	if (!target) {
		error = may_create_dentry(rd->mnt_idmap, new_dir, new_dentry);
	} else {
		new_is_dir = d_is_dir(new_dentry);

		if (!(flags & RENAME_EXCHANGE))
			error = may_delete_dentry(rd->mnt_idmap, new_dir,
						  new_dentry, is_dir);
		else
			error = may_delete_dentry(rd->mnt_idmap, new_dir,
						  new_dentry, new_is_dir);
	}
    ...
}

When target is non-NULL and flags includes RENAME_WHITEOUT, vfs_rename()
only calls may_delete_dentry() on old_dentry and new_dentry, which validates
the existing victim inodes and directory permissions without checking
fsuidgid_has_mapping(old_dir->i_sb, rd->mnt_idmap).

vfs_rename() then invokes old_dir->i_op->rename(..., RENAME_WHITEOUT), which
allocates a new whiteout inode in old_dir and initializes its i_uid and i_gid
from current_fsuid() and current_fsgid() via inode_init_owner(), such as in
fs/ext4/namei.c:

fs/ext4/namei.c:ext4_rename() {
    ...
	if (!(flags & RENAME_WHITEOUT)) {
		handle = ext4_journal_start(old.dir, EXT4_HT_DIR, credits);
		if (IS_ERR(handle)) {
			retval = PTR_ERR(handle);
			goto release_bh;
		}
	} else {
		whiteout = ext4_whiteout_for_rename(idmap, &old, credits, &handle);
		if (IS_ERR(whiteout)) {
			retval = PTR_ERR(whiteout);
			goto release_bh;
		}
	}
    ...
}

The same whiteout creation occurs in ext4_rename() ->
ext4_whiteout_for_rename() -> __ext4_new_inode(), btrfs_rename() ->
new_whiteout_inode(), f2fs_rename() -> f2fs_create_whiteout(), and
xfs_rename() -> xfs_rename_alloc_whiteout().

On filesystems without SB_I_UID_R (such as btrfs, f2fs, xfs, ubifs, and ext4
prior to the later commit "ext4: add the uid_r feature for UUID and GUID
owners"), the whiteout inode's owner is written to disk via i_uid_read() and
i_gid_read() as (uid_t)-1 / (gid_t)-1 (0xffffffff). When read back from disk,
that becomes INVALID_UID / INVALID_GID and leaves the whiteout entry
undeletable because may_delete_dentry() rejects inodes with invalid vfsuid or
vfsgid with -EOVERFLOW.

Additionally, once the later commit "ext4: add the uid_r feature for UUID and
GUID owners" is applied, __ext4_new_inode() -> ext4_mark_inode_dirty() ->
ext4_do_update_inode() -> ext4_fill_raw_inode() -> ext4_inode_set_id_r()
fails with -EOVERFLOW on an ext4 filesystem without the uid_r feature and
calls EXT4_ERROR_INODE() and ext4_std_error(), aborting the journal and
remounting the filesystem read-only (or panicking when mounted with
errors=panic).

Should vfs_rename() also check fsuidgid_has_mapping(old_dir->i_sb,
rd->mnt_idmap) when flags & RENAME_WHITEOUT is set?

>  	kgid = mapped_fsgid(idmap, fs_userns);
> -	if (!gid_valid(kgid))
> +	if (!gid_valid(kgid) || !sb_can_own_kgid(sb, kgid))
>  		return false;

[Severity: Medium]
Are there also filesystem-internal inode creation paths that initialize new
inodes with inode_init_owner() without going through fsuidgid_has_mapping()?

Because sb_can_own_kuid() and sb_can_own_kgid() are only checked in
fsuidgid_has_mapping() (called by may_create_dentry(), may_o_create(), and
vfs_tmpfile()), callers that allocate new inodes through private helpers skip
this check:

1. In fs/ocfs2/refcounttree.c, ioctl(OCFS2_IOC_REFLINK) ->
ocfs2_reflink_ioctl() -> ocfs2_vfs_reflink() checks permissions with
ocfs2_may_create() rather than may_create_dentry():

fs/ocfs2/refcounttree.c:ocfs2_may_create() {
	if (d_really_is_positive(child))
		return -EEXIST;
	if (IS_DEADDIR(dir))
		return -ENOENT;
	return inode_permission(&nop_mnt_idmap, dir, MAY_WRITE | MAY_EXEC);
}

When preserve is false, a caller with a UUID or GUID fsuid or fsgid creates a
new inode via ocfs2_reflink() -> ocfs2_create_inode_in_orphan() ->
ocfs2_get_init_inode() -> inode_init_owner() in fs/ocfs2/namei.c.
__ocfs2_mknod_locked() then writes i_uid_read(inode) and i_gid_read(inode) as
-1 to fe->i_uid and fe->i_gid on disk and calls ocfs2_populate_inode(), which
sets the in-memory inode's i_uid and i_gid to INVALID_UID and INVALID_GID
before ocfs2_mv_orphaned_inode_to_new() links it into the target directory.

2. In fs/f2fs/file.c, f2fs_ioc_start_atomic_write() checks
inode_owner_or_capable() (which does not check fsgid, and permits a UUID or
GUID fsuid if the caller has CAP_FOWNER) and then allocates a COW inode via
f2fs_get_tmpfile() -> __f2fs_tmpfile() -> f2fs_new_inode() ->
inode_init_owner() without calling vfs_tmpfile() or fsuidgid_has_mapping():

fs/f2fs/file.c:f2fs_ioc_start_atomic_write() {
    ...
	if (fi->cow_inode == NULL) {
		struct dentry *dentry = file_dentry(filp);
		struct inode *dir = d_inode(dentry->d_parent);

		ret = f2fs_get_tmpfile(idmap, dir, &fi->cow_inode);
    ...
}

This causes f2fs_update_inode() to write (uid_t)-1 / (gid_t)-1 to the on-disk
COW inode.

Should ocfs2_may_create() and f2fs_ioc_start_atomic_write() (or the common
inode allocation path) also verify fsuidgid_has_mapping()?

>  	return kuid_has_mapping(fs_userns, kuid) &&
>  	       kgid_has_mapping(fs_userns, kgid);

[ ... ]

-- 
Sashiko AI review · https://sashiko.dev/#/patchset/cover.1792552161.git.jan.koester@tuxist.de?part=18