* Re: [RFC v5 1/1] ns: add binfmt_misc to the user namespace
From: Kirill Tkhai @ 2018-10-09 16:15 UTC (permalink / raw)
To: Laurent Vivier, linux-kernel@vger.kernel.org
Cc: Eric Biederman, Dmitry Safonov, linux-api@vger.kernel.org,
James Bottomley, Alexander Viro, linux-fsdevel@vger.kernel.org,
Andrei Vagin (C), containers@lists.linux-foundation.org,
Jann Horn
In-Reply-To: <20181009103752.21482-2-laurent@vivier.eu>
On 09.10.2018 13:37, Laurent Vivier wrote:
> This patch allows to have a different binfmt_misc configuration
> for each new user namespace. By default, the binfmt_misc configuration
> is the one of the previous level, but if the binfmt_misc filesystem is
> mounted in the new namespace a new empty binfmt instance is created and
> used in this namespace.
>
> For instance, using "unshare" we can start a chroot of an another
> architecture and configure the binfmt_misc interpreter without being root
> to run the binaries in this chroot.
>
> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
> ---
> fs/binfmt_misc.c | 106 ++++++++++++++++++++++++---------
> include/linux/user_namespace.h | 13 ++++
> kernel/user.c | 13 ++++
> kernel/user_namespace.c | 3 +
> 4 files changed, 107 insertions(+), 28 deletions(-)
>
> diff --git a/fs/binfmt_misc.c b/fs/binfmt_misc.c
> index aa4a7a23ff99..1e0029d097d9 100644
> --- a/fs/binfmt_misc.c
> +++ b/fs/binfmt_misc.c
> @@ -38,9 +38,6 @@ enum {
> VERBOSE_STATUS = 1 /* make it zero to save 400 bytes kernel memory */
> };
>
> -static LIST_HEAD(entries);
> -static int enabled = 1;
> -
> enum {Enabled, Magic};
> #define MISC_FMT_PRESERVE_ARGV0 (1 << 31)
> #define MISC_FMT_OPEN_BINARY (1 << 30)
> @@ -60,10 +57,7 @@ typedef struct {
> struct file *interp_file;
> } Node;
>
> -static DEFINE_RWLOCK(entries_lock);
> static struct file_system_type bm_fs_type;
> -static struct vfsmount *bm_mnt;
> -static int entry_count;
>
> /*
> * Max length of the register string. Determined by:
> @@ -80,18 +74,32 @@ static int entry_count;
> */
> #define MAX_REGISTER_LENGTH 1920
>
> +static struct binfmt_namespace *binfmt_ns(struct user_namespace *ns)
> +{
> + struct binfmt_namespace *b_ns;
> +
> + while (ns) {
> + b_ns = READ_ONCE(ns->binfmt_ns);
> + if (b_ns)
> + return b_ns;
> + ns = ns->parent;
> + }
> + WARN_ON_ONCE(1);
> + return NULL;
> +}
> +
> /*
> * Check if we support the binfmt
> * if we do, return the node, else NULL
> * locking is done in load_misc_binary
> */
> -static Node *check_file(struct linux_binprm *bprm)
> +static Node *check_file(struct binfmt_namespace *ns, struct linux_binprm *bprm)
> {
> char *p = strrchr(bprm->interp, '.');
> struct list_head *l;
>
> /* Walk all the registered handlers. */
> - list_for_each(l, &entries) {
> + list_for_each(l, &ns->entries) {
> Node *e = list_entry(l, Node, list);
> char *s;
> int j;
> @@ -133,17 +141,18 @@ static int load_misc_binary(struct linux_binprm *bprm)
> struct file *interp_file = NULL;
> int retval;
> int fd_binary = -1;
> + struct binfmt_namespace *ns = binfmt_ns(current_user_ns());
>
> retval = -ENOEXEC;
> - if (!enabled)
> + if (!ns->enabled)
> return retval;
>
> /* to keep locking time low, we copy the interpreter string */
> - read_lock(&entries_lock);
> - fmt = check_file(bprm);
> + read_lock(&ns->entries_lock);
> + fmt = check_file(ns, bprm);
> if (fmt)
> dget(fmt->dentry);
> - read_unlock(&entries_lock);
> + read_unlock(&ns->entries_lock);
> if (!fmt)
> return retval;
>
> @@ -609,19 +618,19 @@ static void bm_evict_inode(struct inode *inode)
> kfree(e);
> }
>
> -static void kill_node(Node *e)
> +static void kill_node(struct binfmt_namespace *ns, Node *e)
> {
> struct dentry *dentry;
>
> - write_lock(&entries_lock);
> + write_lock(&ns->entries_lock);
> list_del_init(&e->list);
> - write_unlock(&entries_lock);
> + write_unlock(&ns->entries_lock);
>
> dentry = e->dentry;
> drop_nlink(d_inode(dentry));
> d_drop(dentry);
> dput(dentry);
> - simple_release_fs(&bm_mnt, &entry_count);
> + simple_release_fs(&ns->bm_mnt, &ns->entry_count);
> }
>
> /* /<entry> */
> @@ -651,6 +660,9 @@ static ssize_t bm_entry_write(struct file *file, const char __user *buffer,
> struct dentry *root;
> Node *e = file_inode(file)->i_private;
> int res = parse_command(buffer, count);
> + struct binfmt_namespace *ns;
> +
> + ns = binfmt_ns(file->f_path.dentry->d_sb->s_user_ns);
>
> switch (res) {
> case 1:
> @@ -667,7 +679,7 @@ static ssize_t bm_entry_write(struct file *file, const char __user *buffer,
> inode_lock(d_inode(root));
>
> if (!list_empty(&e->list))
> - kill_node(e);
> + kill_node(ns, e);
>
> inode_unlock(d_inode(root));
> break;
> @@ -693,6 +705,7 @@ static ssize_t bm_register_write(struct file *file, const char __user *buffer,
> struct inode *inode;
> struct super_block *sb = file_inode(file)->i_sb;
> struct dentry *root = sb->s_root, *dentry;
> + struct binfmt_namespace *ns;
> int err = 0;
>
> e = create_entry(buffer, count);
> @@ -716,7 +729,9 @@ static ssize_t bm_register_write(struct file *file, const char __user *buffer,
> if (!inode)
> goto out2;
>
> - err = simple_pin_fs(&bm_fs_type, &bm_mnt, &entry_count);
> + ns = binfmt_ns(file->f_path.dentry->d_sb->s_user_ns);
> + err = simple_pin_fs(&bm_fs_type, &ns->bm_mnt,
> + &ns->entry_count);
> if (err) {
> iput(inode);
> inode = NULL;
> @@ -725,12 +740,16 @@ static ssize_t bm_register_write(struct file *file, const char __user *buffer,
>
> if (e->flags & MISC_FMT_OPEN_FILE) {
> struct file *f;
> + const struct cred *old_cred;
>
> + old_cred = override_creds(file->f_cred);
> f = open_exec(e->interpreter);
> + revert_creds(old_cred);
> if (IS_ERR(f)) {
> err = PTR_ERR(f);
> pr_notice("register: failed to install interpreter file %s\n", e->interpreter);
> - simple_release_fs(&bm_mnt, &entry_count);
> + simple_release_fs(&ns->bm_mnt,
> + &ns->entry_count);
> iput(inode);
> inode = NULL;
> goto out2;
> @@ -743,9 +762,9 @@ static ssize_t bm_register_write(struct file *file, const char __user *buffer,
> inode->i_fop = &bm_entry_operations;
>
> d_instantiate(dentry, inode);
> - write_lock(&entries_lock);
> - list_add(&e->list, &entries);
> - write_unlock(&entries_lock);
> + write_lock(&ns->entries_lock);
> + list_add(&e->list, &ns->entries);
> + write_unlock(&ns->entries_lock);
>
> err = 0;
> out2:
> @@ -770,7 +789,9 @@ static const struct file_operations bm_register_operations = {
> static ssize_t
> bm_status_read(struct file *file, char __user *buf, size_t nbytes, loff_t *ppos)
> {
> - char *s = enabled ? "enabled\n" : "disabled\n";
> + struct binfmt_namespace *ns =
> + binfmt_ns(file->f_path.dentry->d_sb->s_user_ns);
> + char *s = ns->enabled ? "enabled\n" : "disabled\n";
>
> return simple_read_from_buffer(buf, nbytes, ppos, s, strlen(s));
> }
> @@ -778,25 +799,28 @@ bm_status_read(struct file *file, char __user *buf, size_t nbytes, loff_t *ppos)
> static ssize_t bm_status_write(struct file *file, const char __user *buffer,
> size_t count, loff_t *ppos)
> {
> + struct binfmt_namespace *ns;
> int res = parse_command(buffer, count);
> struct dentry *root;
>
> + ns = binfmt_ns(file->f_path.dentry->d_sb->s_user_ns);
> switch (res) {
> case 1:
> /* Disable all handlers. */
> - enabled = 0;
> + ns->enabled = 0;
> break;
> case 2:
> /* Enable all handlers. */
> - enabled = 1;
> + ns->enabled = 1;
> break;
> case 3:
> /* Delete all handlers. */
> root = file_inode(file)->i_sb->s_root;
> inode_lock(d_inode(root));
>
> - while (!list_empty(&entries))
> - kill_node(list_first_entry(&entries, Node, list));
> + while (!list_empty(&ns->entries))
> + kill_node(ns, list_first_entry(&ns->entries,
> + Node, list));
>
> inode_unlock(d_inode(root));
> break;
> @@ -823,12 +847,34 @@ static const struct super_operations s_ops = {
> static int bm_fill_super(struct super_block *sb, void *data, int silent)
> {
> int err;
> + struct user_namespace *ns = sb->s_user_ns;
> static const struct tree_descr bm_files[] = {
> [2] = {"status", &bm_status_operations, S_IWUSR|S_IRUGO},
> [3] = {"register", &bm_register_operations, S_IWUSR},
> /* last one */ {""}
> };
>
> + /* create a new binfmt namespace
> + * if we are not in the first user namespace
> + * but the binfmt namespace is the first one
> + */
> + if (READ_ONCE(ns->binfmt_ns) == NULL) {
> + struct binfmt_namespace *new_ns;
> +
> + new_ns = kmalloc(sizeof(struct binfmt_namespace),
> + GFP_KERNEL);
> + if (new_ns == NULL)
> + return -ENOMEM;
> + INIT_LIST_HEAD(&new_ns->entries);
> + new_ns->enabled = 1;
> + rwlock_init(&new_ns->entries_lock);
> + new_ns->bm_mnt = NULL;
> + new_ns->entry_count = 0;
> + /* ensure new_ns is completely initialized before sharing it */
> + smp_wmb();
(I haven't dived into patch logic, here just small barrier remark from quick sight).
smp_wmb() has no sense without paired smp_rmb() on the read side. Possible,
you want something like below in read hunk:
+ b_ns = READ_ONCE(ns->binfmt_ns);
+ if (b_ns) {
+ smp_rmb();
+ return b_ns;
+ }
> + WRITE_ONCE(ns->binfmt_ns, new_ns);
> + }
> +
> err = simple_fill_super(sb, BINFMTFS_MAGIC, bm_files);
> if (!err)
> sb->s_op = &s_ops;
> @@ -838,7 +884,10 @@ static int bm_fill_super(struct super_block *sb, void *data, int silent)
> static struct dentry *bm_mount(struct file_system_type *fs_type,
> int flags, const char *dev_name, void *data)
> {
> - return mount_single(fs_type, flags, data, bm_fill_super);
> + struct user_namespace *ns = current_user_ns();
> +
> + return mount_ns(fs_type, flags, data, ns, ns,
> + bm_fill_super);
> }
>
> static struct linux_binfmt misc_format = {
> @@ -849,6 +898,7 @@ static struct linux_binfmt misc_format = {
> static struct file_system_type bm_fs_type = {
> .owner = THIS_MODULE,
> .name = "binfmt_misc",
> + .fs_flags = FS_USERNS_MOUNT,
> .mount = bm_mount,
> .kill_sb = kill_litter_super,
> };
> diff --git a/include/linux/user_namespace.h b/include/linux/user_namespace.h
> index d6b74b91096b..5c6e7e63b97e 100644
> --- a/include/linux/user_namespace.h
> +++ b/include/linux/user_namespace.h
> @@ -52,6 +52,16 @@ enum ucount_type {
> UCOUNT_COUNTS,
> };
>
> +#if IS_ENABLED(CONFIG_BINFMT_MISC)
> +struct binfmt_namespace {
> + struct list_head entries;
> + rwlock_t entries_lock;
> + int enabled;
> + struct vfsmount *bm_mnt;
> + int entry_count;
> +} __randomize_layout;
> +#endif
> +
> struct user_namespace {
> struct uid_gid_map uid_map;
> struct uid_gid_map gid_map;
> @@ -76,6 +86,9 @@ struct user_namespace {
> #endif
> struct ucounts *ucounts;
> int ucount_max[UCOUNT_COUNTS];
> +#if IS_ENABLED(CONFIG_BINFMT_MISC)
> + struct binfmt_namespace *binfmt_ns;
> +#endif
> } __randomize_layout;
>
> struct ucounts {
> diff --git a/kernel/user.c b/kernel/user.c
> index 0df9b1640b2a..912916d435aa 100644
> --- a/kernel/user.c
> +++ b/kernel/user.c
> @@ -19,6 +19,16 @@
> #include <linux/user_namespace.h>
> #include <linux/proc_ns.h>
>
> +#if IS_ENABLED(CONFIG_BINFMT_MISC)
> +static struct binfmt_namespace init_binfmt_ns = {
> + .entries = LIST_HEAD_INIT(init_binfmt_ns.entries),
> + .enabled = 1,
> + .entries_lock = __RW_LOCK_UNLOCKED(init_binfmt_ns.entries_lock),
> + .bm_mnt = NULL,
> + .entry_count = 0,
> +};
> +#endif
> +
> /*
> * userns count is 1 for root user, 1 for init_uts_ns,
> * and 1 for... ?
> @@ -66,6 +76,9 @@ struct user_namespace init_user_ns = {
> .persistent_keyring_register_sem =
> __RWSEM_INITIALIZER(init_user_ns.persistent_keyring_register_sem),
> #endif
> +#if IS_ENABLED(CONFIG_BINFMT_MISC)
> + .binfmt_ns = &init_binfmt_ns,
> +#endif
> };
> EXPORT_SYMBOL_GPL(init_user_ns);
>
> diff --git a/kernel/user_namespace.c b/kernel/user_namespace.c
> index e5222b5fb4fe..990cf5950a89 100644
> --- a/kernel/user_namespace.c
> +++ b/kernel/user_namespace.c
> @@ -195,6 +195,9 @@ static void free_user_ns(struct work_struct *work)
> kfree(ns->projid_map.forward);
> kfree(ns->projid_map.reverse);
> }
> +#if IS_ENABLED(CONFIG_BINFMT_MISC)
> + kfree(ns->binfmt_ns);
> +#endif
> retire_userns_sysctls(ns);
> #ifdef CONFIG_PERSISTENT_KEYRINGS
> key_put(ns->persistent_keyring_register);
>
^ permalink raw reply
* Re: [PATCH v7 3/6] seccomp: add a way to get a listener fd from ptrace
From: Christian Brauner @ 2018-10-09 16:20 UTC (permalink / raw)
To: Jann Horn
Cc: Tycho Andersen, Kees Cook, Linux API, containers, suda.akihiro,
Oleg Nesterov, kernel list, Eric W. Biederman, linux-fsdevel,
Christian Brauner, Andy Lutomirski, linux-security-module,
selinux, Paul Moore, Stephen Smalley, Eric Paris
In-Reply-To: <CAG48ez12SYRUJTWRdzWhpvHBkQion4i8Gpef4r1L6epo9JF-ng@mail.gmail.com>
On Tue, Oct 09, 2018 at 05:26:26PM +0200, Jann Horn wrote:
> On Tue, Oct 9, 2018 at 4:09 PM Christian Brauner <christian@brauner.io> wrote:
> > On Tue, Oct 09, 2018 at 03:50:53PM +0200, Jann Horn wrote:
> > > On Tue, Oct 9, 2018 at 3:49 PM Christian Brauner <christian@brauner.io> wrote:
> > > > On Tue, Oct 09, 2018 at 03:36:04PM +0200, Jann Horn wrote:
> > > > > On Tue, Oct 9, 2018 at 3:29 PM Christian Brauner <christian@brauner.io> wrote:
> > > > > > One more thing. Citing from [1]
> > > > > >
> > > > > > > I think there's a security problem here. Imagine the following scenario:
> > > > > > >
> > > > > > > 1. task A (uid==0) sets up a seccomp filter that uses SECCOMP_RET_USER_NOTIF
> > > > > > > 2. task A forks off a child B
> > > > > > > 3. task B uses setuid(1) to drop its privileges
> > > > > > > 4. task B becomes dumpable again, either via prctl(PR_SET_DUMPABLE, 1)
> > > > > > > or via execve()
> > > > > > > 5. task C (the attacker, uid==1) attaches to task B via ptrace
> > > > > > > 6. task C uses PTRACE_SECCOMP_NEW_LISTENER on task B
> > > > > >
> > > > > > Sorry, to be late to the party but would this really pass
> > > > > > __ptrace_may_access() in ptrace_attach()? It doesn't seem obvious to me
> > > > > > that it would... Doesn't look like it would get past:
> > > > > >
> > > > > > tcred = __task_cred(task);
> > > > > > if (uid_eq(caller_uid, tcred->euid) &&
> > > > > > uid_eq(caller_uid, tcred->suid) &&
> > > > > > uid_eq(caller_uid, tcred->uid) &&
> > > > > > gid_eq(caller_gid, tcred->egid) &&
> > > > > > gid_eq(caller_gid, tcred->sgid) &&
> > > > > > gid_eq(caller_gid, tcred->gid))
> > > > > > goto ok;
> > > > > > if (ptrace_has_cap(tcred->user_ns, mode))
> > > > > > goto ok;
> > > > > > rcu_read_unlock();
> > > > > > return -EPERM;
> > > > > > ok:
> > > > > > rcu_read_unlock();
> > > > > > mm = task->mm;
> > > > > > if (mm &&
> > > > > > ((get_dumpable(mm) != SUID_DUMP_USER) &&
> > > > > > !ptrace_has_cap(mm->user_ns, mode)))
> > > > > > return -EPERM;
> > > > >
> > > > > Which specific check would prevent task C from attaching to task B? If
> > > > > the UIDs match, the first "goto ok" executes; and you're dumpable, so
> > > > > you don't trigger the second "return -EPERM".
> > > >
> > > > You'd also need CAP_SYS_PTRACE in the mm->user_ns which you shouldn't
> > > > have if you did a setuid to an unpriv user. (But I always find that code
> > > > confusing.)
> > >
> > > Only if the target hasn't gone through execve() since setuid().
> >
> > Sorry if I want to know this in excessive detail but I'd like to
> > understand this properly so bear with me :)
> > - If task B has setuid()ed and prctl(PR_SET_DUMPABLE, 1)ed but not
> > execve()ed then C won't pass ptrace_has_cap(mm->user_ns, mode).
>
> Yeah.
>
> > - If task B has setuid()ed, exeved()ed it will get its dumpable flag set
> > to /proc/sys/fs/suid_dumpable
>
> Not if you changed all UIDs (e.g. by calling setuid() as root). In
> that case, setup_new_exec() calls "set_dumpable(current->mm,
> SUID_DUMP_USER)".
Actually, looking at this when C is trying to PTRACE_ATTACH to B as an
unprivileged user even if B execve()ed and it is dumpable C still
wouldn't have CAP_SYS_PTRACE in the mm->user_ns unless it already is
privileged over mm->user_ns which means it must be in an ancestor
user_ns.
>
> > which by default is 0. So C won't pass
> > (get_dumpable(mm) != SUID_DUMP_USER).
> > In both cases PTRACE_ATTACH shouldn't work. Now, if
> > /proc/sys/fs/suid_dumpable is 1 I'd find it acceptable for this to work.
> > This is an administrator choice.
^ permalink raw reply
* Re: [PATCH v7 1/6] seccomp: add a return code to trap to userspace
From: Christian Brauner @ 2018-10-09 16:24 UTC (permalink / raw)
To: Tycho Andersen
Cc: Kees Cook, Jann Horn, Linux API, Linux Containers, Akihiro Suda,
Oleg Nesterov, LKML, Eric W . Biederman,
linux-fsdevel@vger.kernel.org, Christian Brauner, Andy Lutomirski
In-Reply-To: <20181009142833.GA10149@cisco>
On Tue, Oct 09, 2018 at 07:28:33AM -0700, Tycho Andersen wrote:
> On Mon, Oct 08, 2018 at 04:58:05PM +0200, Christian Brauner wrote:
> > On Thu, Sep 27, 2018 at 04:48:39PM -0600, Tycho Andersen wrote:
> > > On Thu, Sep 27, 2018 at 02:31:24PM -0700, Kees Cook wrote:
> > > > I have to say, I'm vaguely nervous about changing the semantics here
> > > > for passing back the fd as the return code from the seccomp() syscall.
> > > > Alternatives seem less appealing, though: changing the meaning of the
> > > > uargs parameter when SECCOMP_FILTER_FLAG_NEW_LISTENER is set, for
> > > > example. Hmm.
> > >
> > > From my perspective we can drop this whole thing. The only thing I'll
> > > ever use is the ptrace version. Someone at some point (I don't
> > > remember who, maybe stgraber) suggested this version would be useful
> > > as well.
> >
> > So I think we want to have the ability to get an fd via seccomp().
> > Especially, if we all we worry about are weird semantics. When we
> > discussed this we knew the whole patchset was going to be weird. :)
> >
> > This is a seccomp feature so seccomp should - if feasible - equip you
> > with everything to use it in a meaningful way without having to go
> > through a different kernel api. I know ptrace and seccomp are
> > already connected but I still find this cleaner. :)
> >
> > Another thing is that the container itself might be traced for some
> > reason while you still might want to get an fd out.
>
> Sure, I don't see the problem here.
How'd you to PTRACE_ATTACH in that case?
Anyway, the whole point is as we've discusses in the other thread we
really want a one-syscall-only, purely-seccomp() based way of getting
the fd. There are multiple options to get the fd even when you block
sendmsg()/socket() whatever and there's no good reason to only be able
to get the fd via a three-syscall-ptrace dance. :)
^ permalink raw reply
* Re: [PATCH v7 3/6] seccomp: add a way to get a listener fd from ptrace
From: Jann Horn @ 2018-10-09 16:26 UTC (permalink / raw)
To: christian
Cc: Tycho Andersen, Kees Cook, Linux API, containers, suda.akihiro,
Oleg Nesterov, kernel list, Eric W. Biederman, linux-fsdevel,
Christian Brauner, Andy Lutomirski, linux-security-module,
selinux, Paul Moore, Stephen Smalley, Eric Paris
In-Reply-To: <20181009162022.d7fd2wibyq6xi6sg@brauner.io>
On Tue, Oct 9, 2018 at 6:20 PM Christian Brauner <christian@brauner.io> wrote:
> On Tue, Oct 09, 2018 at 05:26:26PM +0200, Jann Horn wrote:
> > On Tue, Oct 9, 2018 at 4:09 PM Christian Brauner <christian@brauner.io> wrote:
> > > On Tue, Oct 09, 2018 at 03:50:53PM +0200, Jann Horn wrote:
> > > > On Tue, Oct 9, 2018 at 3:49 PM Christian Brauner <christian@brauner.io> wrote:
> > > > > On Tue, Oct 09, 2018 at 03:36:04PM +0200, Jann Horn wrote:
> > > > > > On Tue, Oct 9, 2018 at 3:29 PM Christian Brauner <christian@brauner.io> wrote:
> > > > > > > One more thing. Citing from [1]
> > > > > > >
> > > > > > > > I think there's a security problem here. Imagine the following scenario:
> > > > > > > >
> > > > > > > > 1. task A (uid==0) sets up a seccomp filter that uses SECCOMP_RET_USER_NOTIF
> > > > > > > > 2. task A forks off a child B
> > > > > > > > 3. task B uses setuid(1) to drop its privileges
> > > > > > > > 4. task B becomes dumpable again, either via prctl(PR_SET_DUMPABLE, 1)
> > > > > > > > or via execve()
> > > > > > > > 5. task C (the attacker, uid==1) attaches to task B via ptrace
> > > > > > > > 6. task C uses PTRACE_SECCOMP_NEW_LISTENER on task B
> > > > > > >
> > > > > > > Sorry, to be late to the party but would this really pass
> > > > > > > __ptrace_may_access() in ptrace_attach()? It doesn't seem obvious to me
> > > > > > > that it would... Doesn't look like it would get past:
> > > > > > >
> > > > > > > tcred = __task_cred(task);
> > > > > > > if (uid_eq(caller_uid, tcred->euid) &&
> > > > > > > uid_eq(caller_uid, tcred->suid) &&
> > > > > > > uid_eq(caller_uid, tcred->uid) &&
> > > > > > > gid_eq(caller_gid, tcred->egid) &&
> > > > > > > gid_eq(caller_gid, tcred->sgid) &&
> > > > > > > gid_eq(caller_gid, tcred->gid))
> > > > > > > goto ok;
> > > > > > > if (ptrace_has_cap(tcred->user_ns, mode))
> > > > > > > goto ok;
> > > > > > > rcu_read_unlock();
> > > > > > > return -EPERM;
> > > > > > > ok:
> > > > > > > rcu_read_unlock();
> > > > > > > mm = task->mm;
> > > > > > > if (mm &&
> > > > > > > ((get_dumpable(mm) != SUID_DUMP_USER) &&
> > > > > > > !ptrace_has_cap(mm->user_ns, mode)))
> > > > > > > return -EPERM;
> > > > > >
> > > > > > Which specific check would prevent task C from attaching to task B? If
> > > > > > the UIDs match, the first "goto ok" executes; and you're dumpable, so
> > > > > > you don't trigger the second "return -EPERM".
> > > > >
> > > > > You'd also need CAP_SYS_PTRACE in the mm->user_ns which you shouldn't
> > > > > have if you did a setuid to an unpriv user. (But I always find that code
> > > > > confusing.)
> > > >
> > > > Only if the target hasn't gone through execve() since setuid().
> > >
> > > Sorry if I want to know this in excessive detail but I'd like to
> > > understand this properly so bear with me :)
> > > - If task B has setuid()ed and prctl(PR_SET_DUMPABLE, 1)ed but not
> > > execve()ed then C won't pass ptrace_has_cap(mm->user_ns, mode).
> >
> > Yeah.
> >
> > > - If task B has setuid()ed, exeved()ed it will get its dumpable flag set
> > > to /proc/sys/fs/suid_dumpable
> >
> > Not if you changed all UIDs (e.g. by calling setuid() as root). In
> > that case, setup_new_exec() calls "set_dumpable(current->mm,
> > SUID_DUMP_USER)".
>
> Actually, looking at this when C is trying to PTRACE_ATTACH to B as an
> unprivileged user even if B execve()ed and it is dumpable C still
> wouldn't have CAP_SYS_PTRACE in the mm->user_ns unless it already is
> privileged over mm->user_ns which means it must be in an ancestor
> user_ns.
Huh? Why would you need CAP_SYS_PTRACE for anything here? You can
ptrace another process running under your UID just fine, no matter
what the namespaces are. I'm not sure what you're saying.
^ permalink raw reply
* Re: [PATCH v7 1/6] seccomp: add a return code to trap to userspace
From: Tycho Andersen @ 2018-10-09 16:29 UTC (permalink / raw)
To: Christian Brauner
Cc: Kees Cook, Jann Horn, Linux API, Linux Containers, Akihiro Suda,
Oleg Nesterov, LKML, Eric W . Biederman,
linux-fsdevel@vger.kernel.org, Christian Brauner, Andy Lutomirski
In-Reply-To: <20181009162413.sry5hnplvvpjggd4@brauner.io>
On Tue, Oct 09, 2018 at 06:24:14PM +0200, Christian Brauner wrote:
> On Tue, Oct 09, 2018 at 07:28:33AM -0700, Tycho Andersen wrote:
> > On Mon, Oct 08, 2018 at 04:58:05PM +0200, Christian Brauner wrote:
> > > On Thu, Sep 27, 2018 at 04:48:39PM -0600, Tycho Andersen wrote:
> > > > On Thu, Sep 27, 2018 at 02:31:24PM -0700, Kees Cook wrote:
> > > > > I have to say, I'm vaguely nervous about changing the semantics here
> > > > > for passing back the fd as the return code from the seccomp() syscall.
> > > > > Alternatives seem less appealing, though: changing the meaning of the
> > > > > uargs parameter when SECCOMP_FILTER_FLAG_NEW_LISTENER is set, for
> > > > > example. Hmm.
> > > >
> > > > From my perspective we can drop this whole thing. The only thing I'll
> > > > ever use is the ptrace version. Someone at some point (I don't
> > > > remember who, maybe stgraber) suggested this version would be useful
> > > > as well.
> > >
> > > So I think we want to have the ability to get an fd via seccomp().
> > > Especially, if we all we worry about are weird semantics. When we
> > > discussed this we knew the whole patchset was going to be weird. :)
> > >
> > > This is a seccomp feature so seccomp should - if feasible - equip you
> > > with everything to use it in a meaningful way without having to go
> > > through a different kernel api. I know ptrace and seccomp are
> > > already connected but I still find this cleaner. :)
> > >
> > > Another thing is that the container itself might be traced for some
> > > reason while you still might want to get an fd out.
> >
> > Sure, I don't see the problem here.
>
> How'd you to PTRACE_ATTACH in that case?
Oh, you mean if someone has *ptrace*'d the task, and a third party
wants to get a seccomp fd? I think "too bad" is the answer; I don't
really mind not supporting this case.
> Anyway, the whole point is as we've discusses in the other thread we
> really want a one-syscall-only, purely-seccomp() based way of getting
> the fd. There are multiple options to get the fd even when you block
> sendmsg()/socket() whatever and there's no good reason to only be able
> to get the fd via a three-syscall-ptrace dance. :)
Ok, I'll leave these bits in then for v8.
Tycho
^ permalink raw reply
* Re: [RFC v5 1/1] ns: add binfmt_misc to the user namespace
From: Laurent Vivier @ 2018-10-09 16:45 UTC (permalink / raw)
To: Kirill Tkhai, linux-kernel@vger.kernel.org
Cc: Eric Biederman, Dmitry Safonov, linux-api@vger.kernel.org,
James Bottomley, Alexander Viro, linux-fsdevel@vger.kernel.org,
Andrei Vagin (C), containers@lists.linux-foundation.org,
Jann Horn
In-Reply-To: <f14ce318-5a1a-06f3-2e90-91220f39e74b@virtuozzo.com>
Le 09/10/2018 à 18:15, Kirill Tkhai a écrit :
> On 09.10.2018 13:37, Laurent Vivier wrote:
>> This patch allows to have a different binfmt_misc configuration
>> for each new user namespace. By default, the binfmt_misc configuration
>> is the one of the previous level, but if the binfmt_misc filesystem is
>> mounted in the new namespace a new empty binfmt instance is created and
>> used in this namespace.
>>
>> For instance, using "unshare" we can start a chroot of an another
>> architecture and configure the binfmt_misc interpreter without being root
>> to run the binaries in this chroot.
>>
>> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
>> ---
>> fs/binfmt_misc.c | 106 ++++++++++++++++++++++++---------
>> include/linux/user_namespace.h | 13 ++++
>> kernel/user.c | 13 ++++
>> kernel/user_namespace.c | 3 +
>> 4 files changed, 107 insertions(+), 28 deletions(-)
>>
>> diff --git a/fs/binfmt_misc.c b/fs/binfmt_misc.c
>> index aa4a7a23ff99..1e0029d097d9 100644
>> --- a/fs/binfmt_misc.c
>> +++ b/fs/binfmt_misc.c
...
>> @@ -80,18 +74,32 @@ static int entry_count;
>> */
>> #define MAX_REGISTER_LENGTH 1920
>>
>> +static struct binfmt_namespace *binfmt_ns(struct user_namespace *ns)
>> +{
>> + struct binfmt_namespace *b_ns;
>> +
>> + while (ns) {
>> + b_ns = READ_ONCE(ns->binfmt_ns);
>> + if (b_ns)
>> + return b_ns;
>> + ns = ns->parent;
>> + }
>> + WARN_ON_ONCE(1);
>> + return NULL;
>> +}
>> +
...
>> @@ -823,12 +847,34 @@ static const struct super_operations s_ops = {
>> static int bm_fill_super(struct super_block *sb, void *data, int silent)
>> {
>> int err;
>> + struct user_namespace *ns = sb->s_user_ns;
>> static const struct tree_descr bm_files[] = {
>> [2] = {"status", &bm_status_operations, S_IWUSR|S_IRUGO},
>> [3] = {"register", &bm_register_operations, S_IWUSR},
>> /* last one */ {""}
>> };
>>
>> + /* create a new binfmt namespace
>> + * if we are not in the first user namespace
>> + * but the binfmt namespace is the first one
>> + */
>> + if (READ_ONCE(ns->binfmt_ns) == NULL) {
>> + struct binfmt_namespace *new_ns;
>> +
>> + new_ns = kmalloc(sizeof(struct binfmt_namespace),
>> + GFP_KERNEL);
>> + if (new_ns == NULL)
>> + return -ENOMEM;
>> + INIT_LIST_HEAD(&new_ns->entries);
>> + new_ns->enabled = 1;
>> + rwlock_init(&new_ns->entries_lock);
>> + new_ns->bm_mnt = NULL;
>> + new_ns->entry_count = 0;
>> + /* ensure new_ns is completely initialized before sharing it */
>> + smp_wmb();
>
> (I haven't dived into patch logic, here just small barrier remark from quick sight).
> smp_wmb() has no sense without paired smp_rmb() on the read side. Possible,
> you want something like below in read hunk:
>
> + b_ns = READ_ONCE(ns->binfmt_ns);
> + if (b_ns) {
> + smp_rmb();
> + return b_ns;
> + }
>
>
The write barrier is here to ensure the structure is fully written
before we set the pointer.
I don't understand how read barrier can change something at this level,
IMHO the couple WRITE_ONCE()/READ_ONCE() should be enough to ensure we
have correctly initialized the pointer and the structure when we read
the pointer back.
I think the pointer itself is the "barrier" to access the memory
modified before.
Thanks,
Laurent
^ permalink raw reply
* Re: [PATCH v3 3/3] namei: aggressively check for nd->root escape on ".." resolution
From: Jann Horn @ 2018-10-09 16:46 UTC (permalink / raw)
To: asarai
Cc: cyphar, Al Viro, Eric W. Biederman, jlayton, Bruce Fields,
Arnd Bergmann, Andy Lutomirski, David Howells, christian,
Tycho Andersen, David Drysdale, dev, containers, linux-fsdevel,
kernel list, linux-arch, Linux API
In-Reply-To: <20181009153728.2altaqxclntvyc7b@mikami>
On Tue, Oct 9, 2018 at 5:36 PM Aleksa Sarai <asarai@suse.de> wrote:
> On 2018-10-09, 'Jann Horn' via dev <jannh@google.com> wrote:
> > On Tue, Oct 9, 2018 at 9:03 AM Aleksa Sarai <cyphar@cyphar.com> wrote:
> > > This patch allows for AT_BENEATH and AT_THIS_ROOT to safely permit ".."
> > > resolution (in the case of AT_BENEATH the resolution will still fail if
> > > ".." resolution would resolve a path outside of the root -- while
> > > AT_THIS_ROOT will chroot(2)-style scope it). "proclink" jumps are still
> > > disallowed entirely because now they could result in inconsistent
> > > behaviour if resolution encounters a subsequent "..".
> > >
> > > The need for this patch is explained by observing there is a fairly
> > > easy-to-exploit race condition with chroot(2) (and thus by extension
> > > AT_THIS_ROOT and AT_BENEATH) where a rename(2) of a path can be used to
> > > "skip over" nd->root and thus escape to the filesystem above nd->root.
> > >
> > > thread1 [attacker]:
> > > for (;;)
> > > renameat2(AT_FDCWD, "/a/b/c", AT_FDCWD, "/a/d", RENAME_EXCHANGE);
> > > thread2 [victim]:
> > > for (;;)
> > > openat(dirb, "b/c/../../etc/shadow", O_THISROOT);
> > >
> > > With fairly significant regularity, thread2 will resolve to
> > > "/etc/shadow" rather than "/a/b/etc/shadow". There is also a similar
> > > (though somewhat more privileged) attack using MS_MOVE.
> > >
> > > With this patch, such cases will be detected *during* ".." resolution
> > > (which is the weak point of chroot(2) -- since walking *into* a
> > > subdirectory tautologically cannot result in you walking *outside*
> > > nd->root -- except through a bind-mount or "proclink"). By detecting
> > > this at ".." resolution (rather than checking only at the end of the
> > > entire resolution) we can both correct escapes by jumping back to the
> > > root (in the case of AT_THIS_ROOT), as well as avoid revealing to
> > > attackers the structure of the filesystem outside of the root (through
> > > timing attacks for instance).
> > >
> > > In order to avoid a quadratic lookup with each ".." entry, we only
> > > activate the slow path if a write through &rename_lock or &mount_lock
> > > have occurred during path resolution (&rename_lock and &mount_lock are
> > > re-taken to further optimise the lookup). Since the primary attack being
> > > protected against is MS_MOVE or rename(2), not doing additional checks
> > > unless a mount or rename have occurred avoids making the common case
> > > slow.
> > >
> > > The use of __d_path here might seem suspect, but on further inspection
> > > of the most important race (a path was *inside* the root but is now
> > > *outside*), there appears to be no attack potential. If __d_path occurs
> > > before the rename, then the path will be resolved but since the path was
> > > originally inside the root there is no escape. Subsequent ".." jumps are
> > > guaranteed to check __d_path reachable (by construction, &rename_lock or
> > > &mount_lock must have been taken after __d_path returned),
> >
> > "after"? Don't you mean "before"? Otherwise I don't understand what
> > you're saying here.
>
> I meant that the attacker doing the rename must've taken &rename_lock
> or &mount_lock after __d_path returns in the target process (because the
> race being examined is that the rename occurs *after* __d_path) and thus
> are guaranteed to be detected).
>
> Maybe there's a better way to phrase what I mean...
Aah, I thought you were referring to what the victim process is doing,
not what the racing attacker is doing.
^ permalink raw reply
* Re: [RFC v5 1/1] ns: add binfmt_misc to the user namespace
From: Jann Horn @ 2018-10-09 16:53 UTC (permalink / raw)
To: Laurent Vivier
Cc: ktkhai, kernel list, Eric W. Biederman, dima, Linux API,
James Bottomley, Al Viro, linux-fsdevel, Andrei Vagin, containers
In-Reply-To: <bc11ca64-8616-f06a-c1fa-c76511e60257@vivier.eu>
On Tue, Oct 9, 2018 at 6:45 PM Laurent Vivier <laurent@vivier.eu> wrote:
> Le 09/10/2018 à 18:15, Kirill Tkhai a écrit :
> > On 09.10.2018 13:37, Laurent Vivier wrote:
> >> This patch allows to have a different binfmt_misc configuration
> >> for each new user namespace. By default, the binfmt_misc configuration
> >> is the one of the previous level, but if the binfmt_misc filesystem is
> >> mounted in the new namespace a new empty binfmt instance is created and
> >> used in this namespace.
> >>
> >> For instance, using "unshare" we can start a chroot of an another
> >> architecture and configure the binfmt_misc interpreter without being root
> >> to run the binaries in this chroot.
> >>
> >> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
> >> ---
> >> fs/binfmt_misc.c | 106 ++++++++++++++++++++++++---------
> >> include/linux/user_namespace.h | 13 ++++
> >> kernel/user.c | 13 ++++
> >> kernel/user_namespace.c | 3 +
> >> 4 files changed, 107 insertions(+), 28 deletions(-)
> >>
> >> diff --git a/fs/binfmt_misc.c b/fs/binfmt_misc.c
> >> index aa4a7a23ff99..1e0029d097d9 100644
> >> --- a/fs/binfmt_misc.c
> >> +++ b/fs/binfmt_misc.c
> ...
> >> @@ -80,18 +74,32 @@ static int entry_count;
> >> */
> >> #define MAX_REGISTER_LENGTH 1920
> >>
> >> +static struct binfmt_namespace *binfmt_ns(struct user_namespace *ns)
> >> +{
> >> + struct binfmt_namespace *b_ns;
> >> +
> >> + while (ns) {
> >> + b_ns = READ_ONCE(ns->binfmt_ns);
> >> + if (b_ns)
> >> + return b_ns;
> >> + ns = ns->parent;
> >> + }
> >> + WARN_ON_ONCE(1);
> >> + return NULL;
> >> +}
> >> +
> ...
> >> @@ -823,12 +847,34 @@ static const struct super_operations s_ops = {
> >> static int bm_fill_super(struct super_block *sb, void *data, int silent)
> >> {
> >> int err;
> >> + struct user_namespace *ns = sb->s_user_ns;
> >> static const struct tree_descr bm_files[] = {
> >> [2] = {"status", &bm_status_operations, S_IWUSR|S_IRUGO},
> >> [3] = {"register", &bm_register_operations, S_IWUSR},
> >> /* last one */ {""}
> >> };
> >>
> >> + /* create a new binfmt namespace
> >> + * if we are not in the first user namespace
> >> + * but the binfmt namespace is the first one
> >> + */
> >> + if (READ_ONCE(ns->binfmt_ns) == NULL) {
> >> + struct binfmt_namespace *new_ns;
> >> +
> >> + new_ns = kmalloc(sizeof(struct binfmt_namespace),
> >> + GFP_KERNEL);
> >> + if (new_ns == NULL)
> >> + return -ENOMEM;
> >> + INIT_LIST_HEAD(&new_ns->entries);
> >> + new_ns->enabled = 1;
> >> + rwlock_init(&new_ns->entries_lock);
> >> + new_ns->bm_mnt = NULL;
> >> + new_ns->entry_count = 0;
> >> + /* ensure new_ns is completely initialized before sharing it */
> >> + smp_wmb();
> >
> > (I haven't dived into patch logic, here just small barrier remark from quick sight).
> > smp_wmb() has no sense without paired smp_rmb() on the read side. Possible,
> > you want something like below in read hunk:
> >
> > + b_ns = READ_ONCE(ns->binfmt_ns);
> > + if (b_ns) {
> > + smp_rmb();
> > + return b_ns;
> > + }
> >
> >
>
> The write barrier is here to ensure the structure is fully written
> before we set the pointer.
>
> I don't understand how read barrier can change something at this level,
> IMHO the couple WRITE_ONCE()/READ_ONCE() should be enough to ensure we
> have correctly initialized the pointer and the structure when we read
> the pointer back.
>
> I think the pointer itself is the "barrier" to access the memory
> modified before.
Things don't work that way on alpha, but that's why READ_ONCE()
includes an smp_read_barrier_depends():
#define __READ_ONCE(x, check) \
({ \
union { typeof(x) __val; char __c[1]; } __u; \
if (check) \
__read_once_size(&(x), __u.__c, sizeof(x)); \
else \
__read_once_size_nocheck(&(x), __u.__c, sizeof(x)); \
smp_read_barrier_depends(); /* Enforce dependency ordering from x */ \
__u.__val; \
})
#define READ_ONCE(x) __READ_ONCE(x, 1)
^ permalink raw reply
* Re: [PATCH 04/11] UAPI: bcache: Fix use of embedded flexible array
From: Jan Engelhardt @ 2018-10-09 16:54 UTC (permalink / raw)
To: David Howells
Cc: linux-api, linux-kbuild, Coly Li, Kent Overstreet, linux-bcache,
linux-kernel
In-Reply-To: <17992.1539099666@warthog.procyon.org.uk>
On Tuesday 2018-10-09 17:41, David Howells wrote:
>Jan Engelhardt <jengelh@inai.de> wrote:
>
>> """it [the array size expression] shall be a converted constant expression of
>> type std::size_t and its value shall be greater than zero."""
>> —http://eel.is/c++draft/dcl.array
>
>Interesting. You're not actually quoting the full sentence:
>
> If the constant-expression is present, it shall be a converted
> constant expression of type std::size_t and its value shall be
> greater than zero.
>
>This suggests that:
>
> __u64 ptr[]
>
>is actually valid
I think that kind of validity only goes for this kind of standalone
decl:
extern int myints[];
but not for []-inside-struct.
^ permalink raw reply
* Re: [RFC v5 1/1] ns: add binfmt_misc to the user namespace
From: Laurent Vivier @ 2018-10-09 16:57 UTC (permalink / raw)
To: Jann Horn
Cc: ktkhai, kernel list, Eric W. Biederman, dima, Linux API,
James Bottomley, Al Viro, linux-fsdevel, Andrei Vagin, containers
In-Reply-To: <CAG48ez0pAxC+yetPfM+XyS8kfqA4hw1hxjKG55gbF-4qcNWUiA@mail.gmail.com>
Le 09/10/2018 à 18:53, Jann Horn a écrit :
> On Tue, Oct 9, 2018 at 6:45 PM Laurent Vivier <laurent@vivier.eu> wrote:
>> Le 09/10/2018 à 18:15, Kirill Tkhai a écrit :
>>> On 09.10.2018 13:37, Laurent Vivier wrote:
>>>> This patch allows to have a different binfmt_misc configuration
>>>> for each new user namespace. By default, the binfmt_misc configuration
>>>> is the one of the previous level, but if the binfmt_misc filesystem is
>>>> mounted in the new namespace a new empty binfmt instance is created and
>>>> used in this namespace.
>>>>
>>>> For instance, using "unshare" we can start a chroot of an another
>>>> architecture and configure the binfmt_misc interpreter without being root
>>>> to run the binaries in this chroot.
>>>>
>>>> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
>>>> ---
>>>> fs/binfmt_misc.c | 106 ++++++++++++++++++++++++---------
>>>> include/linux/user_namespace.h | 13 ++++
>>>> kernel/user.c | 13 ++++
>>>> kernel/user_namespace.c | 3 +
>>>> 4 files changed, 107 insertions(+), 28 deletions(-)
>>>>
>>>> diff --git a/fs/binfmt_misc.c b/fs/binfmt_misc.c
>>>> index aa4a7a23ff99..1e0029d097d9 100644
>>>> --- a/fs/binfmt_misc.c
>>>> +++ b/fs/binfmt_misc.c
>> ...
>>>> @@ -80,18 +74,32 @@ static int entry_count;
>>>> */
>>>> #define MAX_REGISTER_LENGTH 1920
>>>>
>>>> +static struct binfmt_namespace *binfmt_ns(struct user_namespace *ns)
>>>> +{
>>>> + struct binfmt_namespace *b_ns;
>>>> +
>>>> + while (ns) {
>>>> + b_ns = READ_ONCE(ns->binfmt_ns);
>>>> + if (b_ns)
>>>> + return b_ns;
>>>> + ns = ns->parent;
>>>> + }
>>>> + WARN_ON_ONCE(1);
>>>> + return NULL;
>>>> +}
>>>> +
>> ...
>>>> @@ -823,12 +847,34 @@ static const struct super_operations s_ops = {
>>>> static int bm_fill_super(struct super_block *sb, void *data, int silent)
>>>> {
>>>> int err;
>>>> + struct user_namespace *ns = sb->s_user_ns;
>>>> static const struct tree_descr bm_files[] = {
>>>> [2] = {"status", &bm_status_operations, S_IWUSR|S_IRUGO},
>>>> [3] = {"register", &bm_register_operations, S_IWUSR},
>>>> /* last one */ {""}
>>>> };
>>>>
>>>> + /* create a new binfmt namespace
>>>> + * if we are not in the first user namespace
>>>> + * but the binfmt namespace is the first one
>>>> + */
>>>> + if (READ_ONCE(ns->binfmt_ns) == NULL) {
>>>> + struct binfmt_namespace *new_ns;
>>>> +
>>>> + new_ns = kmalloc(sizeof(struct binfmt_namespace),
>>>> + GFP_KERNEL);
>>>> + if (new_ns == NULL)
>>>> + return -ENOMEM;
>>>> + INIT_LIST_HEAD(&new_ns->entries);
>>>> + new_ns->enabled = 1;
>>>> + rwlock_init(&new_ns->entries_lock);
>>>> + new_ns->bm_mnt = NULL;
>>>> + new_ns->entry_count = 0;
>>>> + /* ensure new_ns is completely initialized before sharing it */
>>>> + smp_wmb();
>>>
>>> (I haven't dived into patch logic, here just small barrier remark from quick sight).
>>> smp_wmb() has no sense without paired smp_rmb() on the read side. Possible,
>>> you want something like below in read hunk:
>>>
>>> + b_ns = READ_ONCE(ns->binfmt_ns);
>>> + if (b_ns) {
>>> + smp_rmb();
>>> + return b_ns;
>>> + }
>>>
>>>
>>
>> The write barrier is here to ensure the structure is fully written
>> before we set the pointer.
>>
>> I don't understand how read barrier can change something at this level,
>> IMHO the couple WRITE_ONCE()/READ_ONCE() should be enough to ensure we
>> have correctly initialized the pointer and the structure when we read
>> the pointer back.
>>
>> I think the pointer itself is the "barrier" to access the memory
>> modified before.
>
> Things don't work that way on alpha, but that's why READ_ONCE()
> includes an smp_read_barrier_depends():
>
> #define __READ_ONCE(x, check) \
> ({ \
> union { typeof(x) __val; char __c[1]; } __u; \
> if (check) \
> __read_once_size(&(x), __u.__c, sizeof(x)); \
> else \
> __read_once_size_nocheck(&(x), __u.__c, sizeof(x)); \
> smp_read_barrier_depends(); /* Enforce dependency ordering from x */ \
> __u.__val; \
> })
> #define READ_ONCE(x) __READ_ONCE(x, 1)
>
So my questions are:
- do we need a smp_wmb() barrier if we use READ_ONCE() and WRITE_ONCE()?
- if we need an smp_wmb() barrier, do we need an smp_rmb() barrier as
the data we want to "protect" are behind an access to the pointer?
Thanks,
Laurent
^ permalink raw reply
* Re: [RFC v5 1/1] ns: add binfmt_misc to the user namespace
From: Kirill Tkhai @ 2018-10-09 17:01 UTC (permalink / raw)
To: Laurent Vivier, linux-kernel@vger.kernel.org
Cc: Eric Biederman, Dmitry Safonov, linux-api@vger.kernel.org,
James Bottomley, Alexander Viro, linux-fsdevel@vger.kernel.org,
Andrei Vagin (C), containers@lists.linux-foundation.org,
Jann Horn
In-Reply-To: <bc11ca64-8616-f06a-c1fa-c76511e60257@vivier.eu>
On 09.10.2018 19:45, Laurent Vivier wrote:
> Le 09/10/2018 à 18:15, Kirill Tkhai a écrit :
>> On 09.10.2018 13:37, Laurent Vivier wrote:
>>> This patch allows to have a different binfmt_misc configuration
>>> for each new user namespace. By default, the binfmt_misc configuration
>>> is the one of the previous level, but if the binfmt_misc filesystem is
>>> mounted in the new namespace a new empty binfmt instance is created and
>>> used in this namespace.
>>>
>>> For instance, using "unshare" we can start a chroot of an another
>>> architecture and configure the binfmt_misc interpreter without being root
>>> to run the binaries in this chroot.
>>>
>>> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
>>> ---
>>> fs/binfmt_misc.c | 106 ++++++++++++++++++++++++---------
>>> include/linux/user_namespace.h | 13 ++++
>>> kernel/user.c | 13 ++++
>>> kernel/user_namespace.c | 3 +
>>> 4 files changed, 107 insertions(+), 28 deletions(-)
>>>
>>> diff --git a/fs/binfmt_misc.c b/fs/binfmt_misc.c
>>> index aa4a7a23ff99..1e0029d097d9 100644
>>> --- a/fs/binfmt_misc.c
>>> +++ b/fs/binfmt_misc.c
> ...
>>> @@ -80,18 +74,32 @@ static int entry_count;
>>> */
>>> #define MAX_REGISTER_LENGTH 1920
>>>
>>> +static struct binfmt_namespace *binfmt_ns(struct user_namespace *ns)
>>> +{
>>> + struct binfmt_namespace *b_ns;
>>> +
>>> + while (ns) {
>>> + b_ns = READ_ONCE(ns->binfmt_ns);
>>> + if (b_ns)
>>> + return b_ns;
>>> + ns = ns->parent;
>>> + }
>>> + WARN_ON_ONCE(1);
>>> + return NULL;
>>> +}
>>> +
> ...
>>> @@ -823,12 +847,34 @@ static const struct super_operations s_ops = {
>>> static int bm_fill_super(struct super_block *sb, void *data, int silent)
>>> {
>>> int err;
>>> + struct user_namespace *ns = sb->s_user_ns;
>>> static const struct tree_descr bm_files[] = {
>>> [2] = {"status", &bm_status_operations, S_IWUSR|S_IRUGO},
>>> [3] = {"register", &bm_register_operations, S_IWUSR},
>>> /* last one */ {""}
>>> };
>>>
>>> + /* create a new binfmt namespace
>>> + * if we are not in the first user namespace
>>> + * but the binfmt namespace is the first one
>>> + */
>>> + if (READ_ONCE(ns->binfmt_ns) == NULL) {
>>> + struct binfmt_namespace *new_ns;
>>> +
>>> + new_ns = kmalloc(sizeof(struct binfmt_namespace),
>>> + GFP_KERNEL);
>>> + if (new_ns == NULL)
>>> + return -ENOMEM;
>>> + INIT_LIST_HEAD(&new_ns->entries);
>>> + new_ns->enabled = 1;
>>> + rwlock_init(&new_ns->entries_lock);
>>> + new_ns->bm_mnt = NULL;
>>> + new_ns->entry_count = 0;
>>> + /* ensure new_ns is completely initialized before sharing it */
>>> + smp_wmb();
>>
>> (I haven't dived into patch logic, here just small barrier remark from quick sight).
>> smp_wmb() has no sense without paired smp_rmb() on the read side. Possible,
>> you want something like below in read hunk:
>>
>> + b_ns = READ_ONCE(ns->binfmt_ns);
>> + if (b_ns) {
>> + smp_rmb();
>> + return b_ns;
>> + }
>>
>>
>
> The write barrier is here to ensure the structure is fully written
> before we set the pointer.
>
> I don't understand how read barrier can change something at this level,
> IMHO the couple WRITE_ONCE()/READ_ONCE() should be enough to ensure we
> have correctly initialized the pointer and the structure when we read
> the pointer back.
>
> I think the pointer itself is the "barrier" to access the memory
> modified before.
smp_rmb() guarantees you see stores in the order you want. If you have:
[cpu0] [cpu1]
new_ns->entry_count = 0;
smp_wmb();
WRITE_ONCE(ns->binfmt_ns, new_ns); b_ns = READ_ONCE(ns->binfmt_ns);
smp_rmb();
<access b_ns->entry_count>
smp_rmb() guarantees you see true entry_count on the cpu1. Without
smp_rmb() you may see old value of new_ns->entry_count.
See Documentation/memory-barriers.txt
^ permalink raw reply
* Re: [RFC v5 1/1] ns: add binfmt_misc to the user namespace
From: Jann Horn @ 2018-10-09 17:01 UTC (permalink / raw)
To: Laurent Vivier
Cc: ktkhai, kernel list, Eric W. Biederman, dima, Linux API,
James Bottomley, Al Viro, linux-fsdevel, Andrei Vagin, containers
In-Reply-To: <795fea1a-97bd-87f5-6a14-6d47a4e42f74@vivier.eu>
On Tue, Oct 9, 2018 at 6:57 PM Laurent Vivier <laurent@vivier.eu> wrote:
> Le 09/10/2018 à 18:53, Jann Horn a écrit :
> > On Tue, Oct 9, 2018 at 6:45 PM Laurent Vivier <laurent@vivier.eu> wrote:
> >> Le 09/10/2018 à 18:15, Kirill Tkhai a écrit :
> >>> On 09.10.2018 13:37, Laurent Vivier wrote:
> >>>> This patch allows to have a different binfmt_misc configuration
> >>>> for each new user namespace. By default, the binfmt_misc configuration
> >>>> is the one of the previous level, but if the binfmt_misc filesystem is
> >>>> mounted in the new namespace a new empty binfmt instance is created and
> >>>> used in this namespace.
> >>>>
> >>>> For instance, using "unshare" we can start a chroot of an another
> >>>> architecture and configure the binfmt_misc interpreter without being root
> >>>> to run the binaries in this chroot.
> >>>>
> >>>> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
> >>>> ---
> >>>> fs/binfmt_misc.c | 106 ++++++++++++++++++++++++---------
> >>>> include/linux/user_namespace.h | 13 ++++
> >>>> kernel/user.c | 13 ++++
> >>>> kernel/user_namespace.c | 3 +
> >>>> 4 files changed, 107 insertions(+), 28 deletions(-)
> >>>>
> >>>> diff --git a/fs/binfmt_misc.c b/fs/binfmt_misc.c
> >>>> index aa4a7a23ff99..1e0029d097d9 100644
> >>>> --- a/fs/binfmt_misc.c
> >>>> +++ b/fs/binfmt_misc.c
> >> ...
> >>>> @@ -80,18 +74,32 @@ static int entry_count;
> >>>> */
> >>>> #define MAX_REGISTER_LENGTH 1920
> >>>>
> >>>> +static struct binfmt_namespace *binfmt_ns(struct user_namespace *ns)
> >>>> +{
> >>>> + struct binfmt_namespace *b_ns;
> >>>> +
> >>>> + while (ns) {
> >>>> + b_ns = READ_ONCE(ns->binfmt_ns);
> >>>> + if (b_ns)
> >>>> + return b_ns;
> >>>> + ns = ns->parent;
> >>>> + }
> >>>> + WARN_ON_ONCE(1);
> >>>> + return NULL;
> >>>> +}
> >>>> +
> >> ...
> >>>> @@ -823,12 +847,34 @@ static const struct super_operations s_ops = {
> >>>> static int bm_fill_super(struct super_block *sb, void *data, int silent)
> >>>> {
> >>>> int err;
> >>>> + struct user_namespace *ns = sb->s_user_ns;
> >>>> static const struct tree_descr bm_files[] = {
> >>>> [2] = {"status", &bm_status_operations, S_IWUSR|S_IRUGO},
> >>>> [3] = {"register", &bm_register_operations, S_IWUSR},
> >>>> /* last one */ {""}
> >>>> };
> >>>>
> >>>> + /* create a new binfmt namespace
> >>>> + * if we are not in the first user namespace
> >>>> + * but the binfmt namespace is the first one
> >>>> + */
> >>>> + if (READ_ONCE(ns->binfmt_ns) == NULL) {
> >>>> + struct binfmt_namespace *new_ns;
> >>>> +
> >>>> + new_ns = kmalloc(sizeof(struct binfmt_namespace),
> >>>> + GFP_KERNEL);
> >>>> + if (new_ns == NULL)
> >>>> + return -ENOMEM;
> >>>> + INIT_LIST_HEAD(&new_ns->entries);
> >>>> + new_ns->enabled = 1;
> >>>> + rwlock_init(&new_ns->entries_lock);
> >>>> + new_ns->bm_mnt = NULL;
> >>>> + new_ns->entry_count = 0;
> >>>> + /* ensure new_ns is completely initialized before sharing it */
> >>>> + smp_wmb();
> >>>
> >>> (I haven't dived into patch logic, here just small barrier remark from quick sight).
> >>> smp_wmb() has no sense without paired smp_rmb() on the read side. Possible,
> >>> you want something like below in read hunk:
> >>>
> >>> + b_ns = READ_ONCE(ns->binfmt_ns);
> >>> + if (b_ns) {
> >>> + smp_rmb();
> >>> + return b_ns;
> >>> + }
> >>>
> >>>
> >>
> >> The write barrier is here to ensure the structure is fully written
> >> before we set the pointer.
> >>
> >> I don't understand how read barrier can change something at this level,
> >> IMHO the couple WRITE_ONCE()/READ_ONCE() should be enough to ensure we
> >> have correctly initialized the pointer and the structure when we read
> >> the pointer back.
> >>
> >> I think the pointer itself is the "barrier" to access the memory
> >> modified before.
> >
> > Things don't work that way on alpha, but that's why READ_ONCE()
> > includes an smp_read_barrier_depends():
> >
> > #define __READ_ONCE(x, check) \
> > ({ \
> > union { typeof(x) __val; char __c[1]; } __u; \
> > if (check) \
> > __read_once_size(&(x), __u.__c, sizeof(x)); \
> > else \
> > __read_once_size_nocheck(&(x), __u.__c, sizeof(x)); \
> > smp_read_barrier_depends(); /* Enforce dependency ordering from x */ \
> > __u.__val; \
> > })
> > #define READ_ONCE(x) __READ_ONCE(x, 1)
> >
>
> So my questions are:
>
> - do we need a smp_wmb() barrier if we use READ_ONCE() and WRITE_ONCE()?
Yes.
> - if we need an smp_wmb() barrier, do we need an smp_rmb() barrier as
> the data we want to "protect" are behind an access to the pointer?
No. You only need an smp_read_barrier_depends() to access things
behind the pointer you're reading (documented in a big comment block
in arch/alpha/include/asm/barrier.h); and that barrier is implied by
READ_ONCE(), so you don't have to write it yourself.
^ permalink raw reply
* Re: [RFC v5 1/1] ns: add binfmt_misc to the user namespace
From: Laurent Vivier @ 2018-10-09 17:22 UTC (permalink / raw)
To: Kirill Tkhai, linux-kernel@vger.kernel.org
Cc: Eric Biederman, Dmitry Safonov, linux-api@vger.kernel.org,
James Bottomley, Alexander Viro, linux-fsdevel@vger.kernel.org,
Andrei Vagin (C), containers@lists.linux-foundation.org,
Jann Horn
In-Reply-To: <7d9d7846-d153-f328-f5b4-8dc9d9705339@virtuozzo.com>
Le 09/10/2018 à 19:01, Kirill Tkhai a écrit :
> On 09.10.2018 19:45, Laurent Vivier wrote:
>> Le 09/10/2018 à 18:15, Kirill Tkhai a écrit :
>>> On 09.10.2018 13:37, Laurent Vivier wrote:
>>>> This patch allows to have a different binfmt_misc configuration
>>>> for each new user namespace. By default, the binfmt_misc configuration
>>>> is the one of the previous level, but if the binfmt_misc filesystem is
>>>> mounted in the new namespace a new empty binfmt instance is created and
>>>> used in this namespace.
>>>>
>>>> For instance, using "unshare" we can start a chroot of an another
>>>> architecture and configure the binfmt_misc interpreter without being root
>>>> to run the binaries in this chroot.
>>>>
>>>> Signed-off-by: Laurent Vivier <laurent@vivier.eu>
>>>> ---
>>>> fs/binfmt_misc.c | 106 ++++++++++++++++++++++++---------
>>>> include/linux/user_namespace.h | 13 ++++
>>>> kernel/user.c | 13 ++++
>>>> kernel/user_namespace.c | 3 +
>>>> 4 files changed, 107 insertions(+), 28 deletions(-)
>>>>
>>>> diff --git a/fs/binfmt_misc.c b/fs/binfmt_misc.c
>>>> index aa4a7a23ff99..1e0029d097d9 100644
>>>> --- a/fs/binfmt_misc.c
>>>> +++ b/fs/binfmt_misc.c
>> ...
>>>> @@ -80,18 +74,32 @@ static int entry_count;
>>>> */
>>>> #define MAX_REGISTER_LENGTH 1920
>>>>
>>>> +static struct binfmt_namespace *binfmt_ns(struct user_namespace *ns)
>>>> +{
>>>> + struct binfmt_namespace *b_ns;
>>>> +
>>>> + while (ns) {
>>>> + b_ns = READ_ONCE(ns->binfmt_ns);
>>>> + if (b_ns)
>>>> + return b_ns;
>>>> + ns = ns->parent;
>>>> + }
>>>> + WARN_ON_ONCE(1);
>>>> + return NULL;
>>>> +}
>>>> +
>> ...
>>>> @@ -823,12 +847,34 @@ static const struct super_operations s_ops = {
>>>> static int bm_fill_super(struct super_block *sb, void *data, int silent)
>>>> {
>>>> int err;
>>>> + struct user_namespace *ns = sb->s_user_ns;
>>>> static const struct tree_descr bm_files[] = {
>>>> [2] = {"status", &bm_status_operations, S_IWUSR|S_IRUGO},
>>>> [3] = {"register", &bm_register_operations, S_IWUSR},
>>>> /* last one */ {""}
>>>> };
>>>>
>>>> + /* create a new binfmt namespace
>>>> + * if we are not in the first user namespace
>>>> + * but the binfmt namespace is the first one
>>>> + */
>>>> + if (READ_ONCE(ns->binfmt_ns) == NULL) {
>>>> + struct binfmt_namespace *new_ns;
>>>> +
>>>> + new_ns = kmalloc(sizeof(struct binfmt_namespace),
>>>> + GFP_KERNEL);
>>>> + if (new_ns == NULL)
>>>> + return -ENOMEM;
>>>> + INIT_LIST_HEAD(&new_ns->entries);
>>>> + new_ns->enabled = 1;
>>>> + rwlock_init(&new_ns->entries_lock);
>>>> + new_ns->bm_mnt = NULL;
>>>> + new_ns->entry_count = 0;
>>>> + /* ensure new_ns is completely initialized before sharing it */
>>>> + smp_wmb();
>>>
>>> (I haven't dived into patch logic, here just small barrier remark from quick sight).
>>> smp_wmb() has no sense without paired smp_rmb() on the read side. Possible,
>>> you want something like below in read hunk:
>>>
>>> + b_ns = READ_ONCE(ns->binfmt_ns);
>>> + if (b_ns) {
>>> + smp_rmb();
>>> + return b_ns;
>>> + }
>>>
>>>
>>
>> The write barrier is here to ensure the structure is fully written
>> before we set the pointer.
>>
>> I don't understand how read barrier can change something at this level,
>> IMHO the couple WRITE_ONCE()/READ_ONCE() should be enough to ensure we
>> have correctly initialized the pointer and the structure when we read
>> the pointer back.
>>
>> I think the pointer itself is the "barrier" to access the memory
>> modified before.
>
> smp_rmb() guarantees you see stores in the order you want. If you have:
>
> [cpu0] [cpu1]
> new_ns->entry_count = 0;
> smp_wmb();
> WRITE_ONCE(ns->binfmt_ns, new_ns); b_ns = READ_ONCE(ns->binfmt_ns);
> smp_rmb();
> <access b_ns->entry_count>
>
> smp_rmb() guarantees you see true entry_count on the cpu1. Without
> smp_rmb() you may see old value of new_ns->entry_count.
>
> See Documentation/memory-barriers.txt
Yes, I tried to read this document several times...
What I understand from example line 1077 (7696f9910a9a
Documentation/memory-barriers.txt) is we only need a data dependency
barrier, and as explained by Jann it comes with the READ_ONCE() (and is
only needed for alpha).
Thanks,
Laurent
^ permalink raw reply
* [RFC PATCH v2] glibc: Perform rseq(2) registration at nptl init and thread creation (v2)
From: Mathieu Desnoyers @ 2018-10-09 18:02 UTC (permalink / raw)
To: Carlos O'Donell, Florian Weimer, Joseph Myers, Szabolcs Nagy
Cc: Mathieu Desnoyers, Thomas Gleixner, Ben Maurer, Peter Zijlstra,
Paul E. McKenney, Boqun Feng, Will Deacon, Dave Watson,
Paul Turner, libc-alpha, linux-kernel, linux-api
Here is a second round of prototype registering rseq(2) TLS for each
thread (including main), and unregistering for each thread (excluding
main). "rseq" stands for Restartable Sequences.
Remaining open questions:
- How early do we want to register rseq and how late do we want to
unregister it ? It's important to consider if we expect rseq to
be used by the memory allocator and within destructor callbacks.
However, we want to be sure the TLS (__thread) area is properly
allocated across its entire use by rseq.
- We do not need an atomic increment/decrement for the refcount per
se. Just being atomic with respect to the current thread (and nested
signals) would be enough. What is the proper API to use there ?
- Should we expose struct rseq_lib in a public glibc header ?
See the rseq(2) man page proposed here:
https://lkml.org/lkml/2018/9/19/647
This patch is based on glibc 2.28.
To try it out, refer to the following kernel and librseq development
branches:
* rseq and cpu_opv:
https://github.com/compudj/linux-percpu-dev branch: rseq/dev-local
* librseq:
https://github.com/compudj/librseq branch: master
TODO:
- Add documentation, tests and a NEWS entry.
- Update ABI test baselines.
- Update abilist for non-x86-64 architectures.
Signed-off-by: Mathieu Desnoyers <mathieu.desnoyers@efficios.com>
CC: Carlos O'Donell <carlos@redhat.com>
CC: Florian Weimer <fweimer@redhat.com>
CC: Joseph Myers <joseph@codesourcery.com>
CC: Szabolcs Nagy <szabolcs.nagy@arm.com>
CC: Thomas Gleixner <tglx@linutronix.de>
CC: Ben Maurer <bmaurer@fb.com>
CC: Peter Zijlstra <peterz@infradead.org>
CC: "Paul E. McKenney" <paulmck@linux.vnet.ibm.com>
CC: Boqun Feng <boqun.feng@gmail.com>
CC: Will Deacon <will.deacon@arm.com>
CC: Dave Watson <davejwatson@fb.com>
CC: Paul Turner <pjt@google.com>
CC: libc-alpha@sourceware.org
CC: linux-kernel@vger.kernel.org
CC: linux-api@vger.kernel.org
---
Changes since v1:
- Move __rseq_refcount to an extra field at the end of __rseq_abi to
eliminate one symbol.
All libraries/programs which try to register rseq (glibc,
early-adopter applications, early-adopter libraries) should use the
rseq refcount. It becomes part of the ABI within a user-space
process, but it's not part of the ABI shared with the kernel per se.
- Restructure how this code is organized so glibc keeps building on
non-Linux targets.
- Use non-weak symbol for __rseq_abi.
- Move rseq registration/unregistration implementation into its own
nptl/rseq.c compile unit.
- Move __rseq_abi symbol under GLIBC_2.29.
---
nptl/Makefile | 2 +-
nptl/Versions | 4 ++
nptl/nptl-init.c | 3 +
nptl/pthreadP.h | 3 +
nptl/pthread_create.c | 8 +++
nptl/rseq.c | 62 +++++++++++++++++
sysdeps/nptl/rseq-internal.h | 34 ++++++++++
sysdeps/unix/sysv/linux/rseq-internal.h | 68 +++++++++++++++++++
.../sysv/linux/x86_64/64/libpthread.abilist | 1 +
9 files changed, 184 insertions(+), 1 deletion(-)
create mode 100644 nptl/rseq.c
create mode 100644 sysdeps/nptl/rseq-internal.h
create mode 100644 sysdeps/unix/sysv/linux/rseq-internal.h
diff --git a/nptl/Makefile b/nptl/Makefile
index be8066524c..9def8b3f13 100644
--- a/nptl/Makefile
+++ b/nptl/Makefile
@@ -145,7 +145,7 @@ libpthread-routines = nptl-init nptlfreeres vars events version pt-interp \
mtx_destroy mtx_init mtx_lock mtx_timedlock \
mtx_trylock mtx_unlock call_once cnd_broadcast \
cnd_destroy cnd_init cnd_signal cnd_timedwait cnd_wait \
- tss_create tss_delete tss_get tss_set
+ tss_create tss_delete tss_get tss_set rseq
# pthread_setuid pthread_seteuid pthread_setreuid \
# pthread_setresuid \
# pthread_setgid pthread_setegid pthread_setregid \
diff --git a/nptl/Versions b/nptl/Versions
index e7f691da7a..f6e645c624 100644
--- a/nptl/Versions
+++ b/nptl/Versions
@@ -277,6 +277,10 @@ libpthread {
cnd_timedwait; cnd_wait; tss_create; tss_delete; tss_get; tss_set;
}
+ GLIBC_2.29 {
+ __rseq_abi;
+ }
+
GLIBC_PRIVATE {
__pthread_initialize_minimal;
__pthread_clock_gettime; __pthread_clock_settime;
diff --git a/nptl/nptl-init.c b/nptl/nptl-init.c
index 907411d5bc..ab17bbb6e4 100644
--- a/nptl/nptl-init.c
+++ b/nptl/nptl-init.c
@@ -279,6 +279,9 @@ __pthread_initialize_minimal_internal (void)
THREAD_SETMEM (pd, cpuclock_offset, GL(dl_cpuclock_offset));
#endif
+ /* Register rseq ABI to the kernel. */
+ (void) __rseq_register_current_thread ();
+
/* Initialize the robust mutex data. */
{
#if __PTHREAD_MUTEX_HAVE_PREV
diff --git a/nptl/pthreadP.h b/nptl/pthreadP.h
index 13bdb11133..aba641c170 100644
--- a/nptl/pthreadP.h
+++ b/nptl/pthreadP.h
@@ -605,6 +605,9 @@ extern void __shm_directory_freeres (void) attribute_hidden;
extern void __wait_lookup_done (void) attribute_hidden;
+extern int __rseq_register_current_thread (void) attribute_hidden;
+extern int __rseq_unregister_current_thread (void) attribute_hidden;
+
#ifdef SHARED
# define PTHREAD_STATIC_FN_REQUIRE(name)
#else
diff --git a/nptl/pthread_create.c b/nptl/pthread_create.c
index fe75d04113..a5233cdf2f 100644
--- a/nptl/pthread_create.c
+++ b/nptl/pthread_create.c
@@ -378,6 +378,7 @@ __free_tcb (struct pthread *pd)
START_THREAD_DEFN
{
struct pthread *pd = START_THREAD_SELF;
+ bool has_rseq = false;
#if HP_TIMING_AVAIL
/* Remember the time when the thread was started. */
@@ -396,6 +397,9 @@ START_THREAD_DEFN
if (__glibc_unlikely (atomic_exchange_acq (&pd->setxid_futex, 0) == -2))
futex_wake (&pd->setxid_futex, 1, FUTEX_PRIVATE);
+ /* Register rseq TLS to the kernel. */
+ has_rseq = !__rseq_register_current_thread ();
+
#ifdef __NR_set_robust_list
# ifndef __ASSUME_SET_ROBUST_LIST
if (__set_robust_list_avail >= 0)
@@ -573,6 +577,10 @@ START_THREAD_DEFN
}
#endif
+ /* Unregister rseq TLS from kernel. */
+ if (has_rseq && __rseq_unregister_current_thread ())
+ abort();
+
advise_stack_range (pd->stackblock, pd->stackblock_size, (uintptr_t) pd,
pd->guardsize);
diff --git a/nptl/rseq.c b/nptl/rseq.c
new file mode 100644
index 0000000000..81ad62e1d3
--- /dev/null
+++ b/nptl/rseq.c
@@ -0,0 +1,62 @@
+/* Copyright (C) 2018 Free Software Foundation, Inc.
+ This file is part of the GNU C Library.
+ Contributed by Mathieu Desnoyers <mathieu.desnoyers@efficios.com>, 2018.
+
+ The GNU C Library is free software; you can redistribute it and/or
+ modify it under the terms of the GNU Lesser General Public
+ License as published by the Free Software Foundation; either
+ version 2.1 of the License, or (at your option) any later version.
+
+ The GNU C Library is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public
+ License along with the GNU C Library; if not, see
+ <http://www.gnu.org/licenses/>. */
+
+#include "pthreadP.h"
+
+enum libc_rseq_cpu_id_state {
+ LIBC_RSEQ_CPU_ID_UNINITIALIZED = -1,
+ LIBC_RSEQ_CPU_ID_REGISTRATION_FAILED = -2,
+};
+
+/* linux/rseq.h defines struct rseq as aligned on 32 bytes. The kernel ABI
+ size is 20 bytes. For support of multiple rseq users within a process,
+ user-space defines an extra 4 bytes field as a reference count, for a
+ total of 24 bytes. */
+struct libc_rseq {
+ /* kernel-userspace ABI. */
+ uint32_t cpu_id_start;
+ uint32_t cpu_id;
+ uint64_t rseq_cs;
+ uint32_t flags;
+ /* user-space ABI. */
+ uint32_t refcount;
+} __attribute__((aligned(4 * sizeof(uint64_t))));
+
+__thread volatile struct libc_rseq __rseq_abi = {
+ .cpu_id = LIBC_RSEQ_CPU_ID_UNINITIALIZED,
+};
+
+#ifdef __NR_rseq
+#include <sysdeps/unix/sysv/linux/rseq-internal.h>
+#else
+#include <sysdeps/nptl/rseq-internal.h>
+#endif /* __NR_rseq. */
+
+int
+attribute_hidden
+__rseq_register_current_thread (void)
+{
+ return sysdep_rseq_register_current_thread ();
+}
+
+int
+attribute_hidden
+__rseq_unregister_current_thread (void)
+{
+ return sysdep_rseq_register_current_thread ();
+}
diff --git a/sysdeps/nptl/rseq-internal.h b/sysdeps/nptl/rseq-internal.h
new file mode 100644
index 0000000000..96422ebd57
--- /dev/null
+++ b/sysdeps/nptl/rseq-internal.h
@@ -0,0 +1,34 @@
+/* Copyright (C) 2018 Free Software Foundation, Inc.
+ This file is part of the GNU C Library.
+ Contributed by Mathieu Desnoyers <mathieu.desnoyers@efficios.com>, 2018.
+
+ The GNU C Library is free software; you can redistribute it and/or
+ modify it under the terms of the GNU Lesser General Public
+ License as published by the Free Software Foundation; either
+ version 2.1 of the License, or (at your option) any later version.
+
+ The GNU C Library is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public
+ License along with the GNU C Library; if not, see
+ <http://www.gnu.org/licenses/>. */
+
+#ifndef RSEQ_INTERNAL_H
+#define RSEQ_INTERNAL_H
+
+static inline int
+sysdep_rseq_register_current_thread (void)
+{
+ return -1;
+}
+
+static inline int
+sysdep_rseq_unregister_current_thread (void)
+{
+ return -1;
+}
+
+#endif /* rseq-internal.h */
diff --git a/sysdeps/unix/sysv/linux/rseq-internal.h b/sysdeps/unix/sysv/linux/rseq-internal.h
new file mode 100644
index 0000000000..b744aca648
--- /dev/null
+++ b/sysdeps/unix/sysv/linux/rseq-internal.h
@@ -0,0 +1,68 @@
+/* Copyright (C) 2018 Free Software Foundation, Inc.
+ This file is part of the GNU C Library.
+ Contributed by Mathieu Desnoyers <mathieu.desnoyers@efficios.com>, 2018.
+
+ The GNU C Library is free software; you can redistribute it and/or
+ modify it under the terms of the GNU Lesser General Public
+ License as published by the Free Software Foundation; either
+ version 2.1 of the License, or (at your option) any later version.
+
+ The GNU C Library is distributed in the hope that it will be useful,
+ but WITHOUT ANY WARRANTY; without even the implied warranty of
+ MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the GNU
+ Lesser General Public License for more details.
+
+ You should have received a copy of the GNU Lesser General Public
+ License along with the GNU C Library; if not, see
+ <http://www.gnu.org/licenses/>. */
+
+#ifndef RSEQ_INTERNAL_H
+#define RSEQ_INTERNAL_H
+
+#include <linux/rseq.h>
+
+#define RSEQ_SIG 0x53053053
+
+extern __thread volatile struct libc_rseq __rseq_abi
+__attribute__ ((tls_model ("initial-exec")));
+
+static inline int
+sysdep_rseq_register_current_thread (void)
+{
+ int rc, ret = 0;
+ INTERNAL_SYSCALL_DECL (err);
+
+ if (__rseq_abi.cpu_id == LIBC_RSEQ_CPU_ID_REGISTRATION_FAILED)
+ return -1;
+ if (atomic_increment_val (&__rseq_abi.refcount) - 1)
+ goto end;
+ rc = INTERNAL_SYSCALL_CALL (rseq, err, &__rseq_abi, sizeof (struct rseq),
+ 0, RSEQ_SIG);
+ if (!rc)
+ goto end;
+ if (INTERNAL_SYSCALL_ERRNO (rc, err) != EBUSY)
+ __rseq_abi.cpu_id = LIBC_RSEQ_CPU_ID_REGISTRATION_FAILED;
+ ret = -1;
+ atomic_decrement (&__rseq_abi.refcount);
+end:
+ return ret;
+}
+
+static inline int
+sysdep_rseq_unregister_current_thread (void)
+{
+ int rc, ret = 0;
+ INTERNAL_SYSCALL_DECL (err);
+
+ if (atomic_decrement_val (&__rseq_abi.refcount))
+ goto end;
+ rc = INTERNAL_SYSCALL_CALL (rseq, err, &__rseq_abi, sizeof (struct rseq),
+ RSEQ_FLAG_UNREGISTER, RSEQ_SIG);
+ if (!rc)
+ goto end;
+ ret = -1;
+end:
+ return ret;
+}
+
+#endif /* rseq-internal.h */
diff --git a/sysdeps/unix/sysv/linux/x86_64/64/libpthread.abilist b/sysdeps/unix/sysv/linux/x86_64/64/libpthread.abilist
index 931c8277a8..6d3ef80031 100644
--- a/sysdeps/unix/sysv/linux/x86_64/64/libpthread.abilist
+++ b/sysdeps/unix/sysv/linux/x86_64/64/libpthread.abilist
@@ -219,6 +219,7 @@ GLIBC_2.28 tss_create F
GLIBC_2.28 tss_delete F
GLIBC_2.28 tss_get F
GLIBC_2.28 tss_set F
+GLIBC_2.29 __rseq_abi D
GLIBC_2.3.2 pthread_cond_broadcast F
GLIBC_2.3.2 pthread_cond_destroy F
GLIBC_2.3.2 pthread_cond_init F
--
2.17.1
^ permalink raw reply related
* Re: [PATCH v2 1/3] namei: implement O_BENEATH-style AT_* flags
From: Andy Lutomirski @ 2018-10-09 19:25 UTC (permalink / raw)
To: Aleksa Sarai
Cc: Al Viro, Eric W. Biederman, Christian Brauner, Jeff Layton,
J. Bruce Fields, Arnd Bergmann, Andrew Lutomirski, David Howells,
Jann Horn, Tycho Andersen, David Drysdale, dev, Linux Containers,
Linux FS Devel, LKML, linux-arch, Linux API
In-Reply-To: <20181009065300.11053-3-cyphar@cyphar.com>
On Mon, Oct 8, 2018 at 11:53 PM Aleksa Sarai <cyphar@cyphar.com> wrote:
> * AT_NO_PROCLINK: Disallows ->get_link "symlink" jumping. This is a very
> specific restriction, and it exists because /proc/$pid/fd/...
> "symlinks" allow for access outside nd->root and pose risk to
> container runtimes that don't want to be tricked into accessing a host
> path (but do want to allow no-funny-business symlink resolution).
Can you elaborate on the use case?
If I'm set up a container namespace and walk it for real (through the
outside /proc/PID/root or otherwise starting from an fd that points
into that namespace), and I walk through that namespace's /proc, I'm
going to see the same thing that the processes in the namespace would
see. So what's the issue?
Similarly, if I somehow manage to walk into the outside /proc, then
I've pretty much lost regardless of the links.
--Andy
^ permalink raw reply
* Re: [RFC PATCH v4 21/27] x86/cet/shstk: ELF header parsing of Shadow Stack
From: Yu-cheng Yu @ 2018-10-09 21:15 UTC (permalink / raw)
To: Eugene Syromiatnikov
Cc: x86, H. Peter Anvin, Thomas Gleixner, Ingo Molnar, linux-kernel,
linux-doc, linux-mm, linux-arch, linux-api, Arnd Bergmann,
Andy Lutomirski, Balbir Singh, Cyrill Gorcunov, Dave Hansen,
Florian Weimer, H.J. Lu, Jann Horn, Jonathan Corbet, Kees Cook,
Mike Kravetz, Nadav Amit, Oleg Nesterov, Pavel Machek
In-Reply-To: <20181003232736.GI32759@asgard.redhat.com>
On Thu, 2018-10-04 at 01:27 +0200, Eugene Syromiatnikov wrote:
> On Fri, Sep 21, 2018 at 08:03:45AM -0700, Yu-cheng Yu wrote:
[...]
> > diff --git a/arch/x86/include/asm/elf.h b/arch/x86/include/asm/elf.h
> > index 0d157d2a1e2a..5b5f169c5c07 100644
> > --- a/arch/x86/include/asm/elf.h
> > +++ b/arch/x86/include/asm/elf.h
> > @@ -382,4 +382,9 @@ struct va_alignment {
> >
> > extern struct va_alignment va_align;
> > extern unsigned long align_vdso_addr(unsigned long);
> > +
> > +#ifdef CONFIG_ARCH_HAS_PROGRAM_PROPERTIES
> > +extern int arch_setup_features(void *ehdr, void *phdr, struct file *file,
> > + bool interp);
> > +#endif
> > #endif /* _ASM_X86_ELF_H */
> > diff --git a/arch/x86/include/uapi/asm/elf_property.h
> > b/arch/x86/include/uapi/asm/elf_property.h
> > new file mode 100644
> > index 000000000000..af361207718c
> > --- /dev/null
> > +++ b/arch/x86/include/uapi/asm/elf_property.h
> > @@ -0,0 +1,15 @@
> > +/* SPDX-License-Identifier: GPL-2.0 */
> > +#ifndef _UAPI_ASM_X86_ELF_PROPERTY_H
> > +#define _UAPI_ASM_X86_ELF_PROPERTY_H
> > +
> > +/*
> > + * pr_type
> > + */
> > +#define GNU_PROPERTY_X86_FEATURE_1_AND (0xc0000002)
> > +
> > +/*
> > + * Bits for GNU_PROPERTY_X86_FEATURE_1_AND
> > + */
> > +#define GNU_PROPERTY_X86_FEATURE_1_SHSTK (0x00000002)
>
> Hm, these defeinitions aren't much different comparing to NT_*
> definitions in include/uapi/linux/elf.h, is it expected that those
> properties have to be parsed individually for each architecture?
Yes, we have NT_GNU_PROPERTY_TYPE_0 defined in include/uapi/linux/elf.h.
GNU_PROPERTY_X86_FEATURE_1_xxxx is for X86 only.
[...]
>
> There's a lot of similar code with bpf stackmap .build-id code (commit
> v4.17-rc1~148^2~156^2~3^2~1), it might be worthy generalising some ELF
> traversal routines, since there's general need of parsing ELF property
> segments.
Only a small similarity exists. The routine find_note_type_0() does a lot more
validation. It appears stack_map_get_build_id() does not need that.
Yu-cheng
^ permalink raw reply
* Re: [PATCH v2 1/3] namei: implement O_BENEATH-style AT_* flags
From: Aleksa Sarai @ 2018-10-10 7:07 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Al Viro, Eric W. Biederman, Christian Brauner, Jeff Layton,
J. Bruce Fields, Arnd Bergmann, David Howells, Jann Horn,
Tycho Andersen, David Drysdale, dev, Linux Containers,
Linux FS Devel, LKML, linux-arch, Linux API
In-Reply-To: <CALCETrUZAYw-g+75WAijS+fmRA_DrTahpv156WrxgYuX3KX2xw@mail.gmail.com>
[-- Attachment #1: Type: text/plain, Size: 3338 bytes --]
On 2018-10-09, Andy Lutomirski <luto@kernel.org> wrote:
> On Mon, Oct 8, 2018 at 11:53 PM Aleksa Sarai <cyphar@cyphar.com> wrote:
> > * AT_NO_PROCLINK: Disallows ->get_link "symlink" jumping. This is a very
> > specific restriction, and it exists because /proc/$pid/fd/...
> > "symlinks" allow for access outside nd->root and pose risk to
> > container runtimes that don't want to be tricked into accessing a host
> > path (but do want to allow no-funny-business symlink resolution).
>
> Can you elaborate on the use case?
>
> If I'm set up a container namespace and walk it for real (through the
> outside /proc/PID/root or otherwise starting from an fd that points
> into that namespace), and I walk through that namespace's /proc, I'm
> going to see the same thing that the processes in the namespace would
> see. So what's the issue?
>
> Similarly, if I somehow manage to walk into the outside /proc, then
> I've pretty much lost regardless of the links.
Well, there's a couple of reasons:
* The original AT_NO_JUMPS patchset similarly disabled "proclinks" but
it was sort of all contained within AT_NO_JUMPS. In order to have a
precise 1:1 feature mapping we need this in *some* form (and in v1 the
only way to get it was to add a separate flag). According to the
original O_BENEATH changelog, both you and Al pushed for this to be
part of O_BENEATH. :P
*However* in v2 of the patchset, proclinks are also disabled by
AT_BENEATH (because it's not really safe or consistent to allow them
at the moment -- we'd need to add __d_path checks when jumping through
them as well if we wanted them to be consistent) -- so the need for
this flag (purely for AT_NO_JUMPS compatibility) is reduced.
* There were cases in the past where races caused (temporarily)
something like /proc/self/exe (or a file descriptor referencing the
host filesystem) to be exposed into a container -- but because of
set_dumpable they were blocked. CVE-2016-9962 was an example of this
(it wasn't blocked by set_dumpable -- but the fix used set_dumpable).
In those cases, if you can trick a host-side process to open that
procfs file through a symlink/bind-mount (which is technically
"accessible" but not actually usable by the container process), you
can trick the resolution to resolve the host filesystem (and this
might be a file which is unlinked and thus there's no way for __d_path
checking to verify whether it is safe or not).
I think that AT_BENEATH allowing only proclinks that result in you
being under the root is something we might want in the future, but I
think there are some cases where you want to be _very_ sure you don't
follow a proclink (now or in the future).
* And finally, some containers run with the host's pidns. This is not a
usecase that I'm particularly fond of, but some folks do use this (as
far as I'm aware this is one of the reasons why the subreaper concept
exists). In those cases, the procfs mount would be able to see the
host processes -- and thus /proc/self would resolve (as would the
host's init and so on).
I will admit that this flag is more paranoid than the others though.
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [RFC v5 1/1] ns: add binfmt_misc to the user namespace
From: Aleksa Sarai @ 2018-10-10 7:14 UTC (permalink / raw)
Cc: Tycho Andersen, Jann Horn, linux-api, containers, Dmitry Safonov,
linux-kernel, James Bottomley, Eric Biederman, linux-fsdevel,
Alexander Viro
In-Reply-To: <409c22e3-1df8-cf7f-2462-ead2bb3020cf@vivier.eu>
[-- Attachment #1: Type: text/plain, Size: 1610 bytes --]
On 2018-10-09, Laurent Vivier <laurent@vivier.eu> wrote:
> Le 09/10/2018 à 17:16, Tycho Andersen a écrit :
> > On Tue, Oct 09, 2018 at 12:37:52PM +0200, Laurent Vivier wrote:
> >> @@ -80,18 +74,32 @@ static int entry_count;
> >> */
> >> #define MAX_REGISTER_LENGTH 1920
> >>
> >> +static struct binfmt_namespace *binfmt_ns(struct user_namespace *ns)
> >> +{
> >> + struct binfmt_namespace *b_ns;
> >> +
> >> + while (ns) {
> >> + b_ns = READ_ONCE(ns->binfmt_ns);
> >> + if (b_ns)
> >> + return b_ns;
> >> + ns = ns->parent;
> >> + }
> >> + WARN_ON_ONCE(1);
> >
> > It looks like we warn here,
> >
> >> @@ -133,17 +141,18 @@ static int load_misc_binary(struct linux_binprm *bprm)
> >> struct file *interp_file = NULL;
> >> int retval;
> >> int fd_binary = -1;
> >> + struct binfmt_namespace *ns = binfmt_ns(current_user_ns());
> >>
> >> retval = -ENOEXEC;
> >> - if (!enabled)
> >> + if (!ns->enabled)
> >
> > ...but then in cases like this we immediately dereference the pointer
> > anyways and crash. Can we return some other error code here in the !ns
> > case so we don't crash?
>
> My concern here is I don't want to add code to check an error case that
> cannot happen. The first namespace binfmt_ns pointer is initialized with
> &init_binfmt_ns, so the return value cannot be NULL.
I'd argue that BUG() is a better thing to do then -- if doing a dummy
error path makes no sense. Though IIRC BUG() is no longer a popular
thing to do.
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [PATCH v2 1/3] namei: implement O_BENEATH-style AT_* flags
From: Aleksa Sarai @ 2018-10-10 7:28 UTC (permalink / raw)
To: Andy Lutomirski
Cc: Al Viro, Eric W. Biederman, Christian Brauner, Jeff Layton,
J. Bruce Fields, Arnd Bergmann, David Howells, Jann Horn,
Tycho Andersen, David Drysdale, dev, Linux Containers,
Linux FS Devel, LKML, linux-arch, Linux API
In-Reply-To: <20181010070747.byi2itbi4j42gynq@ryuk>
[-- Attachment #1: Type: text/plain, Size: 2031 bytes --]
On 2018-10-10, Aleksa Sarai <cyphar@cyphar.com> wrote:
> On 2018-10-09, Andy Lutomirski <luto@kernel.org> wrote:
> > On Mon, Oct 8, 2018 at 11:53 PM Aleksa Sarai <cyphar@cyphar.com> wrote:
> > > * AT_NO_PROCLINK: Disallows ->get_link "symlink" jumping. This is a very
> > > specific restriction, and it exists because /proc/$pid/fd/...
> > > "symlinks" allow for access outside nd->root and pose risk to
> > > container runtimes that don't want to be tricked into accessing a host
> > > path (but do want to allow no-funny-business symlink resolution).
> >
> > Can you elaborate on the use case?
> >
> [...]
> I think that AT_BENEATH allowing only proclinks that result in you
> being under the root is something we might want in the future, but I
> think there are some cases where you want to be _very_ sure you don't
> follow a proclink (now or in the future).
> [...]
Sorry, just to clarify this point a bit more.
At the moment, "proclinks" are entirely disabled with AT_BENEATH. This
is a (hopefully) temporary measure until it's decided _how_ they should
be allowed. Personally I think we should allow them if they follow the
same requirement as ".." escapes (that __d_path can resolve them).
But then the question arises -- what if we're looking at a never-mounted
pseudo-filesystem dentry (see the ->d_dname code in d_path)? If we don't
allow it then we'd probably disallow quite a few cases where you'd want
to allow access (nsfs proclinks come immediately to mind).
*But* if we allow it then there's no real way to tell if the container
process has tricked us into opening something we shouldn't (like an open
file descriptor to a memfd or pipe related to some host service). Maybe
we should still allow them in that case because the likelihood of such a
case is very small (and allowing them would let you open nsfs links with
AT_BENEATH), but I'm not sure.
--
Aleksa Sarai
Senior Software Engineer (Containers)
SUSE Linux GmbH
<https://www.cyphar.com/>
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]
^ permalink raw reply
* Re: [RFC v5 1/1] ns: add binfmt_misc to the user namespace
From: Laurent Vivier @ 2018-10-10 10:11 UTC (permalink / raw)
To: Tycho Andersen
Cc: linux-kernel, Dmitry Safonov, linux-api, containers, Jann Horn,
James Bottomley, Eric Biederman, linux-fsdevel, Alexander Viro
In-Reply-To: <409c22e3-1df8-cf7f-2462-ead2bb3020cf@vivier.eu>
On 09/10/2018 17:19, Laurent Vivier wrote:
> Le 09/10/2018 à 17:16, Tycho Andersen a écrit :
>> On Tue, Oct 09, 2018 at 12:37:52PM +0200, Laurent Vivier wrote:
>>> @@ -80,18 +74,32 @@ static int entry_count;
>>> */
>>> #define MAX_REGISTER_LENGTH 1920
>>>
>>> +static struct binfmt_namespace *binfmt_ns(struct user_namespace *ns)
>>> +{
>>> + struct binfmt_namespace *b_ns;
>>> +
>>> + while (ns) {
>>> + b_ns = READ_ONCE(ns->binfmt_ns);
>>> + if (b_ns)
>>> + return b_ns;
>>> + ns = ns->parent;
>>> + }
>>> + WARN_ON_ONCE(1);
>>
>> It looks like we warn here,
>>
>>> @@ -133,17 +141,18 @@ static int load_misc_binary(struct linux_binprm *bprm)
>>> struct file *interp_file = NULL;
>>> int retval;
>>> int fd_binary = -1;
>>> + struct binfmt_namespace *ns = binfmt_ns(current_user_ns());
>>>
>>> retval = -ENOEXEC;
>>> - if (!enabled)
>>> + if (!ns->enabled)
>>
>> ...but then in cases like this we immediately dereference the pointer
>> anyways and crash. Can we return some other error code here in the !ns
>> case so we don't crash?
>
> My concern here is I don't want to add code to check an error case that
> cannot happen. The first namespace binfmt_ns pointer is initialized with
> &init_binfmt_ns, so the return value cannot be NULL.
Perhaps it could be reasonable to return &init_binfmt_ns rather than
NULL in this case?
Thanks,
Laurent
^ permalink raw reply
* Re: [PATCH 0/5] VFS: Introduce filesystem information query syscall [ver #12]
From: David Howells @ 2018-10-10 11:58 UTC (permalink / raw)
To: Rasmus Villemoes
Cc: dhowells, viro, linux-api, torvalds, linux-fsdevel, linux-kernel,
mszeredi
In-Reply-To: <f3646774-ee9e-d5b7-8a11-670012034d59@rasmusvillemoes.dk>
Rasmus Villemoes <linux@rasmusvillemoes.dk> wrote:
> Would you consider adding a Kconfig knob to not include all this (and
> all the extra tricks that syscall might learn down the road)?
I guess that can be done, though the bits are spread out into various
filesystems.
David
^ permalink raw reply
* Re: [PATCH v7 3/6] seccomp: add a way to get a listener fd from ptrace
From: Christian Brauner @ 2018-10-10 12:54 UTC (permalink / raw)
To: Jann Horn
Cc: Tycho Andersen, Kees Cook, Linux API, containers, suda.akihiro,
Oleg Nesterov, kernel list, Eric W. Biederman, linux-fsdevel,
Christian Brauner, Andy Lutomirski, linux-security-module,
selinux, Paul Moore, Stephen Smalley, Eric Paris
In-Reply-To: <CAG48ez2ROhq4R1az2TXNmBvCXY=3UVxnmh6CeR05N0xVGrpwpA@mail.gmail.com>
On Tue, Oct 09, 2018 at 06:26:47PM +0200, Jann Horn wrote:
> On Tue, Oct 9, 2018 at 6:20 PM Christian Brauner <christian@brauner.io> wrote:
> > On Tue, Oct 09, 2018 at 05:26:26PM +0200, Jann Horn wrote:
> > > On Tue, Oct 9, 2018 at 4:09 PM Christian Brauner <christian@brauner.io> wrote:
> > > > On Tue, Oct 09, 2018 at 03:50:53PM +0200, Jann Horn wrote:
> > > > > On Tue, Oct 9, 2018 at 3:49 PM Christian Brauner <christian@brauner.io> wrote:
> > > > > > On Tue, Oct 09, 2018 at 03:36:04PM +0200, Jann Horn wrote:
> > > > > > > On Tue, Oct 9, 2018 at 3:29 PM Christian Brauner <christian@brauner.io> wrote:
> > > > > > > > One more thing. Citing from [1]
> > > > > > > >
> > > > > > > > > I think there's a security problem here. Imagine the following scenario:
> > > > > > > > >
> > > > > > > > > 1. task A (uid==0) sets up a seccomp filter that uses SECCOMP_RET_USER_NOTIF
> > > > > > > > > 2. task A forks off a child B
> > > > > > > > > 3. task B uses setuid(1) to drop its privileges
> > > > > > > > > 4. task B becomes dumpable again, either via prctl(PR_SET_DUMPABLE, 1)
> > > > > > > > > or via execve()
> > > > > > > > > 5. task C (the attacker, uid==1) attaches to task B via ptrace
> > > > > > > > > 6. task C uses PTRACE_SECCOMP_NEW_LISTENER on task B
> > > > > > > >
> > > > > > > > Sorry, to be late to the party but would this really pass
> > > > > > > > __ptrace_may_access() in ptrace_attach()? It doesn't seem obvious to me
> > > > > > > > that it would... Doesn't look like it would get past:
> > > > > > > >
> > > > > > > > tcred = __task_cred(task);
> > > > > > > > if (uid_eq(caller_uid, tcred->euid) &&
> > > > > > > > uid_eq(caller_uid, tcred->suid) &&
> > > > > > > > uid_eq(caller_uid, tcred->uid) &&
> > > > > > > > gid_eq(caller_gid, tcred->egid) &&
> > > > > > > > gid_eq(caller_gid, tcred->sgid) &&
> > > > > > > > gid_eq(caller_gid, tcred->gid))
> > > > > > > > goto ok;
> > > > > > > > if (ptrace_has_cap(tcred->user_ns, mode))
> > > > > > > > goto ok;
> > > > > > > > rcu_read_unlock();
> > > > > > > > return -EPERM;
> > > > > > > > ok:
> > > > > > > > rcu_read_unlock();
> > > > > > > > mm = task->mm;
> > > > > > > > if (mm &&
> > > > > > > > ((get_dumpable(mm) != SUID_DUMP_USER) &&
> > > > > > > > !ptrace_has_cap(mm->user_ns, mode)))
> > > > > > > > return -EPERM;
> > > > > > >
> > > > > > > Which specific check would prevent task C from attaching to task B? If
> > > > > > > the UIDs match, the first "goto ok" executes; and you're dumpable, so
> > > > > > > you don't trigger the second "return -EPERM".
> > > > > >
> > > > > > You'd also need CAP_SYS_PTRACE in the mm->user_ns which you shouldn't
> > > > > > have if you did a setuid to an unpriv user. (But I always find that code
> > > > > > confusing.)
> > > > >
> > > > > Only if the target hasn't gone through execve() since setuid().
> > > >
> > > > Sorry if I want to know this in excessive detail but I'd like to
> > > > understand this properly so bear with me :)
> > > > - If task B has setuid()ed and prctl(PR_SET_DUMPABLE, 1)ed but not
> > > > execve()ed then C won't pass ptrace_has_cap(mm->user_ns, mode).
> > >
> > > Yeah.
> > >
> > > > - If task B has setuid()ed, exeved()ed it will get its dumpable flag set
> > > > to /proc/sys/fs/suid_dumpable
> > >
> > > Not if you changed all UIDs (e.g. by calling setuid() as root). In
> > > that case, setup_new_exec() calls "set_dumpable(current->mm,
> > > SUID_DUMP_USER)".
> >
> > Actually, looking at this when C is trying to PTRACE_ATTACH to B as an
> > unprivileged user even if B execve()ed and it is dumpable C still
> > wouldn't have CAP_SYS_PTRACE in the mm->user_ns unless it already is
> > privileged over mm->user_ns which means it must be in an ancestor
> > user_ns.
>
> Huh? Why would you need CAP_SYS_PTRACE for anything here? You can
> ptrace another process running under your UID just fine, no matter
> what the namespaces are. I'm not sure what you're saying.
Sorry, I was out the door yesterday when answering this and was too
brief. I forgot to mention: /proc/sys/kernel/yama/ptrace_scope. It
should be enabled by default on nearly all distros and even if not -
which is an administrators choice - you can usually easily enable it via
sysctl.
1 ("restricted ptrace") [default value]
When performing an operation that requires a PTRACE_MODE_ATTACH check,
the calling process must either have the CAP_SYS_PTRACE capability in
the user namespace of the target process or it must have a prede‐ fined
relationship with the target process. By default, the predefined
relationship is that the target process must be a descendant of the
caller.
If you don't have it set you're already susceptible to all kinds of
other attacks and I'm still not convinced we need to bring out the big
capable(CAP_SYS_ADMIN) gun here.
^ permalink raw reply
* Re: [PATCH v7 3/6] seccomp: add a way to get a listener fd from ptrace
From: Christian Brauner @ 2018-10-10 13:09 UTC (permalink / raw)
To: Jann Horn
Cc: Tycho Andersen, Kees Cook, Linux API, containers, suda.akihiro,
Oleg Nesterov, kernel list, Eric W. Biederman, linux-fsdevel,
Christian Brauner, Andy Lutomirski, linux-security-module,
selinux, Paul Moore, Stephen Smalley, Eric Paris
In-Reply-To: <20181010125422.rouslofknxzvygzr@brauner.io>
On Wed, Oct 10, 2018 at 02:54:22PM +0200, Christian Brauner wrote:
> On Tue, Oct 09, 2018 at 06:26:47PM +0200, Jann Horn wrote:
> > On Tue, Oct 9, 2018 at 6:20 PM Christian Brauner <christian@brauner.io> wrote:
> > > On Tue, Oct 09, 2018 at 05:26:26PM +0200, Jann Horn wrote:
> > > > On Tue, Oct 9, 2018 at 4:09 PM Christian Brauner <christian@brauner.io> wrote:
> > > > > On Tue, Oct 09, 2018 at 03:50:53PM +0200, Jann Horn wrote:
> > > > > > On Tue, Oct 9, 2018 at 3:49 PM Christian Brauner <christian@brauner.io> wrote:
> > > > > > > On Tue, Oct 09, 2018 at 03:36:04PM +0200, Jann Horn wrote:
> > > > > > > > On Tue, Oct 9, 2018 at 3:29 PM Christian Brauner <christian@brauner.io> wrote:
> > > > > > > > > One more thing. Citing from [1]
> > > > > > > > >
> > > > > > > > > > I think there's a security problem here. Imagine the following scenario:
> > > > > > > > > >
> > > > > > > > > > 1. task A (uid==0) sets up a seccomp filter that uses SECCOMP_RET_USER_NOTIF
> > > > > > > > > > 2. task A forks off a child B
> > > > > > > > > > 3. task B uses setuid(1) to drop its privileges
> > > > > > > > > > 4. task B becomes dumpable again, either via prctl(PR_SET_DUMPABLE, 1)
> > > > > > > > > > or via execve()
> > > > > > > > > > 5. task C (the attacker, uid==1) attaches to task B via ptrace
> > > > > > > > > > 6. task C uses PTRACE_SECCOMP_NEW_LISTENER on task B
> > > > > > > > >
> > > > > > > > > Sorry, to be late to the party but would this really pass
> > > > > > > > > __ptrace_may_access() in ptrace_attach()? It doesn't seem obvious to me
> > > > > > > > > that it would... Doesn't look like it would get past:
> > > > > > > > >
> > > > > > > > > tcred = __task_cred(task);
> > > > > > > > > if (uid_eq(caller_uid, tcred->euid) &&
> > > > > > > > > uid_eq(caller_uid, tcred->suid) &&
> > > > > > > > > uid_eq(caller_uid, tcred->uid) &&
> > > > > > > > > gid_eq(caller_gid, tcred->egid) &&
> > > > > > > > > gid_eq(caller_gid, tcred->sgid) &&
> > > > > > > > > gid_eq(caller_gid, tcred->gid))
> > > > > > > > > goto ok;
> > > > > > > > > if (ptrace_has_cap(tcred->user_ns, mode))
> > > > > > > > > goto ok;
> > > > > > > > > rcu_read_unlock();
> > > > > > > > > return -EPERM;
> > > > > > > > > ok:
> > > > > > > > > rcu_read_unlock();
> > > > > > > > > mm = task->mm;
> > > > > > > > > if (mm &&
> > > > > > > > > ((get_dumpable(mm) != SUID_DUMP_USER) &&
> > > > > > > > > !ptrace_has_cap(mm->user_ns, mode)))
> > > > > > > > > return -EPERM;
> > > > > > > >
> > > > > > > > Which specific check would prevent task C from attaching to task B? If
> > > > > > > > the UIDs match, the first "goto ok" executes; and you're dumpable, so
> > > > > > > > you don't trigger the second "return -EPERM".
> > > > > > >
> > > > > > > You'd also need CAP_SYS_PTRACE in the mm->user_ns which you shouldn't
> > > > > > > have if you did a setuid to an unpriv user. (But I always find that code
> > > > > > > confusing.)
> > > > > >
> > > > > > Only if the target hasn't gone through execve() since setuid().
> > > > >
> > > > > Sorry if I want to know this in excessive detail but I'd like to
> > > > > understand this properly so bear with me :)
> > > > > - If task B has setuid()ed and prctl(PR_SET_DUMPABLE, 1)ed but not
> > > > > execve()ed then C won't pass ptrace_has_cap(mm->user_ns, mode).
> > > >
> > > > Yeah.
> > > >
> > > > > - If task B has setuid()ed, exeved()ed it will get its dumpable flag set
> > > > > to /proc/sys/fs/suid_dumpable
> > > >
> > > > Not if you changed all UIDs (e.g. by calling setuid() as root). In
> > > > that case, setup_new_exec() calls "set_dumpable(current->mm,
> > > > SUID_DUMP_USER)".
> > >
> > > Actually, looking at this when C is trying to PTRACE_ATTACH to B as an
> > > unprivileged user even if B execve()ed and it is dumpable C still
> > > wouldn't have CAP_SYS_PTRACE in the mm->user_ns unless it already is
> > > privileged over mm->user_ns which means it must be in an ancestor
> > > user_ns.
> >
> > Huh? Why would you need CAP_SYS_PTRACE for anything here? You can
> > ptrace another process running under your UID just fine, no matter
> > what the namespaces are. I'm not sure what you're saying.
>
> Sorry, I was out the door yesterday when answering this and was too
> brief. I forgot to mention: /proc/sys/kernel/yama/ptrace_scope. It
> should be enabled by default on nearly all distros and even if not -
> which is an administrators choice - you can usually easily enable it via
> sysctl.
>
> 1 ("restricted ptrace") [default value]
> When performing an operation that requires a PTRACE_MODE_ATTACH check,
> the calling process must either have the CAP_SYS_PTRACE capability in
> the user namespace of the target process or it must have a prede‐ fined
> relationship with the target process. By default, the predefined
> relationship is that the target process must be a descendant of the
> caller.
>
> If you don't have it set you're already susceptible to all kinds of
> other attacks and I'm still not convinced we need to bring out the big
> capable(CAP_SYS_ADMIN) gun here.
That being said, given that Tycho agreed to leave in the native
seccomp() way of retrieving an fd from the task without the sys_admin
restriction [1] which we prefer and if we merge it with aforementioned
feature I care way less about whether or not the restriction is upheld
for ptrace() or not.
[1]: https://lists.linuxfoundation.org/pipermail/containers/2018-October/039553.html
^ permalink raw reply
* Re: [PATCH v7 3/6] seccomp: add a way to get a listener fd from ptrace
From: Jann Horn @ 2018-10-10 13:10 UTC (permalink / raw)
To: christian
Cc: Tycho Andersen, Kees Cook, Linux API, containers, suda.akihiro,
Oleg Nesterov, kernel list, Eric W. Biederman, linux-fsdevel,
Christian Brauner, Andy Lutomirski, linux-security-module,
selinux, Paul Moore, Stephen Smalley, Eric Paris
In-Reply-To: <20181010125422.rouslofknxzvygzr@brauner.io>
On Wed, Oct 10, 2018 at 2:54 PM Christian Brauner <christian@brauner.io> wrote:
> On Tue, Oct 09, 2018 at 06:26:47PM +0200, Jann Horn wrote:
> > On Tue, Oct 9, 2018 at 6:20 PM Christian Brauner <christian@brauner.io> wrote:
> > > On Tue, Oct 09, 2018 at 05:26:26PM +0200, Jann Horn wrote:
> > > > On Tue, Oct 9, 2018 at 4:09 PM Christian Brauner <christian@brauner.io> wrote:
> > > > > On Tue, Oct 09, 2018 at 03:50:53PM +0200, Jann Horn wrote:
> > > > > > On Tue, Oct 9, 2018 at 3:49 PM Christian Brauner <christian@brauner.io> wrote:
> > > > > > > On Tue, Oct 09, 2018 at 03:36:04PM +0200, Jann Horn wrote:
> > > > > > > > On Tue, Oct 9, 2018 at 3:29 PM Christian Brauner <christian@brauner.io> wrote:
> > > > > > > > > One more thing. Citing from [1]
> > > > > > > > >
> > > > > > > > > > I think there's a security problem here. Imagine the following scenario:
> > > > > > > > > >
> > > > > > > > > > 1. task A (uid==0) sets up a seccomp filter that uses SECCOMP_RET_USER_NOTIF
> > > > > > > > > > 2. task A forks off a child B
> > > > > > > > > > 3. task B uses setuid(1) to drop its privileges
> > > > > > > > > > 4. task B becomes dumpable again, either via prctl(PR_SET_DUMPABLE, 1)
> > > > > > > > > > or via execve()
> > > > > > > > > > 5. task C (the attacker, uid==1) attaches to task B via ptrace
> > > > > > > > > > 6. task C uses PTRACE_SECCOMP_NEW_LISTENER on task B
> > > > > > > > >
> > > > > > > > > Sorry, to be late to the party but would this really pass
> > > > > > > > > __ptrace_may_access() in ptrace_attach()? It doesn't seem obvious to me
> > > > > > > > > that it would... Doesn't look like it would get past:
> > > > > > > > >
> > > > > > > > > tcred = __task_cred(task);
> > > > > > > > > if (uid_eq(caller_uid, tcred->euid) &&
> > > > > > > > > uid_eq(caller_uid, tcred->suid) &&
> > > > > > > > > uid_eq(caller_uid, tcred->uid) &&
> > > > > > > > > gid_eq(caller_gid, tcred->egid) &&
> > > > > > > > > gid_eq(caller_gid, tcred->sgid) &&
> > > > > > > > > gid_eq(caller_gid, tcred->gid))
> > > > > > > > > goto ok;
> > > > > > > > > if (ptrace_has_cap(tcred->user_ns, mode))
> > > > > > > > > goto ok;
> > > > > > > > > rcu_read_unlock();
> > > > > > > > > return -EPERM;
> > > > > > > > > ok:
> > > > > > > > > rcu_read_unlock();
> > > > > > > > > mm = task->mm;
> > > > > > > > > if (mm &&
> > > > > > > > > ((get_dumpable(mm) != SUID_DUMP_USER) &&
> > > > > > > > > !ptrace_has_cap(mm->user_ns, mode)))
> > > > > > > > > return -EPERM;
> > > > > > > >
> > > > > > > > Which specific check would prevent task C from attaching to task B? If
> > > > > > > > the UIDs match, the first "goto ok" executes; and you're dumpable, so
> > > > > > > > you don't trigger the second "return -EPERM".
> > > > > > >
> > > > > > > You'd also need CAP_SYS_PTRACE in the mm->user_ns which you shouldn't
> > > > > > > have if you did a setuid to an unpriv user. (But I always find that code
> > > > > > > confusing.)
> > > > > >
> > > > > > Only if the target hasn't gone through execve() since setuid().
> > > > >
> > > > > Sorry if I want to know this in excessive detail but I'd like to
> > > > > understand this properly so bear with me :)
> > > > > - If task B has setuid()ed and prctl(PR_SET_DUMPABLE, 1)ed but not
> > > > > execve()ed then C won't pass ptrace_has_cap(mm->user_ns, mode).
> > > >
> > > > Yeah.
> > > >
> > > > > - If task B has setuid()ed, exeved()ed it will get its dumpable flag set
> > > > > to /proc/sys/fs/suid_dumpable
> > > >
> > > > Not if you changed all UIDs (e.g. by calling setuid() as root). In
> > > > that case, setup_new_exec() calls "set_dumpable(current->mm,
> > > > SUID_DUMP_USER)".
> > >
> > > Actually, looking at this when C is trying to PTRACE_ATTACH to B as an
> > > unprivileged user even if B execve()ed and it is dumpable C still
> > > wouldn't have CAP_SYS_PTRACE in the mm->user_ns unless it already is
> > > privileged over mm->user_ns which means it must be in an ancestor
> > > user_ns.
> >
> > Huh? Why would you need CAP_SYS_PTRACE for anything here? You can
> > ptrace another process running under your UID just fine, no matter
> > what the namespaces are. I'm not sure what you're saying.
>
> Sorry, I was out the door yesterday when answering this and was too
> brief. I forgot to mention: /proc/sys/kernel/yama/ptrace_scope. It
> should be enabled by default on nearly all distros
"nearly all distros"? AFAIK it's off on Debian, for starters. And Yama
still doesn't help you if one of the tasks enters a new user namespace
or whatever.
Yama is a little bit of extra, heuristic, **opt-in** hardening enabled
in some configurations. It is **not** a fundamental building block you
can rely on.
> and even if not -
> which is an administrators choice - you can usually easily enable it via
> sysctl.
Opt-in security isn't good enough. Kernel interfaces should still be
safe to use even on a system that has all the LSM stuff disabled in
the kernel config.
> 1 ("restricted ptrace") [default value]
> When performing an operation that requires a PTRACE_MODE_ATTACH check,
> the calling process must either have the CAP_SYS_PTRACE capability in
> the user namespace of the target process or it must have a prede‐ fined
> relationship with the target process. By default, the predefined
> relationship is that the target process must be a descendant of the
> caller.
>
> If you don't have it set you're already susceptible to all kinds of
> other attacks
Oh? Can you be more specific, please?
> and I'm still not convinced we need to bring out the big
> capable(CAP_SYS_ADMIN) gun here.
^ permalink raw reply
* Re: [PATCH v7 3/6] seccomp: add a way to get a listener fd from ptrace
From: Christian Brauner @ 2018-10-10 13:18 UTC (permalink / raw)
To: Jann Horn
Cc: Tycho Andersen, Kees Cook, Linux API, containers, suda.akihiro,
Oleg Nesterov, kernel list, Eric W. Biederman, linux-fsdevel,
Christian Brauner, Andy Lutomirski, linux-security-module,
selinux, Paul Moore, Stephen Smalley, Eric Paris
In-Reply-To: <CAG48ez0ct7_i1U0=vr1Z=4WhVH7GQKFzhtuxR+1oknmPiDTqKg@mail.gmail.com>
On Wed, Oct 10, 2018 at 03:10:11PM +0200, Jann Horn wrote:
> On Wed, Oct 10, 2018 at 2:54 PM Christian Brauner <christian@brauner.io> wrote:
> > On Tue, Oct 09, 2018 at 06:26:47PM +0200, Jann Horn wrote:
> > > On Tue, Oct 9, 2018 at 6:20 PM Christian Brauner <christian@brauner.io> wrote:
> > > > On Tue, Oct 09, 2018 at 05:26:26PM +0200, Jann Horn wrote:
> > > > > On Tue, Oct 9, 2018 at 4:09 PM Christian Brauner <christian@brauner.io> wrote:
> > > > > > On Tue, Oct 09, 2018 at 03:50:53PM +0200, Jann Horn wrote:
> > > > > > > On Tue, Oct 9, 2018 at 3:49 PM Christian Brauner <christian@brauner.io> wrote:
> > > > > > > > On Tue, Oct 09, 2018 at 03:36:04PM +0200, Jann Horn wrote:
> > > > > > > > > On Tue, Oct 9, 2018 at 3:29 PM Christian Brauner <christian@brauner.io> wrote:
> > > > > > > > > > One more thing. Citing from [1]
> > > > > > > > > >
> > > > > > > > > > > I think there's a security problem here. Imagine the following scenario:
> > > > > > > > > > >
> > > > > > > > > > > 1. task A (uid==0) sets up a seccomp filter that uses SECCOMP_RET_USER_NOTIF
> > > > > > > > > > > 2. task A forks off a child B
> > > > > > > > > > > 3. task B uses setuid(1) to drop its privileges
> > > > > > > > > > > 4. task B becomes dumpable again, either via prctl(PR_SET_DUMPABLE, 1)
> > > > > > > > > > > or via execve()
> > > > > > > > > > > 5. task C (the attacker, uid==1) attaches to task B via ptrace
> > > > > > > > > > > 6. task C uses PTRACE_SECCOMP_NEW_LISTENER on task B
> > > > > > > > > >
> > > > > > > > > > Sorry, to be late to the party but would this really pass
> > > > > > > > > > __ptrace_may_access() in ptrace_attach()? It doesn't seem obvious to me
> > > > > > > > > > that it would... Doesn't look like it would get past:
> > > > > > > > > >
> > > > > > > > > > tcred = __task_cred(task);
> > > > > > > > > > if (uid_eq(caller_uid, tcred->euid) &&
> > > > > > > > > > uid_eq(caller_uid, tcred->suid) &&
> > > > > > > > > > uid_eq(caller_uid, tcred->uid) &&
> > > > > > > > > > gid_eq(caller_gid, tcred->egid) &&
> > > > > > > > > > gid_eq(caller_gid, tcred->sgid) &&
> > > > > > > > > > gid_eq(caller_gid, tcred->gid))
> > > > > > > > > > goto ok;
> > > > > > > > > > if (ptrace_has_cap(tcred->user_ns, mode))
> > > > > > > > > > goto ok;
> > > > > > > > > > rcu_read_unlock();
> > > > > > > > > > return -EPERM;
> > > > > > > > > > ok:
> > > > > > > > > > rcu_read_unlock();
> > > > > > > > > > mm = task->mm;
> > > > > > > > > > if (mm &&
> > > > > > > > > > ((get_dumpable(mm) != SUID_DUMP_USER) &&
> > > > > > > > > > !ptrace_has_cap(mm->user_ns, mode)))
> > > > > > > > > > return -EPERM;
> > > > > > > > >
> > > > > > > > > Which specific check would prevent task C from attaching to task B? If
> > > > > > > > > the UIDs match, the first "goto ok" executes; and you're dumpable, so
> > > > > > > > > you don't trigger the second "return -EPERM".
> > > > > > > >
> > > > > > > > You'd also need CAP_SYS_PTRACE in the mm->user_ns which you shouldn't
> > > > > > > > have if you did a setuid to an unpriv user. (But I always find that code
> > > > > > > > confusing.)
> > > > > > >
> > > > > > > Only if the target hasn't gone through execve() since setuid().
> > > > > >
> > > > > > Sorry if I want to know this in excessive detail but I'd like to
> > > > > > understand this properly so bear with me :)
> > > > > > - If task B has setuid()ed and prctl(PR_SET_DUMPABLE, 1)ed but not
> > > > > > execve()ed then C won't pass ptrace_has_cap(mm->user_ns, mode).
> > > > >
> > > > > Yeah.
> > > > >
> > > > > > - If task B has setuid()ed, exeved()ed it will get its dumpable flag set
> > > > > > to /proc/sys/fs/suid_dumpable
> > > > >
> > > > > Not if you changed all UIDs (e.g. by calling setuid() as root). In
> > > > > that case, setup_new_exec() calls "set_dumpable(current->mm,
> > > > > SUID_DUMP_USER)".
> > > >
> > > > Actually, looking at this when C is trying to PTRACE_ATTACH to B as an
> > > > unprivileged user even if B execve()ed and it is dumpable C still
> > > > wouldn't have CAP_SYS_PTRACE in the mm->user_ns unless it already is
> > > > privileged over mm->user_ns which means it must be in an ancestor
> > > > user_ns.
> > >
> > > Huh? Why would you need CAP_SYS_PTRACE for anything here? You can
> > > ptrace another process running under your UID just fine, no matter
> > > what the namespaces are. I'm not sure what you're saying.
> >
> > Sorry, I was out the door yesterday when answering this and was too
> > brief. I forgot to mention: /proc/sys/kernel/yama/ptrace_scope. It
> > should be enabled by default on nearly all distros
>
> "nearly all distros"? AFAIK it's off on Debian, for starters. And Yama
> still doesn't help you if one of the tasks enters a new user namespace
> or whatever.
>
> Yama is a little bit of extra, heuristic, **opt-in** hardening enabled
> in some configurations. It is **not** a fundamental building block you
> can rely on.
>
> > and even if not -
> > which is an administrators choice - you can usually easily enable it via
> > sysctl.
>
> Opt-in security isn't good enough. Kernel interfaces should still be
> safe to use even on a system that has all the LSM stuff disabled in
> the kernel config.
Then ptrace() isn't, I guess?
But see https://lists.linuxfoundation.org/pipermail/containers/2018-October/039567.html
I don't care as long as we have a way of getting the fd without the
CAP_SYS_ADMIN requirement throught seccomp().
>
> > 1 ("restricted ptrace") [default value]
> > When performing an operation that requires a PTRACE_MODE_ATTACH check,
> > the calling process must either have the CAP_SYS_PTRACE capability in
> > the user namespace of the target process or it must have a prede‐ fined
> > relationship with the target process. By default, the predefined
> > relationship is that the target process must be a descendant of the
> > caller.
> >
> > If you don't have it set you're already susceptible to all kinds of
> > other attacks
>
> Oh? Can you be more specific, please?
I was referring to attacks where you attach to processes that run as
your user but might expose in-memory credentials or other sensitive
information, (essentially what the manpage is outlining).
>
> > and I'm still not convinced we need to bring out the big
> > capable(CAP_SYS_ADMIN) gun here.
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox