Messages in this thread | | | From | Kees Cook <> | Date | Wed, 20 Sep 2017 11:40:14 -0700 | Subject | Re: [PATCH] seccomp: fix the usage of get/put_seccomp_filter() in seccomp_get_filter() |
| |
On Wed, Sep 20, 2017 at 6:04 AM, Oleg Nesterov <oleg@redhat.com> wrote: > On 09/20, Oleg Nesterov wrote: >> >> @@ -908,13 +912,13 @@ long seccomp_get_filter(struct task_struct *task, unsigned long filter_off, >> if (!data) >> goto out; >> >> - get_seccomp_filter(task); >> + refcount_inc(&filter->usage); >> spin_unlock_irq(&task->sighand->siglock); >> >> if (copy_to_user(data, fprog->filter, bpf_classic_proglen(fprog))) >> ret = -EFAULT; >> >> - put_seccomp_filter(task); >> + __put_seccomp_filter(filter); > > This is the simple fix for -stable, but again, can't we simplify this > code? Afaics we can do get_seccomp_filter() at the start and drop siglock > right after that. > > Something like the untested patch (on top of this one) below?
Yeah, I think this one looks good (modulo the -stable patch change).
> And I can't understand the SECCOMP_MODE_DISABLED check... shouldn't we > simply remove it?
I like doing these sanity checks -- this isn't fast-path at all.
> --- x/kernel/seccomp.c > +++ x/kernel/seccomp.c > @@ -858,45 +858,36 @@ long prctl_set_seccomp(unsigned long seccomp_mode, char __user *filter) > long seccomp_get_filter(struct task_struct *task, unsigned long filter_off, > void __user *data) > { > - struct seccomp_filter *filter; > + struct seccomp_filter *orig, *filter; > struct sock_fprog_kern *fprog; > + unsigned long count; > long ret; > - unsigned long count = 0; > > if (!capable(CAP_SYS_ADMIN) || > current->seccomp.mode != SECCOMP_MODE_DISABLED) { > return -EACCES; > } > > + if (task->seccomp.mode != SECCOMP_MODE_FILTER) > + return -EINVAL; > + > spin_lock_irq(&task->sighand->siglock); > - if (task->seccomp.mode != SECCOMP_MODE_FILTER) { > - ret = -EINVAL; > - goto out; > - } > + get_seccomp_filter(task); > + orig = task->seccomp.filter; > + spin_unlock_irq(&task->sighand->siglock); > > - filter = task->seccomp.filter; > - while (filter) { > - filter = filter->prev; > + count = 0; > + for (filter = orig; filter; filter = filter->prev) > count++; > - } > > if (filter_off >= count) { > ret = -ENOENT; > goto out; > } > - count -= filter_off; > > - filter = task->seccomp.filter; > - while (filter && count > 1) { > - filter = filter->prev; > + count -= filter_off; > + for (filter = orig; count > 1; filter = filter->prev) > count--; > - } > - > - if (WARN_ON(count != 1 || !filter)) { > - /* The filter tree shouldn't shrink while we're using it. */ > - ret = -ENOENT; > - goto out; > - }
Similarly, there's no reason to remove this check either.
> fprog = filter->prog->orig_prog; > if (!fprog) { > @@ -912,17 +903,11 @@ long seccomp_get_filter(struct task_struct *task, unsigned long filter_off, > if (!data) > goto out; > > - refcount_inc(&filter->usage); > - spin_unlock_irq(&task->sighand->siglock); > - > if (copy_to_user(data, fprog->filter, bpf_classic_proglen(fprog))) > ret = -EFAULT; > > - __put_seccomp_filter(filter); > - return ret; > - > out: > - spin_unlock_irq(&task->sighand->siglock); > + __put_seccomp_filter(orig); > return ret; > } > #endif >
-Kees
-- Kees Cook Pixel Security
| |