Unverified Commit 52edb408 authored by Christian Brauner's avatar Christian Brauner Committed by Christian Brauner (Microsoft)
Browse files

acl: move idmapping handling into posix_acl_xattr_set() 

The uapi POSIX ACL struct passed through the value argument during
setxattr() contains {g,u}id values encoded via ACL_{GROUP,USER} entries
that should actually be stored in the form of k{g,u}id_t (See [1] for a
long explanation of the issue.).

In 0c5fd887 ("acl: move idmapped mount fixup into vfs_{g,s}etxattr()")
we took the mount's idmapping into account in order to let overlayfs
handle POSIX ACLs on idmapped layers correctly. The fixup is currently
performed directly in vfs_setxattr() which piles on top of the earlier
hackiness by handling the mount's idmapping and stuff the vfs{g,u}id_t
values into the uapi struct as well. While that is all correct and works
fine it's just ugly.

Now that we have introduced vfs_make_posix_acl() earlier move handling
idmapped mounts out of vfs_setxattr() and into the POSIX ACL handler
where it belongs.

Note that we also need to call vfs_make_posix_acl() for EVM which
interpretes POSIX ACLs during security_inode_setxattr(). Leave them a
longer comment for future reference.

All filesystems that support idmapped mounts via FS_ALLOW_IDMAP use the
standard POSIX ACL xattr handlers and are covered by this change. This
includes overlayfs which simply calls vfs_{g,s}etxattr().

The following filesystems use custom POSIX ACL xattr handlers: 9p, cifs,
ecryptfs, and ntfs3 (and overlayfs but we've covered that in the paragraph
above) and none of them support idmapped mounts yet.

Link: https://lore.kernel.org/all/20220801145520.1532837-1-brauner@kernel.org/

 [1]
Signed-off-by: default avatarChristian Brauner (Microsoft) <brauner@kernel.org>
Reviewed-by: default avatarSeth Forshee (DigitalOcean) <sforshee@kernel.org>
parent 6b70fe06

fs/posix_acl.c

+11 −41
Original line number Diff line number Diff line
@@ -771,46 +771,6 @@ void posix_acl_getxattr_idmapped_mnt(struct user_namespace *mnt_userns, 
	}

}



void posix_acl_setxattr_idmapped_mnt(struct user_namespace *mnt_userns,

				     const struct inode *inode,

				     void *value, size_t size)

{

	struct posix_acl_xattr_header *header = value;

	struct posix_acl_xattr_entry *entry = (void *)(header + 1), *end;

	struct user_namespace *fs_userns = i_user_ns(inode);

	int count;

	vfsuid_t vfsuid;

	vfsgid_t vfsgid;

	kuid_t uid;

	kgid_t gid;



	if (no_idmapping(mnt_userns, i_user_ns(inode)))

		return;



	count = posix_acl_fix_xattr_common(value, size);

	if (count <= 0)

		return;



	for (end = entry + count; entry != end; entry++) {

		switch (le16_to_cpu(entry->e_tag)) {

		case ACL_USER:

			uid = make_kuid(&init_user_ns, le32_to_cpu(entry->e_id));

			vfsuid = VFSUIDT_INIT(uid);

			uid = from_vfsuid(mnt_userns, fs_userns, vfsuid);

			entry->e_id = cpu_to_le32(from_kuid(&init_user_ns, uid));

			break;

		case ACL_GROUP:

			gid = make_kgid(&init_user_ns, le32_to_cpu(entry->e_id));

			vfsgid = VFSGIDT_INIT(gid);

			gid = from_vfsgid(mnt_userns, fs_userns, vfsgid);

			entry->e_id = cpu_to_le32(from_kgid(&init_user_ns, gid));

			break;

		default:

			break;

		}

	}

}



static void posix_acl_fix_xattr_userns(

	struct user_namespace *to, struct user_namespace *from,

	void *value, size_t size)
@@ -1211,7 +1171,17 @@ posix_acl_xattr_set(const struct xattr_handler *handler, 
	int ret;



	if (value) {

		acl = posix_acl_from_xattr(&init_user_ns, value, size);

		/*

		 * By the time we end up here the {g,u}ids stored in

		 * ACL_{GROUP,USER} have already been mapped according to the

		 * caller's idmapping. The vfs_set_acl_prepare() helper will

		 * recover them and take idmapped mounts into account. The

		 * filesystem will receive the POSIX ACLs in in the correct

		 * format ready to be cached or written to the backing store

		 * taking the filesystem idmapping into account.

		 */

		acl = vfs_set_acl_prepare(mnt_userns, i_user_ns(inode),

					  value, size);

		if (IS_ERR(acl))

			return PTR_ERR(acl);

	}

fs/xattr.c

+0 −3
Original line number Diff line number Diff line
@@ -305,9 +305,6 @@ vfs_setxattr(struct user_namespace *mnt_userns, struct dentry *dentry, 
		size = error;

	}



	if (size && is_posix_acl_xattr(name))

		posix_acl_setxattr_idmapped_mnt(mnt_userns, inode, value, size);



retry_deleg:

	inode_lock(inode);

	error = __vfs_setxattr_locked(mnt_userns, dentry, name, value, size,

include/linux/posix_acl_xattr.h

+0 −9
Original line number Diff line number Diff line
@@ -38,9 +38,6 @@ void posix_acl_fix_xattr_to_user(void *value, size_t size); 
void posix_acl_getxattr_idmapped_mnt(struct user_namespace *mnt_userns,

				     const struct inode *inode,

				     void *value, size_t size);

void posix_acl_setxattr_idmapped_mnt(struct user_namespace *mnt_userns,

				     const struct inode *inode,

				     void *value, size_t size);

#else

static inline void posix_acl_fix_xattr_from_user(void *value, size_t size)

{
@@ -54,12 +51,6 @@ posix_acl_getxattr_idmapped_mnt(struct user_namespace *mnt_userns, 
				size_t size)

{

}

static inline void

posix_acl_setxattr_idmapped_mnt(struct user_namespace *mnt_userns,

				const struct inode *inode, void *value,

				size_t size)

{

}

#endif



struct posix_acl *posix_acl_from_xattr(struct user_namespace *user_ns,

security/integrity/evm/evm_main.c

+14 −3
Original line number Diff line number Diff line
@@ -457,10 +457,21 @@ static int evm_xattr_acl_change(struct user_namespace *mnt_userns, 
	int rc;



	/*

	 * user_ns is not relevant here, ACL_USER/ACL_GROUP don't have impact

	 * on the inode mode (see posix_acl_equiv_mode()).

	 * An earlier comment here mentioned that the idmappings for

	 * ACL_{GROUP,USER} don't matter since EVM is only interested in the

	 * mode stored as part of POSIX ACLs. Nonetheless, if it must translate

	 * from the uapi POSIX ACL representation to the VFS internal POSIX ACL

	 * representation it should do so correctly. There's no guarantee that

	 * we won't change POSIX ACLs in a way that ACL_{GROUP,USER} matters

	 * for the mode at some point and it's difficult to keep track of all

	 * the LSM and integrity modules and what they do to POSIX ACLs.

	 *

	 * Frankly, EVM shouldn't try to interpret the uapi struct for POSIX

	 * ACLs it received. It requires knowledge that only the VFS is

	 * guaranteed to have.

	 */

	acl = posix_acl_from_xattr(&init_user_ns, xattr_value, xattr_value_len);

	acl = vfs_set_acl_prepare(mnt_userns, i_user_ns(inode),

				  xattr_value, xattr_value_len);

	if (IS_ERR_OR_NULL(acl))

		return 1;