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@xxxxxxxxxx>
AuthorDate: Thu Sep 29 10:47:36 2022 +0200
Commit: Christian Brauner (Microsoft) <brauner@xxxxxxxxxx>
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@xxxxxxxxxx>
Notes:
To: linux-fsdevel@xxxxxxxxxxxxxxx
Cc: Seth Forshee <sforshee@xxxxxxxxxx>
Cc: Christoph Hellwig <hch@xxxxxx>
Cc: Al Viro <viro@xxxxxxxxxxxxxxxxxx>
Cc: linux-security-module@xxxxxxxxxxxxxxx
/* v2 */
patch not present
/* v3 */
patch not present
/* v4 */
Christoph Hellwig <hch@xxxxxx>:
- 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;
}
/*
