On Tue, Apr 20, 2021 at 01:41:59PM +0200, Christian Brauner wrote:
> On Tue, Apr 20, 2021 at 09:01:09AM +0300, Amir Goldstein wrote:
> > > One thing, whatever you end up passing to vfs_create() please make sure
> > > to retrieve mnt_userns once so permission checking and object creation
> > > line-up:
> > >
> > > int vfs_create(struct vfsmount *mnt, struct inode *dir,
> > > struct dentry *dentry, umode_t mode, bool want_excl)
> > > {
> > > struct user_namespace *mnt_userns;
> > >
> > > mnt_userns = mnt_user_ns(mnt);
> > >
> > > int error = may_create(mnt_userns, dir, dentry);
> > > if (error)
> > > return error;
> > >
> > > if (!dir->i_op->create)
> > > return -EACCES; /* shouldn't it be ENOSYS? */
> > > mode &= S_IALLUGO;
> > > mode |= S_IFREG;
> > > error = security_inode_create(dir, dentry, mode);
> > > if (error)
> > > return error;
> > > error = dir->i_op->create(mnt_userns, dir, dentry, mode, want_excl);
> > > if (!error)
> > > fsnotify_create(mnt, dir, dentry);
> > > return error;
> > > }
> > >
> >
> > Christian,
> >
> > What is the concern here?
> > Can mnt_user_ns() change under us?
> > I am asking because Al doesn't like both mnt_userns AND path to
> > be passed to do_tuncate() => notify_change()
> > So I will need to retrieve mnt_userns again inside notify_change()
> > after it had been used for security checks in do_open().
> > Would that be acceptable to you?
>
> The mnt_userns can't change once a mnt has been idmapped and it can
> never change if the mount is visible in the filesystem already. The only
> case we've been worried about and why we did it this way is when you
> have a caller do fd = open_tree(OPEN_TREE_CLONE) and then share that
> unattached fd with multiple processes
> T1: mkdirat(fd, "dir1", 0755);
> T2: mount_setattr(fd, "",); /* changes idmapping */
> That case isn't a problem if the mnt_userns is only retrieved once for
> permission checking and operating on the inode. I think with your
> changes that still shouldn't be an issue though since the vfs_*()
> helpers encompass the permission checking anyway and for notify_change,
> we could simply add a mnt_userns field to struct iattr and pass it down.
So I mean something along those lines. I converted a few callers to
illustrate this and I hope Al doesn't kill me. Please note that this
won't compile since I haven't converted all callers. I can give you a
full patch though if you think that is ok:
---
drivers/base/devtmpfs.c | 6 ++++--
fs/attr.c | 16 ++++++++--------
fs/cachefiles/interface.c | 10 ++++++++--
fs/ecryptfs/inode.c | 12 ++++++++++--
fs/inode.c | 7 ++++---
include/linux/fs.h | 3 ++-
6 files changed, 36 insertions(+), 18 deletions(-)
diff --git a/drivers/base/devtmpfs.c b/drivers/base/devtmpfs.c
index 653c8c6ac7a7..323a549c62e3 100644
--- a/drivers/base/devtmpfs.c
+++ b/drivers/base/devtmpfs.c
@@ -221,8 +221,9 @@ static int handle_create(const char *nodename, umode_t mode, kuid_t uid,
newattrs.ia_uid = uid;
newattrs.ia_gid = gid;
newattrs.ia_valid = ATTR_MODE|ATTR_UID|ATTR_GID;
+ newattrs.mnt_userns = &init_user_ns;
inode_lock(d_inode(dentry));
- notify_change(&init_user_ns, dentry, &newattrs, NULL);
+ notify_change(path, dentry, &newattrs, NULL);
inode_unlock(d_inode(dentry));
/* mark as kernel-created inode */
@@ -329,8 +330,9 @@ static int handle_remove(const char *nodename, struct device *dev)
newattrs.ia_mode = stat.mode & ~0777;
newattrs.ia_valid =
ATTR_UID|ATTR_GID|ATTR_MODE;
+ newattrs.mnt_userns = &init_user_ns;
inode_lock(d_inode(dentry));
- notify_change(&init_user_ns, dentry, &newattrs, NULL);
+ notify_change(path, dentry, &newattrs, NULL);
inode_unlock(d_inode(dentry));
err = vfs_unlink(&init_user_ns, d_inode(parent.dentry),
dentry, NULL);
diff --git a/fs/attr.c b/fs/attr.c
index 87ef39db1c34..59a9ed986e49 100644
--- a/fs/attr.c
+++ b/fs/attr.c
@@ -279,7 +279,7 @@ EXPORT_SYMBOL(setattr_copy);
* permissions. On non-idmapped mounts or if permission checking is to be
* performed on the raw inode simply passs init_user_ns.
*/
-int notify_change(struct user_namespace *mnt_userns, struct dentry *dentry,
+int notify_change(struct path *path, struct dentry *dentry,
struct iattr *attr, struct inode **delegated_inode)
{
struct inode *inode = dentry->d_inode;
@@ -303,8 +303,8 @@ int notify_change(struct user_namespace *mnt_userns, struct dentry *dentry,
if (IS_IMMUTABLE(inode))
return -EPERM;
- if (!inode_owner_or_capable(mnt_userns, inode)) {
- error = inode_permission(mnt_userns, inode, MAY_WRITE);
+ if (!inode_owner_or_capable(attr->mnt_userns, inode)) {
+ error = inode_permission(attr->mnt_userns, inode, MAY_WRITE);
if (error)
return error;
}
@@ -381,10 +381,10 @@ int notify_change(struct user_namespace *mnt_userns, struct dentry *dentry,
* gids unless those uids & gids are being made valid.
*/
if (!(ia_valid & ATTR_UID) &&
- !uid_valid(i_uid_into_mnt(mnt_userns, inode)))
+ !uid_valid(i_uid_into_mnt(attr->mnt_userns, inode)))
return -EOVERFLOW;
if (!(ia_valid & ATTR_GID) &&
- !gid_valid(i_gid_into_mnt(mnt_userns, inode)))
+ !gid_valid(i_gid_into_mnt(attr->mnt_userns, inode)))
return -EOVERFLOW;
error = security_inode_setattr(dentry, attr);
@@ -395,13 +395,13 @@ int notify_change(struct user_namespace *mnt_userns, struct dentry *dentry,
return error;
if (inode->i_op->setattr)
- error = inode->i_op->setattr(mnt_userns, dentry, attr);
+ error = inode->i_op->setattr(attr->mnt_userns, dentry, attr);
else
- error = simple_setattr(mnt_userns, dentry, attr);
+ error = simple_setattr(attr->mnt_userns, dentry, attr);
if (!error) {
fsnotify_change(dentry, ia_valid);
- ima_inode_post_setattr(mnt_userns, dentry);
+ ima_inode_post_setattr(attr->mnt_userns, dentry);
evm_inode_post_setattr(dentry, ia_valid);
}
diff --git a/fs/cachefiles/interface.c b/fs/cachefiles/interface.c
index 5efa6a3702c0..cede4b790694 100644
--- a/fs/cachefiles/interface.c
+++ b/fs/cachefiles/interface.c
@@ -429,6 +429,7 @@ static int cachefiles_check_consistency(struct fscache_operation *op)
*/
static int cachefiles_attr_changed(struct fscache_object *_object)
{
+ struct path path;
struct cachefiles_object *object;
struct cachefiles_cache *cache;
const struct cred *saved_cred;
@@ -460,6 +461,9 @@ static int cachefiles_attr_changed(struct fscache_object *_object)
if (oi_size == ni_size)
return 0;
+ path.dentry = object->backer;
+ path.mnt = cache->mnt;
+
cachefiles_begin_secure(cache, &saved_cred);
inode_lock(d_inode(object->backer));
@@ -470,14 +474,16 @@ static int cachefiles_attr_changed(struct fscache_object *_object)
_debug("discard tail %llx", oi_size);
newattrs.ia_valid = ATTR_SIZE;
newattrs.ia_size = oi_size & PAGE_MASK;
- ret = notify_change(&init_user_ns, object->backer, &newattrs, NULL);
+ newattr.mnt_userns = &init_user_ns;
+ ret = notify_change(&path, object->backer, &newattrs, NULL);
if (ret < 0)
goto truncate_failed;
}
newattrs.ia_valid = ATTR_SIZE;
newattrs.ia_size = ni_size;
- ret = notify_change(&init_user_ns, object->backer, &newattrs, NULL);
+ newattr.mnt_userns = &init_user_ns;
+ ret = notify_change(&path, object->backer, &newattrs, NULL);
truncate_failed:
inode_unlock(d_inode(object->backer));
diff --git a/fs/ecryptfs/inode.c b/fs/ecryptfs/inode.c
index 18e9285fbb4c..8347742087e0 100644
--- a/fs/ecryptfs/inode.c
+++ b/fs/ecryptfs/inode.c
@@ -867,10 +867,14 @@ int ecryptfs_truncate(struct dentry *dentry, loff_t new_length)
rc = truncate_upper(dentry, &ia, &lower_ia);
if (!rc && lower_ia.ia_valid & ATTR_SIZE) {
+ struct path *lower_path;
struct dentry *lower_dentry = ecryptfs_dentry_to_lower(dentry);
+ lower_path = ecryptfs_dentry_to_lower_path(dentry);
+ lower_ia.mnt_userns = mnt_user_ns(lower_path);
+
inode_lock(d_inode(lower_dentry));
- rc = notify_change(&init_user_ns, lower_dentry,
+ rc = notify_change(lower_path, lower_dentry,
&lower_ia, NULL);
inode_unlock(d_inode(lower_dentry));
}
@@ -906,6 +910,7 @@ static int ecryptfs_setattr(struct user_namespace *mnt_userns,
struct inode *inode;
struct inode *lower_inode;
struct ecryptfs_crypt_stat *crypt_stat;
+ struct path *lower_path;
crypt_stat = &ecryptfs_inode_to_private(d_inode(dentry))->crypt_stat;
if (!(crypt_stat->flags & ECRYPTFS_STRUCT_INITIALIZED)) {
@@ -977,8 +982,11 @@ static int ecryptfs_setattr(struct user_namespace *mnt_userns,
if (lower_ia.ia_valid & (ATTR_KILL_SUID | ATTR_KILL_SGID))
lower_ia.ia_valid &= ~ATTR_MODE;
+ lower_path = ecryptfs_dentry_to_lower_path(dentry);
+ lower_ia.mnt_userns = mnt_user_ns(lower_path);
+
inode_lock(d_inode(lower_dentry));
- rc = notify_change(&init_user_ns, lower_dentry, &lower_ia, NULL);
+ rc = notify_change(lower_path, lower_dentry, &lower_ia, NULL);
inode_unlock(d_inode(lower_dentry));
out:
fsstack_copy_attr_all(inode, lower_inode);
diff --git a/fs/inode.c b/fs/inode.c
index a047ab306f9a..12a1531a6c52 100644
--- a/fs/inode.c
+++ b/fs/inode.c
@@ -1906,17 +1906,18 @@ int dentry_needs_remove_privs(struct dentry *dentry)
return mask;
}
-static int __remove_privs(struct user_namespace *mnt_userns,
+static int __remove_privs(struct file *file,
struct dentry *dentry, int kill)
{
struct iattr newattrs;
newattrs.ia_valid = ATTR_FORCE | kill;
+ newattrs.mnt_userns = file_mnt_user_ns(file);
/*
* Note we call this on write, so notify_change will not
* encounter any conflicting delegations:
*/
- return notify_change(mnt_userns, dentry, &newattrs, NULL);
+ return notify_change(file->f_path, dentry, &newattrs, NULL);
}
/*
@@ -1943,7 +1944,7 @@ int file_remove_privs(struct file *file)
if (kill < 0)
return kill;
if (kill)
- error = __remove_privs(file_mnt_user_ns(file), dentry, kill);
+ error = __remove_privs(file, dentry, kill);
if (!error)
inode_has_no_xattr(inode);
diff --git a/include/linux/fs.h b/include/linux/fs.h
index ec8f3ddf4a6a..d23bcedf5f92 100644
--- a/include/linux/fs.h
+++ b/include/linux/fs.h
@@ -234,6 +234,7 @@ struct iattr {
* check for (ia_valid & ATTR_FILE), and not for (ia_file != NULL).
*/
struct file *ia_file;
+ struct user_namespace *mnt_userns;
};
/*
@@ -2862,7 +2863,7 @@ static inline int bmap(struct inode *inode, sector_t *block)
}
#endif
-int notify_change(struct user_namespace *, struct dentry *,
+int notify_change(struct path *, struct dentry *,
struct iattr *, struct inode **);
int inode_permission(struct user_namespace *, struct inode *, int);
int generic_permission(struct user_namespace *, struct inode *, int);
--
2.27.0
