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 0c5fd887d2bb ("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/
Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org>
Reviewed-by: Seth Forshee (DigitalOcean) <sforshee@kernel.org>

diff --git a/fs/posix_acl.c b/fs/posix_acl.c
index 31eac28e6582dcf09f42f59bdfc8c8512097f379..c759b8eef62e83f4e06f9b9d43c9b6f3275af952 100644 (file)
--- a/fs/posix_acl.c
+++ b/fs/posix_acl.c
@@ -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);
        }

diff --git a/fs/xattr.c b/fs/xattr.c
index a1f4998bc6be30efe98aa47f494a533c25153c7f..3ac68ec0c0230de60b268d8753cdb5c85c777ec0 100644 (file)
--- a/fs/xattr.c
+++ b/fs/xattr.c
@@ -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,

diff --git a/include/linux/posix_acl_xattr.h b/include/linux/posix_acl_xattr.h
index 47eca15fd842292ea5876a32149cf23f8be71490..8163dd48c430189eb6b512aa3db6df07eb0016d5 100644 (file)
--- a/include/linux/posix_acl_xattr.h
+++ b/include/linux/posix_acl_xattr.h
@@ -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, 

diff --git a/security/integrity/evm/evm_main.c b/security/integrity/evm/evm_main.c
index 2e6fb6e2ffd2fe5e62022c80127f6c26bfbcfb3d..23d484e05e6f2f59975e0cbdd4e1ea9c479142cd 100644 (file)
--- a/security/integrity/evm/evm_main.c
+++ b/security/integrity/evm/evm_main.c
@@ -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;