Re: [PATCH v6 09/40] xattr: handle idmapped mounts
On Wed, Mar 03, 2021 at 02:45:07PM +, David Howells wrote: > Christian Brauner wrote: > > > In order to answer this more confidently I need to know a bit more about > > how cachefiles are supposed to work. > > > > From what I gather here it seemed what this code is trying to set here > > is an internal "CacheFiles.cache" extended attribute on the indode. This > > extended attribute doesn't store any uids and gids or filesystem > > capabilities so the user namespace isn't relevant for that since there > > doesn't need to be any conversion. > > > > What I need to know is what information do you use for cachefiles to > > determine whether someone can set that "Cachefiles.cache" extended > > attribute on the inode: > > - Is it the mnt_userns of a/the mount of the filesystem you're caching for? > > - The mnt_userns of the mnt of struct cachefiles_cache? > > - Or the stashed or current creds of the caller? > > Mostly it's about permission checking. The cache driver wants to do accesses > onto the files in cache using the context of whatever process writes the > "bind" command to /dev/cachefiles, not the context of whichever process issued > a read or write, say, on an NFS file that is being cached. > > This causes standard UNIX perm checking, SELinux checking, etc. all to be > switched to the appropriate context. It also controls what appears in the > audit logs. (Audit always translates from and to init_user_ns. The changes to make it aware of user namespaces proper are delayed until the audit id thing is merged as Paul pointed out to me.) > > There is an exception to this: It also governs the ownership of new files and > directories created in the cache and what security labels will be set on them. So from our offline discussion I gather that cachefilesd creates a cache on a local filesystem (ext4, xfs etc.) for a network filesystem. The way this is done is by writing "bind" to /dev/cachefiles and pointing it to a directory to use as the cache. This directory can currently also be an idmapped mount, say: mount --bind --idmap /mnt /mnt and then pointing cachefilesd via a "bind" operation to /mnt What I would expect is for cachefilesd to now take that idmapping into account when creating files in /mnt but as it stands now, it doesn't. This could leave users confused as the ownership of the files wouldn't match to what they expressed in the idmapping. Since you're reworking cachefilesd currently anyway, I would suggest we port cachefilesd to support idmapped mounts once as part of your rework. I can help there and until then we do: diff --git a/fs/cachefiles/bind.c b/fs/cachefiles/bind.c index dfb14dbddf51..51f21beafad9 100644 --- a/fs/cachefiles/bind.c +++ b/fs/cachefiles/bind.c @@ -115,6 +115,12 @@ static int cachefiles_daemon_add_cache(struct cachefiles_cache *cache) if (ret < 0) goto error_open_root; + if (mnt_user_ns(path.mnt) != _user_ns) { + ret = -EPERM; + pr_err("Caches on idmapped mounts are currently not supported\n"); + goto error_open_root; + } + cache->mnt = path.mnt; root = path.dentry; This is safe to do because if a mount is visible in the filesystem it can't change it's idmapping. (Might even be worth if you add a helper at this point: static inline bool mnt_is_idmapped(struct vfsmount *mnt) { return mnt_user_ns(mnt) != _user_ns; } ) Christian
Re: [PATCH v6 09/40] xattr: handle idmapped mounts
On Wed, Mar 03, 2021 at 01:24:02PM +, David Howells wrote: > Christian Brauner wrote: > > > diff --git a/fs/cachefiles/xattr.c b/fs/cachefiles/xattr.c > > index 72e42438f3d7..a591b5e09637 100644 > > --- a/fs/cachefiles/xattr.c > > +++ b/fs/cachefiles/xattr.c > > @@ -39,8 +39,8 @@ int cachefiles_check_object_type(struct cachefiles_object > > *object) > > _enter("%p{%s}", object, type); > > > > /* attempt to install a type label directly */ > > - ret = vfs_setxattr(dentry, cachefiles_xattr_cache, type, 2, > > - XATTR_CREATE); > > + ret = vfs_setxattr(_user_ns, dentry, cachefiles_xattr_cache, type, > > + 2, XATTR_CREATE); > Hey David, (Ok, recovered from my run-in with the swapfile bug. I even managed to get my emails back.) > Actually, on further consideration, this might be the wrong thing to do in > cachefiles. The creds are (or should be) overridden when accesses to the > underlying filesystem are being made. > > I wonder if this should be using current_cred()->user_ns or > cache->cache_cred->user_ns instead. Before I go into the second question please note that this is a no-op change. So if this is wrong it was wrong before. Which is your point, I guess. Please also note that the mnt_userns is _never_ used for (capability) permission checking, only for idmapping vfs objects and permission checks based on the i_uid and i_gid. So if your argument about passing one of those two user namespaces above has anything to do with permission checking on caps it's most likely wrong. :) In order to answer this more confidently I need to know a bit more about how cachefiles are supposed to work. >From what I gather here it seemed what this code is trying to set here is an internal "CacheFiles.cache" extended attribute on the indode. This extended attribute doesn't store any uids and gids or filesystem capabilities so the user namespace isn't relevant for that since there doesn't need to be any conversion. What I need to know is what information do you use for cachefiles to determine whether someone can set that "Cachefiles.cache" extended attribute on the inode: - Is it the mnt_userns of a/the mount of the filesystem you're caching for? - The mnt_userns of the mnt of struct cachefiles_cache? - Or the stashed or current creds of the caller? Christian
Re: [PATCH v6 09/40] xattr: handle idmapped mounts
Christian Brauner wrote: > In order to answer this more confidently I need to know a bit more about > how cachefiles are supposed to work. > > From what I gather here it seemed what this code is trying to set here > is an internal "CacheFiles.cache" extended attribute on the indode. This > extended attribute doesn't store any uids and gids or filesystem > capabilities so the user namespace isn't relevant for that since there > doesn't need to be any conversion. > > What I need to know is what information do you use for cachefiles to > determine whether someone can set that "Cachefiles.cache" extended > attribute on the inode: > - Is it the mnt_userns of a/the mount of the filesystem you're caching for? > - The mnt_userns of the mnt of struct cachefiles_cache? > - Or the stashed or current creds of the caller? Mostly it's about permission checking. The cache driver wants to do accesses onto the files in cache using the context of whatever process writes the "bind" command to /dev/cachefiles, not the context of whichever process issued a read or write, say, on an NFS file that is being cached. This causes standard UNIX perm checking, SELinux checking, etc. all to be switched to the appropriate context. It also controls what appears in the audit logs. There is an exception to this: It also governs the ownership of new files and directories created in the cache and what security labels will be set on them. Quite possibly this doesn't matter for the xattr stuff. It's hard to tell since we use user namespaces to convey so many different things at different times. David
Re: [PATCH v6 09/40] xattr: handle idmapped mounts
Christian Brauner wrote: > diff --git a/fs/cachefiles/xattr.c b/fs/cachefiles/xattr.c > index 72e42438f3d7..a591b5e09637 100644 > --- a/fs/cachefiles/xattr.c > +++ b/fs/cachefiles/xattr.c > @@ -39,8 +39,8 @@ int cachefiles_check_object_type(struct cachefiles_object > *object) > _enter("%p{%s}", object, type); > > /* attempt to install a type label directly */ > - ret = vfs_setxattr(dentry, cachefiles_xattr_cache, type, 2, > -XATTR_CREATE); > + ret = vfs_setxattr(_user_ns, dentry, cachefiles_xattr_cache, type, > +2, XATTR_CREATE); Actually, on further consideration, this might be the wrong thing to do in cachefiles. The creds are (or should be) overridden when accesses to the underlying filesystem are being made. I wonder if this should be using current_cred()->user_ns or cache->cache_cred->user_ns instead. David