| Message ID | 20220928160843.382601-15-brauner@kernel.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | acl: add vfs posix acl api | expand |
> +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. > +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. > +static inline int vfs_set_acl(struct user_namespace *mnt_userns, > + struct dentry *dentry, const char *name, > + struct posix_acl *acl) > +{ > + return 0; Should this really return 0 if ACLs are not supported?
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. > > > +static inline int vfs_set_acl(struct user_namespace *mnt_userns, > > + struct dentry *dentry, const char *name, > > + struct posix_acl *acl) > > +{ > > + return 0; > > Should this really return 0 if ACLs are not supported? Yeah, we should probably -EOPNOTSUPP for all of: vfs_{get,set,remove}_acl() in this case. Good point, thanks!
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; } /*
diff --git a/fs/posix_acl.c b/fs/posix_acl.c index 5b857f59535b..ef0908a4bc46 100644 --- a/fs/posix_acl.c +++ b/fs/posix_acl.c @@ -24,6 +24,9 @@ #include <linux/user_namespace.h> #include <linux/namei.h> #include <linux/mnt_idmapping.h> +#include <linux/security.h> +#include <linux/evm.h> +#include <linux/fsnotify.h> static struct posix_acl **acl_by_type(struct inode *inode, int type) { @@ -1254,3 +1257,115 @@ int simple_acl_create(struct inode *dir, struct inode *inode) posix_acl_release(acl); return 0; } + +static inline int posix_acl_type(const char *name) +{ + if (strcmp(name, XATTR_NAME_POSIX_ACL_ACCESS) == 0) + return ACL_TYPE_ACCESS; + else if (strcmp(name, XATTR_NAME_POSIX_ACL_DEFAULT) == 0) + return ACL_TYPE_DEFAULT; + + return -1; +} + +static int vfs_set_acl_idmapped_mnt(struct user_namespace *mnt_userns, + struct user_namespace *fs_userns, + struct posix_acl *acl) +{ + for (int n = 0; n < acl->a_count; n++) { + struct posix_acl_entry *acl_e = &acl->a_entries[n]; + + switch (acl_e->e_tag) { + case ACL_USER: + acl_e->e_uid = from_vfsuid(mnt_userns, fs_userns, + VFSUIDT_INIT(acl_e->e_uid)); + break; + case ACL_GROUP: + acl_e->e_gid = from_vfsgid(mnt_userns, fs_userns, + VFSGIDT_INIT(acl_e->e_gid)); + break; + } + } + + return 0; +} + +/** + * vfs_set_acl - set posix acls + * @mnt_userns: user namespace of the mount + * @dentry: the dentry based on which to set the posix acls + * @acl_name: the name of the posix acl + * @kacl: the posix acls in the appropriate VFS format + * + * This function sets @kacl. The caller must all posix_acl_release() on @kacl + * afterwards. + * + * Return: On success 0, on error negative errno. + */ +int vfs_set_acl(struct user_namespace *mnt_userns, struct dentry *dentry, + const char *acl_name, struct posix_acl *kacl) +{ + int acl_type; + int error; + struct inode *inode = d_inode(dentry); + struct inode *delegated_inode = NULL; + + acl_type = posix_acl_type(acl_name); + if (acl_type < 0) + return -EINVAL; + + if (kacl) { + /* + * If we're on an idmapped mount translate from mount specific + * vfs{g,u}id_t into global filesystem k{g,u}id_t. + * Afterwards we can cache the POSIX ACLs filesystem wide and - + * if this is a filesystem with a backing store - ultimately + * translate them to backing store values. + */ + error = vfs_set_acl_idmapped_mnt(mnt_userns, i_user_ns(inode), kacl); + if (error) + return error; + } + +retry_deleg: + inode_lock(inode); + + /* + * We only care about restrictions the inode struct itself places upon + * us otherwise POSIX ACLs aren't subject to any VFS restrictions. + */ + error = xattr_permission(mnt_userns, inode, acl_name, MAY_WRITE); + if (error) + goto out_inode_unlock; + + error = security_inode_set_acl(mnt_userns, dentry, acl_name, kacl); + if (error) + goto out_inode_unlock; + + error = try_break_deleg(inode, &delegated_inode); + if (error) + goto out_inode_unlock; + + if (inode->i_opflags & IOP_XATTR) + error = set_posix_acl(mnt_userns, dentry, acl_type, kacl); + else if (unlikely(is_bad_inode(inode))) + error = -EIO; + else + error = -EOPNOTSUPP; + if (!error) { + fsnotify_xattr(dentry); + evm_inode_post_set_acl(dentry, acl_name, kacl); + } + +out_inode_unlock: + inode_unlock(inode); + + if (delegated_inode) { + error = break_deleg_wait(&delegated_inode); + if (!error) + goto retry_deleg; + } + + return error; +} +EXPORT_SYMBOL(vfs_set_acl); diff --git a/fs/xattr.c b/fs/xattr.c index 61107b6bbed2..e16d7bde4935 100644 --- a/fs/xattr.c +++ b/fs/xattr.c @@ -84,9 +84,8 @@ xattr_resolve_name(struct inode *inode, const char **name) * Check permissions for extended attribute access. This is a bit complicated * because different namespaces have very different rules. */ -static int -xattr_permission(struct user_namespace *mnt_userns, struct inode *inode, - const char *name, int mask) +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 diff --git a/include/linux/posix_acl.h b/include/linux/posix_acl.h index cd16a756cd1e..85a5671204c4 100644 --- a/include/linux/posix_acl.h +++ b/include/linux/posix_acl.h @@ -99,6 +99,9 @@ static inline void cache_no_acl(struct inode *inode) inode->i_acl = NULL; inode->i_default_acl = NULL; } + +int vfs_set_acl(struct user_namespace *mnt_userns, struct dentry *dentry, + const char *acl_name, struct posix_acl *kacl); #else static inline int posix_acl_chmod(struct user_namespace *mnt_userns, struct dentry *dentry, umode_t mode) @@ -126,6 +129,13 @@ static inline int posix_acl_create(struct inode *inode, umode_t *mode, static inline void forget_all_cached_acls(struct inode *inode) { } + +static inline int vfs_set_acl(struct user_namespace *mnt_userns, + struct dentry *dentry, const char *name, + struct posix_acl *acl) +{ + return 0; +} #endif /* CONFIG_FS_POSIX_ACL */ struct posix_acl *get_acl(struct inode *inode, int type); diff --git a/include/linux/xattr.h b/include/linux/xattr.h index 4c379d23ec6e..8267e547e631 100644 --- a/include/linux/xattr.h +++ b/include/linux/xattr.h @@ -73,6 +73,8 @@ ssize_t vfs_getxattr_alloc(struct user_namespace *mnt_userns, char **xattr_value, size_t size, gfp_t flags); int xattr_supported_namespace(struct inode *inode, const char *prefix); +int xattr_permission(struct user_namespace *mnt_userns, struct inode *inode, + const char *name, int mask); static inline const char *xattr_prefix(const struct xattr_handler *handler) {
In previous patches we implemented get and set inode operations for all non-stacking filesystems that support posix acls but didn't yet implement get and/or set acl inode operations. This specifically affected cifs and 9p. Now we can build a posix acl api based solely on get and set inode operations. We add a new vfs_set_acl() api that can be used to set posix acls. This finally removes all type unsafety and type conversion issues explained in detail in [1] that we aim to get rid of. After we finished building the vfs api we can switch stacking filesystems to rely on the new posix api and then finally switch the xattr system calls themselves to rely on the posix acl api. Link: https://lore.kernel.org/all/20220801145520.1532837-1-brauner@kernel.org [1] Signed-off-by: Christian Brauner (Microsoft) <brauner@kernel.org> --- Notes: /* v2 */ unchanged /* v3 */ unchanged fs/posix_acl.c | 115 ++++++++++++++++++++++++++++++++++++++ fs/xattr.c | 5 +- include/linux/posix_acl.h | 10 ++++ include/linux/xattr.h | 2 + 4 files changed, 129 insertions(+), 3 deletions(-)