mqueue: forbid unprivileged user access to internal mount
Subsystems (5)
PATCH 1/1 mqueue: forbid unprivileged user access to internal mount
Hi all,
Felix reported weird behaviour on 4.16.0-rc6 with regards to mqueue[1], which was introduced by 36735a6a2b5e ("mqueue: switch to on-demand creation of internal mount").
Basically, the reproducer boils down to being able to mount mqueue if you create a new user namespace, even if you don't unshare the IPC namespace.
Previously this was not possible, and you would get an -EPERM. The mount is the *host* mqueue mount, which is being cached and just returned from mqueue_mount(). To be honest, I'm not sure if this is safe or not (or if it was intentional -- since I'm not familiar with mqueue).
To me it looks like there is a missing permission check. I've included a patch below that I've compile-tested, and should block the above case. Can someone please tell me if I'm missing something? Is this actually safe?
[1]: https://github.com/docker/docker/issues/36674
--8<--------------------------------------------------------------------
Fix a regression caused by 36735a6a2b5e ("mqueue: switch to on-demand creation of internal mount"), where an unprivileged user is permitted to mount mqueue even if they don't have CAP_SYS_ADMIN in the ipcns's associated userns. This can be reproduced as in the following.
% unshare -Urm # ipc isn't unshare'd
# mount -t mqueue mqueue /dev/mqueue # should have failed # echo $? 0
Previously the above would error out with an -EPERM, as the mount was protected by mount_ns(), but the patch in question switched to kern_mount_data() which doesn't do this necessary permission check. So add it explicitly to mq_internal_mount().
--- ipc/mqueue.c | 6 ++++++ 1 file changed, 6 insertions(+)
ipc/mqueue.c +6 -0
@@ -353,6 +353,12 @@ static struct vfsmount *mq_internal_mount(void) {struct ipc_namespace *ns = current->nsproxy->ipc_ns;struct vfsmount *m = ns->mq_mnt; +/* +* Match the semantics of mount_ns, to avoid unprivileged users from being +* able to mount mqueue from an IPC namespace they don't have ownership of. +*/ +if (!ns_capable (ns->user_ns,CAP_SYS_ADMIN )) +return ERR_PTR (-EPERM );if (m)return m; m =kern_mount_data (&mqueue_fs_type, ns);
Signature · 7 lines
2.16.2 -- Aleksa Sarai Senior Software Engineer (Containers) SUSE Linux GmbH <https://www.cyphar.com/>
_______________________________________________
Containers mailing list Containers-cunTk1MwBs9QetFLy7KEm3xJsTq8ys+cHZ5vskTnxNA@public.gmane.org
https://lists.linuxfoundation.org/mailman/listinfo/containers
Quoted 15 lines · Aleksa Sarai
Hi all,
Felix reported weird behaviour on 4.16.0-rc6 with regards to mqueue[1], which was introduced by
36735a6a2b5e("mqueue: switch to on-demand creation of internal mount").Basically, the reproducer boils down to being able to mount mqueue if you create a new user namespace, even if you don't unshare the IPC namespace.
Previously this was not possible, and you would get an
-EPERM. The mount is the *host* mqueue mount, which is being cached and just returned frommqueue_mount(). To be honest, I'm not sure if this is safe or not (or if it was intentional -- since I'm not familiar with mqueue).
Quoted 4 lines
To me it looks like there is a missing permission check. I've included a patch below that I've compile-tested, and should block the above case. Can someone please tell me if I'm missing something? Is this actually safe?
I think it may be safe by luck. If mqueuefs had any mount options this would allow them to be changed.
Looking at the code there is another issue. sb->s_user_ns is getting set to &init_user_ns instead of ns->user_ns. That will cause other operations to fail like mount -o remount to fail that should not.
So I think the fix needs a little more work.
Eric
Quoted 46 lines
[1]: https://github.com/docker/docker/issues/36674
--8<--------------------------------------------------------------------
Fix a regression caused by
36735a6a2b5e("mqueue: switch to on-demand creation of internal mount"), where an unprivileged user is permitted to mount mqueue even if they don't haveCAP_SYS_ADMINin the ipcns's associated userns. This can be reproduced as in the following.% unshare -Urm # ipc isn't unshare'd
# mount -t mqueue mqueue /dev/mqueue # should have failed # echo $? 0Previously the above would error out with an
-EPERM, as the mount was protected bymount_ns(), but the patch in question switched tokern_mount_data()which doesn't do this necessary permission check. So add it explicitly tomq_internal_mount().Fixes 36735a6a2b5e ("mqueue: switch to on-demand creation of internal mount")Reported-by Felix Abecassis <[email protected]>Signed-off-by Aleksa Sarai <[email protected]>--- ipc/mqueue.c | 6 ++++++ 1 file changed, 6 insertions(+)ipc/mqueue.c +6
-0@@ -353,6 +353,12 @@ static struct vfsmount *mq_internal_mount(void) {struct ipc_namespace *ns = current->nsproxy->ipc_ns;struct vfsmount *m = ns->mq_mnt; +/* +* Match the semantics of mount_ns, to avoid unprivileged users from being +* able to mount mqueue from an IPC namespace they don't have ownership of. +*/ +if (!ns_capable (ns->user_ns,CAP_SYS_ADMIN )) +return ERR_PTR (-EPERM );if (m)return m; m =kern_mount_data (&mqueue_fs_type, ns);Signature · 1 line
2.16.2
A message that is not in the archive (sent off-list or not yet received): 87k1u3ti9e.fsf-aS9lmoZGLiVWk0Htik3J/[email protected]
PATCH 1/2 fs: Extend mount_ns with support for a fast namespace to vfsmount function
If this function is present use it to lookup up the vfsmount except when performaning internal kernel mounts. When performing internal kernel mounts don't look through the list of superblocks just create a new one.
After a quick survey it appears all callers of mount_ns are candidates for this optimization. So extending the generic helper appears like the right thing.
The motivation for this change is that this optimization was performed recently on mqueuefs and a permission check was dropped and sb->s_user_ns was set incorrectly.
To enable fixing mqueuefs this logic was extracted from mqueuefs and added to mount_ns which gets the permission check correct and set sb->s_user_ns properly.
--- fs/nfsd/nfsctl.c | 3 ++- fs/proc/root.c | 3 ++- fs/super.c | 18 +++++++++++++++--- include/linux/fs.h | 1 + net/sunrpc/rpc_pipe.c | 2 +- 5 files changed, 21 insertions(+), 6 deletions(-)
fs/nfsd/nfsctl.c +2 -1
@@ -1182,7 +1182,8 @@ static struct dentry *nfsd_mount(struct file_system_type *fs_type, int flags, const char *dev_name, void *data) { struct net *net = current->nsproxy->net_ns; -return mount_ns (fs_type, flags, data, net, net->user_ns, nfsd_fill_super); +return mount_ns (fs_type, flags, data, net, net->user_ns, +NULL , nfsd_fill_super); }static void nfsd_umount (struct super_block *sb)
fs/proc/root.c +2 -1
@@ -98,7 +98,8 @@ static struct dentry *proc_mount(struct file_system_type *fs_type, ns =task_active_pid_ns (current); } -return mount_ns (fs_type, flags, data, ns, ns->user_ns, proc_fill_super); +return mount_ns (fs_type, flags, data, ns, ns->user_ns, +NULL , proc_fill_super); }static void proc_kill_sb (struct super_block *sb)
fs/super.c +15 -3
@@ -1016,18 +1016,30 @@ static int ns_set_super(struct super_block *sb, void *data)struct dentry *mount_ns (struct file_system_type *fs_type,int flags,void *data,void *ns,struct user_namespace *user_ns, +struct vfsmount *(*ns_to_mnt)(void *ns),int (*fill_super)(struct super_block *,void *,int )) {struct super_block *sb; - +int (*test_super)(struct super_block *,void *) = ns_test_super;/* Don't allow mounting unless the caller has CAP_SYS_ADMIN * over the namespace. */ if (!(flags &SB_KERNMOUNT ) && !ns_capable (user_ns,CAP_SYS_ADMIN ))return ERR_PTR (-EPERM ); - sb =sget_userns (fs_type, ns_test_super, ns_set_super, flags, - user_ns, ns); +if (ns_to_mnt) { + test_super =NULL ; +if (!(flags &SB_KERNMOUNT )) { +struct vfsmount *m =ns_to_mnt (ns); +if (IS_ERR (m)) +return ERR_CAST (m); +atomic_inc (&m->mnt_sb->s_active); +down_write (&m->mnt_sb->s_umount); +return dget (m->mnt_root); + } + } + + sb =sget_userns (fs_type, test_super, ns_set_super, flags, user_ns, ns);if (IS_ERR (sb))return ERR_CAST (sb);
include/linux/fs.h +1 -0
@@ -2091,6 +2091,7 @@ struct file_system_type {extern struct dentry *mount_ns (struct file_system_type *fs_type,int flags,void *data,void *ns,struct user_namespace *user_ns, +struct vfsmount *(*ns_to_mnt)(void *ns),int (*fill_super)(struct super_block *,void *,int ));#ifdef CONFIG_BLOCK extern struct dentry *mount_bdev(struct file_system_type *fs_type,
net/sunrpc/rpc_pipe.c +1 -1
@@ -1448,7 +1448,7 @@ rpc_mount(struct file_system_type *fs_type, int flags, const char *dev_name, void *data) { struct net *net = current->nsproxy->net_ns; -return mount_ns (fs_type, flags, data, net, net->user_ns, rpc_fill_super); +return mount_ns (fs_type, flags, data, net, net->user_ns,NULL , rpc_fill_super); }static void rpc_kill_sb (struct super_block *sb)
Signature · 1 line
2.14.1
PATCH 1/2 fs: Extend mount_ns with support for a fast namespace to vfsmount function
In reply to Eric W. BiedermanIf this function is present use it to lookup up the vfsmount except when performaning internal kernel mounts. When performing internal kernel mounts don't look through the list of superblocks just create a new one.
After a quick survey it appears all callers of mount_ns are candidates for this optimization. So extending the generic helper appears like the right thing.
The motivation for this change is that this optimization was performed recently on mqueuefs and a permission check was dropped and sb->s_user_ns was set incorrectly.
To enable fixing mqueuefs this logic was extracted from mqueuefs and added to mount_ns which gets the permission check correct and set sb->s_user_ns properly.
--- fs/nfsd/nfsctl.c | 3 ++- fs/proc/root.c | 3 ++- fs/super.c | 18 +++++++++++++++--- include/linux/fs.h | 1 + net/sunrpc/rpc_pipe.c | 2 +- 5 files changed, 21 insertions(+), 6 deletions(-)
fs/nfsd/nfsctl.c +2 -1
@@ -1182,7 +1182,8 @@ static struct dentry *nfsd_mount(struct file_system_type *fs_type, int flags, const char *dev_name, void *data) { struct net *net = current->nsproxy->net_ns; -return mount_ns (fs_type, flags, data, net, net->user_ns, nfsd_fill_super); +return mount_ns (fs_type, flags, data, net, net->user_ns, +NULL , nfsd_fill_super); }static void nfsd_umount (struct super_block *sb)
fs/proc/root.c +2 -1
@@ -98,7 +98,8 @@ static struct dentry *proc_mount(struct file_system_type *fs_type, ns =task_active_pid_ns (current); } -return mount_ns (fs_type, flags, data, ns, ns->user_ns, proc_fill_super); +return mount_ns (fs_type, flags, data, ns, ns->user_ns, +NULL , proc_fill_super); }static void proc_kill_sb (struct super_block *sb)
fs/super.c +15 -3
@@ -1016,18 +1016,30 @@ static int ns_set_super(struct super_block *sb, void *data)struct dentry *mount_ns (struct file_system_type *fs_type,int flags,void *data,void *ns,struct user_namespace *user_ns, +struct vfsmount *(*ns_to_mnt)(void *ns),int (*fill_super)(struct super_block *,void *,int )) {struct super_block *sb; - +int (*test_super)(struct super_block *,void *) = ns_test_super;/* Don't allow mounting unless the caller has CAP_SYS_ADMIN * over the namespace. */ if (!(flags &SB_KERNMOUNT ) && !ns_capable (user_ns,CAP_SYS_ADMIN ))return ERR_PTR (-EPERM ); - sb =sget_userns (fs_type, ns_test_super, ns_set_super, flags, - user_ns, ns); +if (ns_to_mnt) { + test_super =NULL ; +if (!(flags &SB_KERNMOUNT )) { +struct vfsmount *m =ns_to_mnt (ns); +if (IS_ERR (m)) +return ERR_CAST (m); +atomic_inc (&m->mnt_sb->s_active); +down_write (&m->mnt_sb->s_umount); +return dget (m->mnt_root); + } + } + + sb =sget_userns (fs_type, test_super, ns_set_super, flags, user_ns, ns);if (IS_ERR (sb))return ERR_CAST (sb);
include/linux/fs.h +1 -0
@@ -2091,6 +2091,7 @@ struct file_system_type {extern struct dentry *mount_ns (struct file_system_type *fs_type,int flags,void *data,void *ns,struct user_namespace *user_ns, +struct vfsmount *(*ns_to_mnt)(void *ns),int (*fill_super)(struct super_block *,void *,int ));#ifdef CONFIG_BLOCK extern struct dentry *mount_bdev(struct file_system_type *fs_type,
net/sunrpc/rpc_pipe.c +1 -1
@@ -1448,7 +1448,7 @@ rpc_mount(struct file_system_type *fs_type, int flags, const char *dev_name, void *data) { struct net *net = current->nsproxy->net_ns; -return mount_ns (fs_type, flags, data, net, net->user_ns, rpc_fill_super); +return mount_ns (fs_type, flags, data, net, net->user_ns,NULL , rpc_fill_super); }static void rpc_kill_sb (struct super_block *sb)
Signature · 1 line
2.14.1
A message that is not in the archive (sent off-list or not yet received): 87fu4qo4ff.fsf_-_-aS9lmoZGLiVWk0Htik3J/[email protected]
PATCH 2/2 mqueuefs: Fix the permissions and permission checks when mounting mqueuefs
Quoted 14 lines · Aleksa Sarai
Felix reported weird behaviour on 4.16.0-rc6 with regards to mqueue[1], which was introduced by
36735a6a2b5e("mqueue: switch to on-demand creation of internal mount").Basically, the reproducer boils down to being able to mount mqueue if you create a new user namespace, even if you don't unshare the IPC namespace.
Previously this was not possible, and you would get an
-EPERM. The mount is the *host* mqueue mount, which is being cached and just returned frommqueue_mount(). To be honest, I'm not sure if this is safe or not (or if it was intentional -- since I'm not familiar with mqueue).
Quoted 7 lines
To me it looks like there is a missing permission check. I've included a patch below that I've compile-tested, and should block the above case. Can someone please tell me if I'm missing something? Is this actually safe?
After examination of the code it might be safe by chance but it is definitely wrong. The missing permission checks are needed in the general case, and sb->s_user_ns needs to be set to ns->user_ns to give root in the user namespace the appropriate permissions over the filesystem.
---
Unless there are objections I will push these fixes to Linus in a day or so.
ipc/mqueue.c | 21 +++++++-------------- 1 file changed, 7 insertions(+), 14 deletions(-)
ipc/mqueue.c +7 -14
@@ -325,9 +325,8 @@ static struct inode *mqueue_get_inode(struct super_block *sb,static int mqueue_fill_super (struct super_block *sb,void *data,int silent) {struct inode *inode; -struct ipc_namespace *ns = data; +struct ipc_namespace *ns = sb->s_fs_info; - sb->s_fs_info = ns; sb->s_iflags |=SB_I_NOEXEC |SB_I_NODEV ; sb->s_blocksize =PAGE_SIZE ; sb->s_blocksize_bits =PAGE_SHIFT ;
@@ -349,9 +348,9 @@ static struct file_system_type mqueue_fs_type;* Return value is pinned only by reference in ->mq_mnt; it will * live until ipcns dies. Caller does not need to drop it. */ -static struct vfsmount *mq_internal_mount (void ) +static struct vfsmount *mq_internal_mount (void *nsp) { -struct ipc_namespace *ns = current->nsproxy->ipc_ns; +struct ipc_namespace *ns = nsp;struct vfsmount *m = ns->mq_mnt;if (m)return m;
@@ -373,15 +372,9 @@ static struct dentry *mqueue_mount(struct file_system_type *fs_type, int flags, const char *dev_name, void *data) { - struct vfsmount *m; -if (flags &SB_KERNMOUNT ) -return mount_nodev (fs_type, flags, data, mqueue_fill_super); - m =mq_internal_mount (); -if (IS_ERR (m)) -return ERR_CAST (m); -atomic_inc (&m->mnt_sb->s_active); -down_write (&m->mnt_sb->s_umount); -return dget (m->mnt_root); + struct ipc_namespace *ns = current->nsproxy->ipc_ns; +return mount_ns (fs_type, flags, data, ns, ns->user_ns, + mq_internal_mount, mqueue_fill_super); }static void init_once (void *foo)
@@ -771,7 +764,7 @@ static int prepare_open(struct dentry *dentry, int oflag, int ro,static int do_mq_open (const char __user *u_name,int oflag,umode_t mode,struct mq_attr *attr) { -struct vfsmount *mnt =mq_internal_mount (); +struct vfsmount *mnt =mq_internal_mount (current->nsproxy->ipc_ns);struct dentry *root;struct filename *name;struct path path;
Signature · 1 line
2.14.1
Re: [PATCH 1/2] fs: Extend mount_ns with support for a fast namespace to vfsmount function
In reply to a message not in the archiveQuoted 15 lines · Eric W. Biederman
struct dentry *mount_ns(struct
file_system_type*fs_type,fs/super.c +2
-2int flags, void *data, void *ns, struct user_namespace *user_ns, + struct vfsmount *(*ns_to_mnt)(void *ns), int (*fill_super)(struct super_block *, void *, int)) { struct super_block *sb; - +int (*test_super)(struct super_block *,void *) = ns_test_super;/* Don't allow mounting unless the caller has CAP_SYS_ADMIN * over the namespace. */ if (!(flags &SB_KERNMOUNT ) && !ns_capable (user_ns,CAP_SYS_ADMIN ))return ERR_PTR (-EPERM ); - sb = sget_userns(fs_type, ns_test_super, ns_set_super, flags,
fs/super.c static int ns_set_super(struct super_block *sb, void *data) · line 1038
Quoted 10 lines
fs/super.c +9
-1- user_ns, ns); +if (ns_to_mnt) { + test_super =NULL ; +if (!(flags &SB_KERNMOUNT )) { +struct vfsmount *m =ns_to_mnt (ns); +if (IS_ERR (m)) +return ERR_CAST (m); +atomic_inc (&m->mnt_sb->s_active); +down_write (&m->mnt_sb->s_umount); +return dget (m->mnt_root);
This is completely wrong. Look:
* SB_KERNMOUNT and !SB_KERNMOUNT cases are almost entirely isolated;
completely so once that ns_to_mnt becomes unconditionally non-NULL.
* in !SB_KERNMOUNT passing ns_to_mnt() is pointless - you might as
well pass existing vfsmount (or ERR_PTR()) and use _that_. fill_super() is not used at all in that case.
* is SB_KERNMOUNT ns_to_mnt serves only as a flag, eventually
constant true.
So let's split it in two helpers and give them sane arguments.
PATCH 2/2 mqueuefs: Fix the permissions and permission checks when mounting mqueuefs
In reply to Eric W. BiedermanQuoted 14 lines · Aleksa Sarai
Felix reported weird behaviour on 4.16.0-rc6 with regards to mqueue[1], which was introduced by
36735a6a2b5e("mqueue: switch to on-demand creation of internal mount").Basically, the reproducer boils down to being able to mount mqueue if you create a new user namespace, even if you don't unshare the IPC namespace.
Previously this was not possible, and you would get an
-EPERM. The mount is the *host* mqueue mount, which is being cached and just returned frommqueue_mount(). To be honest, I'm not sure if this is safe or not (or if it was intentional -- since I'm not familiar with mqueue).
Quoted 7 lines
To me it looks like there is a missing permission check. I've included a patch below that I've compile-tested, and should block the above case. Can someone please tell me if I'm missing something? Is this actually safe?
After examination of the code it might be safe by chance but it is definitely wrong. The missing permission checks are needed in the general case, and sb->s_user_ns needs to be set to ns->user_ns to give root in the user namespace the appropriate permissions over the filesystem.
---
Unless there are objections I will push these fixes to Linus in a day or so.
ipc/mqueue.c | 21 +++++++-------------- 1 file changed, 7 insertions(+), 14 deletions(-)
ipc/mqueue.c +7 -14
@@ -325,9 +325,8 @@ static struct inode *mqueue_get_inode(struct super_block *sb,static int mqueue_fill_super (struct super_block *sb,void *data,int silent) {struct inode *inode; -struct ipc_namespace *ns = data; +struct ipc_namespace *ns = sb->s_fs_info; - sb->s_fs_info = ns; sb->s_iflags |=SB_I_NOEXEC |SB_I_NODEV ; sb->s_blocksize =PAGE_SIZE ; sb->s_blocksize_bits =PAGE_SHIFT ;
@@ -349,9 +348,9 @@ static struct file_system_type mqueue_fs_type;* Return value is pinned only by reference in ->mq_mnt; it will * live until ipcns dies. Caller does not need to drop it. */ -static struct vfsmount *mq_internal_mount (void ) +static struct vfsmount *mq_internal_mount (void *nsp) { -struct ipc_namespace *ns = current->nsproxy->ipc_ns; +struct ipc_namespace *ns = nsp;struct vfsmount *m = ns->mq_mnt;if (m)return m;
@@ -373,15 +372,9 @@ static struct dentry *mqueue_mount(struct file_system_type *fs_type, int flags, const char *dev_name, void *data) { - struct vfsmount *m; -if (flags &SB_KERNMOUNT ) -return mount_nodev (fs_type, flags, data, mqueue_fill_super); - m =mq_internal_mount (); -if (IS_ERR (m)) -return ERR_CAST (m); -atomic_inc (&m->mnt_sb->s_active); -down_write (&m->mnt_sb->s_umount); -return dget (m->mnt_root); + struct ipc_namespace *ns = current->nsproxy->ipc_ns; +return mount_ns (fs_type, flags, data, ns, ns->user_ns, + mq_internal_mount, mqueue_fill_super); }static void init_once (void *foo)
@@ -771,7 +764,7 @@ static int prepare_open(struct dentry *dentry, int oflag, int ro,static int do_mq_open (const char __user *u_name,int oflag,umode_t mode,struct mq_attr *attr) { -struct vfsmount *mnt =mq_internal_mount (); +struct vfsmount *mnt =mq_internal_mount (current->nsproxy->ipc_ns);struct dentry *root;struct filename *name;struct path path;
Signature · 1 line
2.14.1
Al Viro
maintainerRe: [PATCH 1/2] fs: Extend mount_ns with support for a fast namespace to vfsmount function
In reply to Eric W. BiedermanQuoted 15 lines · Eric W. Biederman
struct dentry *mount_ns(struct
file_system_type*fs_type,fs/super.c +2
-2int flags, void *data, void *ns, struct user_namespace *user_ns, + struct vfsmount *(*ns_to_mnt)(void *ns), int (*fill_super)(struct super_block *, void *, int)) { struct super_block *sb; - +int (*test_super)(struct super_block *,void *) = ns_test_super;/* Don't allow mounting unless the caller has CAP_SYS_ADMIN * over the namespace. */ if (!(flags &SB_KERNMOUNT ) && !ns_capable (user_ns,CAP_SYS_ADMIN ))return ERR_PTR (-EPERM ); - sb = sget_userns(fs_type, ns_test_super, ns_set_super, flags,
fs/super.c static int ns_set_super(struct super_block *sb, void *data) · line 1038
Quoted 10 lines
fs/super.c +9
-1- user_ns, ns); +if (ns_to_mnt) { + test_super =NULL ; +if (!(flags &SB_KERNMOUNT )) { +struct vfsmount *m =ns_to_mnt (ns); +if (IS_ERR (m)) +return ERR_CAST (m); +atomic_inc (&m->mnt_sb->s_active); +down_write (&m->mnt_sb->s_umount); +return dget (m->mnt_root);
This is completely wrong. Look:
* SB_KERNMOUNT and !SB_KERNMOUNT cases are almost entirely isolated;
completely so once that ns_to_mnt becomes unconditionally non-NULL.
* in !SB_KERNMOUNT passing ns_to_mnt() is pointless - you might as
well pass existing vfsmount (or ERR_PTR()) and use _that_. fill_super() is not used at all in that case.
* is SB_KERNMOUNT ns_to_mnt serves only as a flag, eventually
constant true.
So let's split it in two helpers and give them sane arguments.
A message that is not in the archive (sent off-list or not yet received): 20180323231511.GK30522-3bDd1+5oDREiFSDQTTA3OLVCufUGDwFn@public.gmane.org
Re: [PATCH 1/2] fs: Extend mount_ns with support for a fast namespace to vfsmount function
Quoted 37 lines · Al Viro
Quoted 15 lines · Eric W. Biederman
struct dentry *mount_ns(struct
file_system_type*fs_type,fs/super.c +2
-2int flags, void *data, void *ns, struct user_namespace *user_ns, + struct vfsmount *(*ns_to_mnt)(void *ns), int (*fill_super)(struct super_block *, void *, int)) { struct super_block *sb; - +int (*test_super)(struct super_block *,void *) = ns_test_super;/* Don't allow mounting unless the caller has CAP_SYS_ADMIN * over the namespace. */ if (!(flags &SB_KERNMOUNT ) && !ns_capable (user_ns,CAP_SYS_ADMIN ))return ERR_PTR (-EPERM ); - sb = sget_userns(fs_type, ns_test_super, ns_set_super, flags,fs/super.c static int ns_set_super(struct super_block *sb, void *data) · line 1038
Quoted 10 lines
fs/super.c +9
-1- user_ns, ns); +if (ns_to_mnt) { + test_super =NULL ; +if (!(flags &SB_KERNMOUNT )) { +struct vfsmount *m =ns_to_mnt (ns); +if (IS_ERR (m)) +return ERR_CAST (m); +atomic_inc (&m->mnt_sb->s_active); +down_write (&m->mnt_sb->s_umount); +return dget (m->mnt_root);This is completely wrong. Look:
*SB_KERNMOUNTand !SB_KERNMOUNT cases are almost entirely isolated;
completely so once thatns_to_mntbecomes unconditionally non-NULL.
* in !SB_KERNMOUNT passingns_to_mnt()is pointless - you might as
well pass existing vfsmount (orERR_PTR()) and use _that_.fill_super()is not used at all in that case.
* isSB_KERNMOUNTns_to_mntserves only as a flag, eventually
constant true.
Quoted 1 line
So let's split it in two helpers and give them sane arguments.
Everything I look at with multiple helpers feels even worse to me. The above has the advantage it is the minimal change to fix the regression. So I am not worried about code correctness.
I keep wondering is the intention long term to fix sget so it has an efficient data structure for finding super blocks (like an rbtree) or if the intention is to deprecate sget entirely and just have everything call alloc_super, and be responsible for their own data structures for finding existing superblocks.
At this point since we are not in agreement on a proper fix I am going
to plan on just queueing up a revert. So that we don't ship 4.16 with
a regression in a permission check.
Eric
Re: [PATCH 1/2] fs: Extend mount_ns with support for a fast namespace to vfsmount function
In reply to Al ViroQuoted 37 lines · Al Viro
Quoted 15 lines · Eric W. Biederman
struct dentry *mount_ns(struct
file_system_type*fs_type,fs/super.c +2
-2int flags, void *data, void *ns, struct user_namespace *user_ns, + struct vfsmount *(*ns_to_mnt)(void *ns), int (*fill_super)(struct super_block *, void *, int)) { struct super_block *sb; - +int (*test_super)(struct super_block *,void *) = ns_test_super;/* Don't allow mounting unless the caller has CAP_SYS_ADMIN * over the namespace. */ if (!(flags &SB_KERNMOUNT ) && !ns_capable (user_ns,CAP_SYS_ADMIN ))return ERR_PTR (-EPERM ); - sb = sget_userns(fs_type, ns_test_super, ns_set_super, flags,fs/super.c static int ns_set_super(struct super_block *sb, void *data) · line 1038
Quoted 10 lines
fs/super.c +9
-1- user_ns, ns); +if (ns_to_mnt) { + test_super =NULL ; +if (!(flags &SB_KERNMOUNT )) { +struct vfsmount *m =ns_to_mnt (ns); +if (IS_ERR (m)) +return ERR_CAST (m); +atomic_inc (&m->mnt_sb->s_active); +down_write (&m->mnt_sb->s_umount); +return dget (m->mnt_root);This is completely wrong. Look:
*SB_KERNMOUNTand !SB_KERNMOUNT cases are almost entirely isolated;
completely so once thatns_to_mntbecomes unconditionally non-NULL.
* in !SB_KERNMOUNT passingns_to_mnt()is pointless - you might as
well pass existing vfsmount (orERR_PTR()) and use _that_.fill_super()is not used at all in that case.
* isSB_KERNMOUNTns_to_mntserves only as a flag, eventually
constant true.
Quoted 1 line
So let's split it in two helpers and give them sane arguments.
Everything I look at with multiple helpers feels even worse to me. The above has the advantage it is the minimal change to fix the regression. So I am not worried about code correctness.
I keep wondering is the intention long term to fix sget so it has an efficient data structure for finding super blocks (like an rbtree) or if the intention is to deprecate sget entirely and just have everything call alloc_super, and be responsible for their own data structures for finding existing superblocks.
At this point since we are not in agreement on a proper fix I am going
to plan on just queueing up a revert. So that we don't ship 4.16 with
a regression in a permission check.
Eric
A message that is not in the archive (sent off-list or not yet received): 87in9ljvvx.fsf-aS9lmoZGLiVWk0Htik3J/[email protected]
Re: [PATCH 1/2] fs: Extend mount_ns with support for a fast namespace to vfsmount function
Quoted 21 lines · Eric W. Biederman
Quoted 10 lines
This is completely wrong. Look:
*SB_KERNMOUNTand !SB_KERNMOUNT cases are almost entirely isolated;
completely so once thatns_to_mntbecomes unconditionally non-NULL.
* in !SB_KERNMOUNT passingns_to_mnt()is pointless - you might as
well pass existing vfsmount (orERR_PTR()) and use _that_.fill_super()is not used at all in that case.
* isSB_KERNMOUNTns_to_mntserves only as a flag, eventually
constant true.So let's split it in two helpers and give them sane arguments.
Everything I look at with multiple helpers feels even worse to me. The above has the advantage it is the minimal change to fix the regression. So I am not worried about code correctness.
I keep wondering is the intention long term to fix sget so it has an efficient data structure for finding super blocks (like an rbtree) or if the intention is to deprecate sget entirely and just have everything call
alloc_super, and be responsible for their own data structures for finding existing superblocks.
Quoted 3 lines
At this point since we are not in agreement on a proper fix I am going
to plan on just queueing up a revert. So that we don't ship 4.16 with
a regression in a permission check.
Permission check is trivial to put back in; I'll do that.
FWIW, I don't believe that sget_userns() is a good place for any kind of universal permission checks. It's a library helper, not a place everything must come through when mounting something. So's mount_ns(), etc.
BTW, will you be at LSF? I would suggest discussing the architectural issues there - they are directly related to fsmount() proposals...
Al Viro
maintainerRe: [PATCH 1/2] fs: Extend mount_ns with support for a fast namespace to vfsmount function
In reply to Eric W. BiedermanQuoted 21 lines · Eric W. Biederman
Quoted 10 lines
This is completely wrong. Look:
*SB_KERNMOUNTand !SB_KERNMOUNT cases are almost entirely isolated;
completely so once thatns_to_mntbecomes unconditionally non-NULL.
* in !SB_KERNMOUNT passingns_to_mnt()is pointless - you might as
well pass existing vfsmount (orERR_PTR()) and use _that_.fill_super()is not used at all in that case.
* isSB_KERNMOUNTns_to_mntserves only as a flag, eventually
constant true.So let's split it in two helpers and give them sane arguments.
Everything I look at with multiple helpers feels even worse to me. The above has the advantage it is the minimal change to fix the regression. So I am not worried about code correctness.
I keep wondering is the intention long term to fix sget so it has an efficient data structure for finding super blocks (like an rbtree) or if the intention is to deprecate sget entirely and just have everything call
alloc_super, and be responsible for their own data structures for finding existing superblocks.
Quoted 3 lines
At this point since we are not in agreement on a proper fix I am going
to plan on just queueing up a revert. So that we don't ship 4.16 with
a regression in a permission check.
Permission check is trivial to put back in; I'll do that.
FWIW, I don't believe that sget_userns() is a good place for any kind of universal permission checks. It's a library helper, not a place everything must come through when mounting something. So's mount_ns(), etc.
BTW, will you be at LSF? I would suggest discussing the architectural issues there - they are directly related to fsmount() proposals...
[GIT PULL] Revert "mqueue: switch to on-demand creation of internal mount"
Linus,
Please pull the for-linus branch from the git tree:
git://git.kernel.org/pub/scm/linux/kernel/git/ebiederm/user-namespace.git for-linus
HEAD: cfb2f6f6e0ba11ea7b263d6b69c170c4b32ac0ea Revert "mqueue: switch to on-demand creation of internal mount"
This fixes a regression that came in the merge window for v4.16. The problem is that the permissions for mounting and using the mqueuefs filesystem are broken. The necessary permission check is missing letting people who should not be able to mount mqueuefs mount mqueuefs. The field sb->s_user_ns is set incorrectly not allowing the mounter of mqueuefs to remount and otherwise have proper control over the filesystem.
Al Viro and I see the path to the necessary fixes differently and I am not even certain at this point he actually sees all of the necessary fixes. Given a couple weeks we can probably work something out but I don't see the review being resolved in time for the final v4.16. I don't want v4.16 shipping with a nasty regression. So unfortunately I am sending a revert.
Eric
From: "Eric W. Biederman" <ebiederm-aS9lmoZGLiVWk0Htik3J/[email protected]>
Date: Sat, 24 Mar 2018 11:28:14 -0500
Subject: [PATCH] Revert "mqueue: switch to on-demand creation of internal mount"
This reverts commit 36735a6a2b5e042db1af956ce4bcc13f3ff99e21.
Quoted 15 lines · Aleksa Sarai
[REGRESSION v4.16-rc6] [PATCH] mqueue: forbid unprivileged user access to internal mount
Felix reported weird behaviour on 4.16.0-rc6 with regards to mqueue[1], which was introduced by
36735a6a2b5e("mqueue: switch to on-demand creation of internal mount").Basically, the reproducer boils down to being able to mount mqueue if you create a new user namespace, even if you don't unshare the IPC namespace.
Previously this was not possible, and you would get an
-EPERM. The mount is the *host* mqueue mount, which is being cached and just returned frommqueue_mount(). To be honest, I'm not sure if this is safe or not (or if it was intentional -- since I'm not familiar with mqueue).
Quoted 6 lines
To me it looks like there is a missing permission check. I've included a patch below that I've compile-tested, and should block the above case. Can someone please tell me if I'm missing something? Is this actually safe?
The issue is a lot deeper than a missing permission check. sb->s_user_ns was is improperly set as well. So in addition to the filesystem being mounted when it should not be mounted, so things are not allow that should be.
We are practically to the release of 4.16 and there is no agreement between Al Viro and myself on what the code should looks like to fix things properly. So revert the code to what it was before so that we can take our time and discuss this properly.
--- ipc/mqueue.c | 74 ++++++++++++++++-------------------------------------------- 1 file changed, 19 insertions(+), 55 deletions(-)
ipc/mqueue.c +19 -55
@@ -325,9 +325,8 @@ static struct inode *mqueue_get_inode(struct super_block *sb,static int mqueue_fill_super (struct super_block *sb,void *data,int silent) {struct inode *inode; -struct ipc_namespace *ns = data; +struct ipc_namespace *ns = sb->s_fs_info; - sb->s_fs_info = ns; sb->s_iflags |=SB_I_NOEXEC |SB_I_NODEV ; sb->s_blocksize =PAGE_SIZE ; sb->s_blocksize_bits =PAGE_SHIFT ;
@@ -344,44 +343,18 @@ static int mqueue_fill_super(struct super_block *sb, void *data, int silent)return 0 ; } -static struct file_system_type mqueue_fs_type; -/* -* Return value is pinned only by reference in ->mq_mnt; it will -* live until ipcns dies. Caller does not need to drop it. -*/ -static struct vfsmount *mq_internal_mount (void ) -{ -struct ipc_namespace *ns = current->nsproxy->ipc_ns; -struct vfsmount *m = ns->mq_mnt; -if (m) -return m; - m =kern_mount_data (&mqueue_fs_type, ns); -spin_lock (&mq_lock); -if (unlikely (ns->mq_mnt)) { -spin_unlock (&mq_lock); -if (!IS_ERR (m)) -kern_unmount (m); -return ns->mq_mnt; - } -if (!IS_ERR (m)) - ns->mq_mnt = m; -spin_unlock (&mq_lock); -return m; -} -static struct dentry *mqueue_mount (struct file_system_type *fs_type,int flags,const char *dev_name,void *data) { -struct vfsmount *m; -if (flags &SB_KERNMOUNT ) -return mount_nodev (fs_type, flags, data, mqueue_fill_super); - m =mq_internal_mount (); -if (IS_ERR (m)) -return ERR_CAST (m); -atomic_inc (&m->mnt_sb->s_active); -down_write (&m->mnt_sb->s_umount); -return dget (m->mnt_root); +struct ipc_namespace *ns; +if (flags &SB_KERNMOUNT ) { + ns = data; + data =NULL ; + }else { + ns = current->nsproxy->ipc_ns; + } +return mount_ns (fs_type, flags, data, ns, ns->user_ns, mqueue_fill_super); }static void init_once (void *foo)
@@ -771,16 +744,13 @@ static int prepare_open(struct dentry *dentry, int oflag, int ro,static int do_mq_open (const char __user *u_name,int oflag,umode_t mode,struct mq_attr *attr) { -struct vfsmount *mnt =mq_internal_mount (); -struct dentry *root; +struct vfsmount *mnt = current->nsproxy->ipc_ns->mq_mnt; +struct dentry *root = mnt->mnt_root;struct filename *name;struct path path;int fd, error;int ro; -if (IS_ERR (mnt)) -return PTR_ERR (mnt); -audit_mq_open (oflag, mode, attr);if (IS_ERR (name =getname (u_name)))
@@ -791,7 +761,6 @@ static int do_mq_open(const char __user *u_name, int oflag, umode_t mode, goto out_putname; ro =mnt_want_write (mnt);/* we'll drop it in any case */ - root = mnt->mnt_root;inode_lock (d_inode (root)); path.dentry =lookup_one_len (name->name, root,strlen (name->name));if (IS_ERR (path.dentry)) {
@@ -840,9 +809,6 @@ SYSCALL_DEFINE1(mq_unlink, const char __user *, u_name)struct ipc_namespace *ipc_ns = current->nsproxy->ipc_ns;struct vfsmount *mnt = ipc_ns->mq_mnt; -if (!mnt) -return -ENOENT ; - name =getname (u_name);if (IS_ERR (name))return PTR_ERR (name);
@@ -1569,26 +1535,28 @@ int mq_init_ns(struct ipc_namespace *ns) ns->mq_msgsize_max =DFLT_MSGSIZEMAX ; ns->mq_msg_default =DFLT_MSG ; ns->mq_msgsize_default =DFLT_MSGSIZE ; - ns->mq_mnt =NULL ; + ns->mq_mnt =kern_mount_data (&mqueue_fs_type, ns); +if (IS_ERR (ns->mq_mnt)) { +int err =PTR_ERR (ns->mq_mnt); + ns->mq_mnt =NULL ; +return err; + }return 0 ; }void mq_clear_sbinfo (struct ipc_namespace *ns) { -if (ns->mq_mnt) - ns->mq_mnt->mnt_sb->s_fs_info =NULL ; + ns->mq_mnt->mnt_sb->s_fs_info =NULL ; }void mq_put_mnt (struct ipc_namespace *ns) { -if (ns->mq_mnt) -kern_unmount (ns->mq_mnt); +kern_unmount (ns->mq_mnt); }static int __init init_mqueue_fs (void ) { -struct vfsmount *m;int error; mqueue_inode_cachep = kmem_cache_create("mqueue_inode_cache",
@@ -1610,10 +1578,6 @@ static int __init init_mqueue_fs(void)if (error) goto out_filesystem; - m =kern_mount_data (&mqueue_fs_type, &init_ipc_ns); -if (IS_ERR (m)) - goto out_filesystem; - init_ipc_ns.mq_mnt = m;return 0 ; out_filesystem:
Signature · 1 line
2.14.1