Re: [PATCH v3 14/29] acl: add vfs_set_acl()

[Christian Brauner <brauner@kernel.org>]

On Thu, Sep 29, 2022 at 10:25:59AM +0200, Christian Brauner wrote:
> On Thu, Sep 29, 2022 at 10:17:27AM +0200, Christoph Hellwig wrote:
> > > +EXPORT_SYMBOL(vfs_set_acl);
> > 
> > I think all this stackable file system infrastucture should be
> > EXPORT_SYMBOL_GPL, like a lot of the other internal stuff.
> 
> Ok, sounds good.
> 
> > 
> > > +int xattr_permission(struct user_namespace *mnt_userns, struct inode *inode,
> > > +		     const char *name, int mask)
> > 
> > Hmm.  The only think ACLs actually need from xattr_permission are
> > the immutable / append check and the HAS_UNMAPPED_ID one.  I'd rather
> > open code that, or if you cane come up with a sane name do a smaller
> > helper rather than doing all the strcmp on the prefixes for now
> > good reason.
> 
> I'll see if a little helper makes more sense than open-coding.

So I've added - which is then used in vfs_{set,remove}_acl():

commit 6ae39d028cb6990d69a7ec27386fc1bb7b1f3e3b
Author:     Christian Brauner <brauner@kernel.org>
AuthorDate: Thu Sep 29 10:47:36 2022 +0200
Commit:     Christian Brauner (Microsoft) <brauner@kernel.org>
CommitDate: Thu Sep 29 10:59:27 2022 +0200

    internal: add may_write_xattr()
    
    Split out the generic checks whether an inode allows writing xattrs. Since
    security.* and system.* xattrs don't have any restrictions and we're going
    to split out posix acls into a dedicated api we will use this helper to
    check whether we can write posix acls.
    
    Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org>

Notes:
    To: linux-fsdevel@vger.kernel.org
    Cc: Seth Forshee <sforshee@kernel.org>
    Cc: Christoph Hellwig <hch@lst.de>
    Cc: Al Viro <viro@zeniv.linux.org.uk>
    Cc: linux-security-module@vger.kernel.org
    
    /* v2 */
    patch not present
    
    /* v3 */
    patch not present
    
    /* v4 */
    Christoph Hellwig <hch@lst.de>:
    - Split out checks whether an inode can have xattrs written to into a helper.

diff --git a/fs/internal.h b/fs/internal.h
index 87e96b9024ce..a95b1500ed65 100644
--- a/fs/internal.h
+++ b/fs/internal.h
@@ -221,3 +221,4 @@ ssize_t do_getxattr(struct user_namespace *mnt_userns,
 int setxattr_copy(const char __user *name, struct xattr_ctx *ctx);
 int do_setxattr(struct user_namespace *mnt_userns, struct dentry *dentry,
 		struct xattr_ctx *ctx);
+int may_write_xattr(struct user_namespace *mnt_userns, struct inode *inode);
diff --git a/fs/xattr.c b/fs/xattr.c
index 61107b6bbed2..57148c207545 100644
--- a/fs/xattr.c
+++ b/fs/xattr.c
@@ -80,6 +80,28 @@ xattr_resolve_name(struct inode *inode, const char **name)
 	return ERR_PTR(-EOPNOTSUPP);
 }
 
+/**
+ * may_write_xattr - check whether inode allows writing xattr
+ * @mnt_userns:	User namespace of the mount the inode was found from
+ * @inode: the inode on which to set an xattr
+ *
+ * Check whether the inode allows writing xattrs. Specifically, we can never
+ * set or remove an extended attribute on a read-only filesystem  or on an
+ * immutable / append-only inode.
+ *
+ * We also need to ensure that the inode has a mapping in the mount to
+ * not risk writing back invalid i_{g,u}id values.
+ *
+ * Return: On success zero is returned. On error a negative errno is returned.
+ */
+int may_write_xattr(struct user_namespace *mnt_userns, struct inode *inode)
+{
+	if (IS_IMMUTABLE(inode) || IS_APPEND(inode) ||
+	    HAS_UNMAPPED_ID(mnt_userns, inode))
+		return -EPERM;
+	return 0;
+}
+
 /*
  * Check permissions for extended attribute access.  This is a bit complicated
  * because different namespaces have very different rules.
@@ -88,20 +110,12 @@ static int
 xattr_permission(struct user_namespace *mnt_userns, struct inode *inode,
 		 const char *name, int mask)
 {
-	/*
-	 * We can never set or remove an extended attribute on a read-only
-	 * filesystem  or on an immutable / append-only inode.
-	 */
 	if (mask & MAY_WRITE) {
-		if (IS_IMMUTABLE(inode) || IS_APPEND(inode))
-			return -EPERM;
-		/*
-		 * Updating an xattr will likely cause i_uid and i_gid
-		 * to be writen back improperly if their true value is
-		 * unknown to the vfs.
-		 */
-		if (HAS_UNMAPPED_ID(mnt_userns, inode))
-			return -EPERM;
+		int ret;
+
+		ret = may_write_xattr(mnt_userns, inode);
+		if (ret)
+			return ret;
 	}
 
 	/*