Re: [RFC PATCH v2 33/41] posix_acl: support UUID and GUID entries

[sashiko-bot@kernel.org]

Thank you for your contribution! Sashiko AI review found 1 potential issue(s) to consider:
- [Medium] posix_acl: version 3 ACLs accepted by posix_acl_from_xattr() corrupt on writeback
--

commit 2e207d2596cd01ebad6cf100258b3d1483a2d6a6
Author: Jan Koester <jan.koester@tuxist.de>

posix_acl: support UUID and GUID entries

This commit adds version 3 of the POSIX ACL xattr format to support 16-byte
UUID and GUID entries in ACL_USER and ACL_GROUP tags. It updates setxattr() to
accept both version 2 and version 3, getxattr() to emit version 3 when needed,
and set_posix_acl() to reject UUID and GUID entries with -EOVERFLOW on
filesystems without SB_I_UID_R.

> diff --git a/fs/posix_acl.c b/fs/posix_acl.c
> index 18b302f941743..bcb0663fe2c83 100644
> --- a/fs/posix_acl.c
> +++ b/fs/posix_acl.c

[ ... ]

> @@ -795,6 +843,13 @@ struct posix_acl *posix_acl_from_xattr(struct user_namespace *userns,
>  		return ERR_PTR(-ENOMEM);
>  	acl_e = acl->a_entries;
>  
> +	if (header->a_version == cpu_to_le32(POSIX_ACL_XATTR_VERSION_R)) {
> +		if (posix_acl_from_xattr_r(userns, (const void *)(header + 1),
> +					   acl))
> +			goto fail;
> +		return acl;
> +	}

[Severity: Medium]
Can accepting POSIX_ACL_XATTR_VERSION_R unconditionally in
posix_acl_from_xattr() corrupt ACLs on writeback for filesystems that do not
set SB_I_UID_R?

posix_acl_from_xattr() is used both by the VFS setxattr() path in do_set_acl()
and by filesystems that store POSIX ACLs in the uapi xattr format (such as
btrfs, fuse, 9p, ceph, erofs, gfs2, jfs, ntfs, ntfs3, and orangefs) in their
->get_acl() and ->get_inode_acl() callbacks.

If a backing store returns a version 3 xattr containing UUID or GUID entries
(for example, a fuse/virtiofs or 9p server exporting a host directory where
getxattr() returns version 3, or a filesystem image with a version 3 xattr),
posix_acl_from_xattr() will load and cache a struct posix_acl with non-legacy
kuid_t or kgid_t entries on a filesystem without SB_I_UID_R.

When chmod() (via posix_acl_chmod()) or file creation under a directory with a
default ACL (via posix_acl_create()) later writes the ACL back, it invokes
->set_acl() or filesystem init_acl directly without going through the
SB_I_UID_R check in set_posix_acl(). Those filesystems serialize the ACL with
posix_acl_to_xattr(), which only emits version 2:

fs/posix_acl.c:posix_acl_to_xattr() {
	...
	ext_acl->a_version = cpu_to_le32(POSIX_ACL_XATTR_VERSION);

	for (n=0; n < acl->a_count; n++, ext_entry++) {
		const struct posix_acl_entry *acl_e = &acl->a_entries[n];
		ext_entry->e_tag  = cpu_to_le16(acl_e->e_tag);
		ext_entry->e_perm = cpu_to_le16(acl_e->e_perm);
		switch(acl_e->e_tag) {
		case ACL_USER:
			ext_entry->e_id =
				cpu_to_le32(from_kuid(user_ns, acl_e->e_uid));
			break;
		case ACL_GROUP:
			ext_entry->e_id =
				cpu_to_le32(from_kgid(user_ns, acl_e->e_gid));
			break;
	...
}

Because from_kuid() and from_kgid() return (uid_t)-1 and (gid_t)-1 for
non-legacy IDs, posix_acl_to_xattr() writes a version 2 xattr with e_id set to
ACL_UNDEFINED_ID (-1). When that xattr is subsequently read back by
posix_acl_from_xattr(), make_kuid() and make_kgid() produce INVALID_UID and
INVALID_GID, causing subsequent get_inode_acl(), getxattr(), and
generic_permission() calls on the inode to fail with -EINVAL.

Should version 3 xattr decoding be restricted to the VFS userspace xattr path
(or gated on filesystems that support version 3 / SB_I_UID_R), rather than
enabled for all posix_acl_from_xattr() callers whose writeback path uses
posix_acl_to_xattr()?

[ ... ]

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