* Re: [PATCH] seccomp: fix the usage of get/put_seccomp_filter() in seccomp_get_filter()
2017-09-20 12:56 ` [PATCH] seccomp: fix the usage of get/put_seccomp_filter() in seccomp_get_filter() Oleg Nesterov
@ 2017-09-20 13:04 ` Oleg Nesterov
2017-09-20 13:37 ` Tycho Andersen
2017-09-20 18:40 ` [PATCH] seccomp: fix the usage of get/put_seccomp_filter() in seccomp_get_filter() Kees Cook
2017-09-20 13:26 ` Tycho Andersen
2017-09-20 18:36 ` Kees Cook
2 siblings, 2 replies; 15+ messages in thread
From: Oleg Nesterov @ 2017-09-20 13:04 UTC (permalink / raw)
To: Chris Salls, Kees Cook, Andy Lutomirski, Will Drewry
Cc: security, linux-kernel, Tycho Andersen
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?
And I can't understand the SECCOMP_MODE_DISABLED check... shouldn't we
simply remove it?
Oleg.
--- 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;
- }
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
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH] seccomp: fix the usage of get/put_seccomp_filter() in seccomp_get_filter()
2017-09-20 13:04 ` Oleg Nesterov
@ 2017-09-20 13:37 ` Tycho Andersen
2017-09-20 15:59 ` introduce get_nth_filter() Oleg Nesterov
2017-09-20 18:40 ` [PATCH] seccomp: fix the usage of get/put_seccomp_filter() in seccomp_get_filter() Kees Cook
1 sibling, 1 reply; 15+ messages in thread
From: Tycho Andersen @ 2017-09-20 13:37 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Chris Salls, Kees Cook, Andy Lutomirski, Will Drewry, security,
linux-kernel
On Wed, Sep 20, 2017 at 03:04:43PM +0200, Oleg Nesterov 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?
Yes, this looks good to me, thanks.
> And I can't understand the SECCOMP_MODE_DISABLED check... shouldn't we
> simply remove it?
I think the idea was to prevent some interaction between
seccomp+ptrace+fork that we didn't understand. Since the user of this
code doesn't have seccomp filters attached, it was fine.
Thanks for cleaning this up, I'll be happy to test whatever final
patch we come up with.
Tycho
> Oleg.
>
>
> --- 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;
> - }
>
> 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
>
^ permalink raw reply [flat|nested] 15+ messages in thread* introduce get_nth_filter()
2017-09-20 13:37 ` Tycho Andersen
@ 2017-09-20 15:59 ` Oleg Nesterov
2017-09-20 16:14 ` Oleg Nesterov
0 siblings, 1 reply; 15+ messages in thread
From: Oleg Nesterov @ 2017-09-20 15:59 UTC (permalink / raw)
To: Tycho Andersen
Cc: Chris Salls, Kees Cook, Andy Lutomirski, Will Drewry, security,
linux-kernel
On 09/20, Tycho Andersen wrote:
>
> Thanks for cleaning this up, I'll be happy to test whatever final
> patch we come up with.
Well, I just noticed you sent another "[PATCH] ptrace, seccomp: add support
for retrieving seccomp flags" today...
So if we need get_nth() helper please consider the UNTESTED change below
(on top of this fix). If you agree with this code, feel free to incorporate
it into your patch.
Oleg.
--- a/kernel/seccomp.c
+++ b/kernel/seccomp.c
@@ -855,49 +855,54 @@ long prctl_set_seccomp(unsigned long seccomp_mode, char __user *filter)
}
#if defined(CONFIG_SECCOMP_FILTER) && defined(CONFIG_CHECKPOINT_RESTORE)
-long seccomp_get_filter(struct task_struct *task, unsigned long filter_off,
- void __user *data)
+static struct seccomp_filter *
+get_nth_filter(struct task_struct *task, unsigned long filter_off)
{
- struct seccomp_filter *filter;
- struct sock_fprog_kern *fprog;
- long ret;
- unsigned long count = 0;
+ struct seccomp_filter *orig, *filter;
+ unsigned long count;
- if (!capable(CAP_SYS_ADMIN) ||
- current->seccomp.mode != SECCOMP_MODE_DISABLED) {
- return -EACCES;
- }
+ if (task->seccomp.mode != SECCOMP_MODE_FILTER)
+ return ERR_PTR(-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;
+ filter = ERR_PTR(-ENOENT);
+ if (filter_off >= count)
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;
+ refcount_inc(&filter->usage);
+out:
+ __put_seccomp_filter(orig);
+ return filter;
+}
+
+long seccomp_get_filter(struct task_struct *task, unsigned long filter_off,
+ void __user *data)
+{
+ struct seccomp_filter *filter;
+ struct sock_fprog_kern *fprog;
+ long ret;
+
+ if (!capable(CAP_SYS_ADMIN) ||
+ current->seccomp.mode != SECCOMP_MODE_DISABLED) {
+ return -EACCES;
}
+ filter = get_nth_filter(task, filter_off);
+ if (IS_ERR(filter))
+ return PTR_ERR(filter);
+
fprog = filter->prog->orig_prog;
if (!fprog) {
/* This must be a new non-cBPF filter, since we save
@@ -912,17 +917,10 @@ 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(filter);
return ret;
}
#endif
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: introduce get_nth_filter()
2017-09-20 15:59 ` introduce get_nth_filter() Oleg Nesterov
@ 2017-09-20 16:14 ` Oleg Nesterov
0 siblings, 0 replies; 15+ messages in thread
From: Oleg Nesterov @ 2017-09-20 16:14 UTC (permalink / raw)
To: Tycho Andersen
Cc: Chris Salls, Kees Cook, Andy Lutomirski, Will Drewry, security,
linux-kernel
On 09/20, Oleg Nesterov wrote:
>
> On 09/20, Tycho Andersen wrote:
> >
> > Thanks for cleaning this up, I'll be happy to test whatever final
> > patch we come up with.
>
> Well, I just noticed you sent another "[PATCH] ptrace, seccomp: add support
> for retrieving seccomp flags" today...
>
> So if we need get_nth() helper please consider the UNTESTED change below
> (on top of this fix). If you agree with this code, feel free to incorporate
> it into your patch.
and probably we should shift the CAP_SYS_ADMIN/SECCOMP_MODE_DISABLED into
get_nth() too, see v2 below.
Perhaps it makes sense to add a comment to explain that spin_lock_irq(siglock)
is only correct because the caller is the tracer, and thus the TASK_TRACED
"task" can't exit. Otherwise we would need lock_task_sighand().
Oleg.
--- a/kernel/seccomp.c
+++ b/kernel/seccomp.c
@@ -855,48 +855,53 @@ long prctl_set_seccomp(unsigned long seccomp_mode, char __user *filter)
}
#if defined(CONFIG_SECCOMP_FILTER) && defined(CONFIG_CHECKPOINT_RESTORE)
-long seccomp_get_filter(struct task_struct *task, unsigned long filter_off,
- void __user *data)
+static struct seccomp_filter *
+get_nth_filter(struct task_struct *task, unsigned long filter_off)
{
- struct seccomp_filter *filter;
- struct sock_fprog_kern *fprog;
- long ret;
- unsigned long count = 0;
+ struct seccomp_filter *orig, *filter;
+ unsigned long count;
if (!capable(CAP_SYS_ADMIN) ||
current->seccomp.mode != SECCOMP_MODE_DISABLED) {
- return -EACCES;
+ return ERR_PTR(-EACCES);
}
+ if (task->seccomp.mode != SECCOMP_MODE_FILTER)
+ return ERR_PTR(-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;
+ filter = ERR_PTR(-ENOENT);
+ if (filter_off >= count)
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;
- }
+ refcount_inc(&filter->usage);
+out:
+ __put_seccomp_filter(orig);
+ return filter;
+}
+
+long seccomp_get_filter(struct task_struct *task, unsigned long filter_off,
+ void __user *data)
+{
+ struct seccomp_filter *filter;
+ struct sock_fprog_kern *fprog;
+ long ret;
+
+ filter = get_nth_filter(task, filter_off);
+ if (IS_ERR(filter))
+ return PTR_ERR(filter);
fprog = filter->prog->orig_prog;
if (!fprog) {
@@ -912,17 +917,10 @@ 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(filter);
return ret;
}
#endif
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] seccomp: fix the usage of get/put_seccomp_filter() in seccomp_get_filter()
2017-09-20 13:04 ` Oleg Nesterov
2017-09-20 13:37 ` Tycho Andersen
@ 2017-09-20 18:40 ` Kees Cook
2017-09-21 11:31 ` Oleg Nesterov
1 sibling, 1 reply; 15+ messages in thread
From: Kees Cook @ 2017-09-20 18:40 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Chris Salls, Andy Lutomirski, Will Drewry, security@kernel.org,
LKML, Tycho Andersen
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
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH] seccomp: fix the usage of get/put_seccomp_filter() in seccomp_get_filter()
2017-09-20 18:40 ` [PATCH] seccomp: fix the usage of get/put_seccomp_filter() in seccomp_get_filter() Kees Cook
@ 2017-09-21 11:31 ` Oleg Nesterov
0 siblings, 0 replies; 15+ messages in thread
From: Oleg Nesterov @ 2017-09-21 11:31 UTC (permalink / raw)
To: Kees Cook
Cc: Chris Salls, Andy Lutomirski, Will Drewry, security@kernel.org,
LKML, Tycho Andersen
On 09/20, Kees Cook wrote:
>
> I like doing these sanity checks -- this isn't fast-path at all.
Yes, but see another "introduce get_nth_filter()" cleanup I sent, it is
similar but more suitable for Tycho's "retrieving seccomp flags" patch.
> > + for (filter = orig; count > 1; filter = filter->prev)
^^^^^^^^^
I just noticed that I forgot to replace this check with "count != 1".
Correctness wise this doesn't matter, but looks more clean.
> > 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.
Well, I disagree, but this is subjective so I won't insist.
Why do we want this WARN_ON() ? The sanity check can only fail if we have
a bug in 10 lines above. Lets look at the code after this cleanup,
count = 0;
for (filter = orig; filter; filter = filter->prev)
count++;
if (filter_off >= count)
goto out;
count -= filter_off;
for (filter = orig; count != 1; filter = filter->prev)
count--;
Do we want to check "count == 1" after the 2nd loop? I don't think so.
filter != NULL ? IMO makes no sense. Again, it can only be NULL if the
quoted code above is wrong, and in this case the next line
refcount_inc(&filter->usage);
will crash.
Oleg.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] seccomp: fix the usage of get/put_seccomp_filter() in seccomp_get_filter()
2017-09-20 12:56 ` [PATCH] seccomp: fix the usage of get/put_seccomp_filter() in seccomp_get_filter() Oleg Nesterov
2017-09-20 13:04 ` Oleg Nesterov
@ 2017-09-20 13:26 ` Tycho Andersen
2017-09-20 18:36 ` Kees Cook
2 siblings, 0 replies; 15+ messages in thread
From: Tycho Andersen @ 2017-09-20 13:26 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Chris Salls, Kees Cook, Andy Lutomirski, Will Drewry, security,
linux-kernel
On Wed, Sep 20, 2017 at 02:56:21PM +0200, Oleg Nesterov wrote:
> As Chris explains, get_seccomp_filter() and put_seccomp_filter() can
> use the different filters, once we drop ->siglock task->seccomp.filter
> can be replaced by SECCOMP_FILTER_FLAG_TSYNC.
>
> Fixes: f8e529ed941b ("seccomp, ptrace: add support for dumping seccomp filters")
> Reported-by: Chris Salls <chrissalls5@gmail.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
Ugh! Whoops.
Acked-by: Tycho Andersen <tycho@docker.com>
> ---
> kernel/seccomp.c | 14 +++++++++-----
> 1 file changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/kernel/seccomp.c b/kernel/seccomp.c
> index 98b59b5..897f153 100644
> --- a/kernel/seccomp.c
> +++ b/kernel/seccomp.c
> @@ -476,10 +476,8 @@ static inline void seccomp_filter_free(struct seccomp_filter *filter)
> }
> }
>
> -/* put_seccomp_filter - decrements the ref count of tsk->seccomp.filter */
> -void put_seccomp_filter(struct task_struct *tsk)
> +static void __put_seccomp_filter(struct seccomp_filter *orig)
> {
> - struct seccomp_filter *orig = tsk->seccomp.filter;
> /* Clean up single-reference branches iteratively. */
> while (orig && refcount_dec_and_test(&orig->usage)) {
> struct seccomp_filter *freeme = orig;
> @@ -488,6 +486,12 @@ void put_seccomp_filter(struct task_struct *tsk)
> }
> }
>
> +/* put_seccomp_filter - decrements the ref count of tsk->seccomp.filter */
> +void put_seccomp_filter(struct task_struct *tsk)
> +{
> + __put_seccomp_filter(tsk->seccomp.filter);
> +}
> +
> static void seccomp_init_siginfo(siginfo_t *info, int syscall, int reason)
> {
> memset(info, 0, sizeof(*info));
> @@ -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);
> return ret;
>
> out:
> --
> 2.5.0
>
>
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH] seccomp: fix the usage of get/put_seccomp_filter() in seccomp_get_filter()
2017-09-20 12:56 ` [PATCH] seccomp: fix the usage of get/put_seccomp_filter() in seccomp_get_filter() Oleg Nesterov
2017-09-20 13:04 ` Oleg Nesterov
2017-09-20 13:26 ` Tycho Andersen
@ 2017-09-20 18:36 ` Kees Cook
2017-09-21 10:57 ` Oleg Nesterov
2 siblings, 1 reply; 15+ messages in thread
From: Kees Cook @ 2017-09-20 18:36 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Chris Salls, Andy Lutomirski, Will Drewry, security@kernel.org,
LKML, Tycho Andersen
On Wed, Sep 20, 2017 at 5:56 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> As Chris explains, get_seccomp_filter() and put_seccomp_filter() can
> use the different filters, once we drop ->siglock task->seccomp.filter
> can be replaced by SECCOMP_FILTER_FLAG_TSYNC.
>
> Fixes: f8e529ed941b ("seccomp, ptrace: add support for dumping seccomp filters")
> Reported-by: Chris Salls <chrissalls5@gmail.com>
> Cc: stable@vger.kernel.org
> Signed-off-by: Oleg Nesterov <oleg@redhat.com>
> ---
> kernel/seccomp.c | 14 +++++++++-----
> 1 file changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/kernel/seccomp.c b/kernel/seccomp.c
> index 98b59b5..897f153 100644
> --- a/kernel/seccomp.c
> +++ b/kernel/seccomp.c
> @@ -476,10 +476,8 @@ static inline void seccomp_filter_free(struct seccomp_filter *filter)
> }
> }
>
> -/* put_seccomp_filter - decrements the ref count of tsk->seccomp.filter */
> -void put_seccomp_filter(struct task_struct *tsk)
> +static void __put_seccomp_filter(struct seccomp_filter *orig)
> {
> - struct seccomp_filter *orig = tsk->seccomp.filter;
> /* Clean up single-reference branches iteratively. */
> while (orig && refcount_dec_and_test(&orig->usage)) {
> struct seccomp_filter *freeme = orig;
> @@ -488,6 +486,12 @@ void put_seccomp_filter(struct task_struct *tsk)
> }
> }
>
> +/* put_seccomp_filter - decrements the ref count of tsk->seccomp.filter */
> +void put_seccomp_filter(struct task_struct *tsk)
> +{
> + __put_seccomp_filter(tsk->seccomp.filter);
> +}
> +
> static void seccomp_init_siginfo(siginfo_t *info, int syscall, int reason)
> {
> memset(info, 0, sizeof(*info));
> @@ -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);
> return ret;
Given how reference counting is done for filters, I'd be happier with
leaving the get_seccomp_filter() as-is, and providing the
__put_seccomp_filter() as the only change here (i.e. don't open-code
the refcount_inc()).
-Kees
--
Kees Cook
Pixel Security
^ permalink raw reply [flat|nested] 15+ messages in thread* Re: [PATCH] seccomp: fix the usage of get/put_seccomp_filter() in seccomp_get_filter()
2017-09-20 18:36 ` Kees Cook
@ 2017-09-21 10:57 ` Oleg Nesterov
2017-09-21 19:51 ` Kees Cook
0 siblings, 1 reply; 15+ messages in thread
From: Oleg Nesterov @ 2017-09-21 10:57 UTC (permalink / raw)
To: Kees Cook
Cc: Chris Salls, Andy Lutomirski, Will Drewry, security@kernel.org,
LKML, Tycho Andersen
On 09/20, Kees Cook wrote:
>
> On Wed, Sep 20, 2017 at 5:56 AM, Oleg Nesterov <oleg@redhat.com> 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);
> > return ret;
>
> Given how reference counting is done for filters, I'd be happier with
> leaving the get_seccomp_filter() as-is,
No, please note that filter != tsk->seccomp.filter, get_seccomp_filter()
won't work.
> (i.e. don't open-code
> the refcount_inc()).
agreed, probably another __get_seccomp_filter(filter) makes sense, especially
if we do other changes like get_nth().
But imo not in this fix.
Oleg.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] seccomp: fix the usage of get/put_seccomp_filter() in seccomp_get_filter()
2017-09-21 10:57 ` Oleg Nesterov
@ 2017-09-21 19:51 ` Kees Cook
2017-09-22 15:22 ` Oleg Nesterov
0 siblings, 1 reply; 15+ messages in thread
From: Kees Cook @ 2017-09-21 19:51 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Chris Salls, Andy Lutomirski, Will Drewry, security@kernel.org,
LKML, Tycho Andersen
On Thu, Sep 21, 2017 at 3:57 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> On 09/20, Kees Cook wrote:
>>
>> On Wed, Sep 20, 2017 at 5:56 AM, Oleg Nesterov <oleg@redhat.com> 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);
>> > return ret;
>>
>> Given how reference counting is done for filters, I'd be happier with
>> leaving the get_seccomp_filter() as-is,
>
> No, please note that filter != tsk->seccomp.filter, get_seccomp_filter()
> won't work.
Ah yes, sorry, you're right.
>> (i.e. don't open-code
>> the refcount_inc()).
>
> agreed, probably another __get_seccomp_filter(filter) makes sense, especially
> if we do other changes like get_nth().
>
> But imo not in this fix.
Regardless, whatever lands will need backport adjustment for
refcount_*/atomic_* in -stable.
Can you resend the two patches; I can send the backport to -stable manually...
-Kees
--
Kees Cook
Pixel Security
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] seccomp: fix the usage of get/put_seccomp_filter() in seccomp_get_filter()
2017-09-21 19:51 ` Kees Cook
@ 2017-09-22 15:22 ` Oleg Nesterov
2017-09-22 15:25 ` Tycho Andersen
2017-09-26 20:15 ` Tycho Andersen
0 siblings, 2 replies; 15+ messages in thread
From: Oleg Nesterov @ 2017-09-22 15:22 UTC (permalink / raw)
To: Kees Cook
Cc: Chris Salls, Andy Lutomirski, Will Drewry, security@kernel.org,
LKML, Tycho Andersen
On 09/21, Kees Cook wrote:
>
> On Thu, Sep 21, 2017 at 3:57 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> > On 09/20, Kees Cook wrote:
> >>
> >> Given how reference counting is done for filters, I'd be happier with
> >> leaving the get_seccomp_filter() as-is,
> >
> > No, please note that filter != tsk->seccomp.filter, get_seccomp_filter()
> > won't work.
>
> Ah yes, sorry, you're right.
>
> >> (i.e. don't open-code
> >> the refcount_inc()).
> >
> > agreed, probably another __get_seccomp_filter(filter) makes sense, especially
> > if we do other changes like get_nth().
> >
> > But imo not in this fix.
>
> Regardless, whatever lands will need backport adjustment for
> refcount_*/atomic_* in -stable.
yes, but this adjustment is trivial, and we will need it whatever we do
in this fix,
> Can you resend the two patches; I can send the backport to -stable manually...
Not sure I understand... Do you mean this fix + untested "introduce get_nth_filter()" ?
Can't we push this simple fix first? Then we can discuss the cleanups. Besides,
the 2nd patch connects to Tycho's "[PATCH] ptrace, seccomp: add support for
retrieving seccomp flags", otherwise it could be more simple.
Oleg.
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] seccomp: fix the usage of get/put_seccomp_filter() in seccomp_get_filter()
2017-09-22 15:22 ` Oleg Nesterov
@ 2017-09-22 15:25 ` Tycho Andersen
2017-09-26 20:15 ` Tycho Andersen
1 sibling, 0 replies; 15+ messages in thread
From: Tycho Andersen @ 2017-09-22 15:25 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Kees Cook, Chris Salls, Andy Lutomirski, Will Drewry,
security@kernel.org, LKML
On Fri, Sep 22, 2017 at 05:22:29PM +0200, Oleg Nesterov wrote:
> On 09/21, Kees Cook wrote:
> >
> > On Thu, Sep 21, 2017 at 3:57 AM, Oleg Nesterov <oleg@redhat.com> wrote:
> > > On 09/20, Kees Cook wrote:
> > >>
> > >> Given how reference counting is done for filters, I'd be happier with
> > >> leaving the get_seccomp_filter() as-is,
> > >
> > > No, please note that filter != tsk->seccomp.filter, get_seccomp_filter()
> > > won't work.
> >
> > Ah yes, sorry, you're right.
> >
> > >> (i.e. don't open-code
> > >> the refcount_inc()).
> > >
> > > agreed, probably another __get_seccomp_filter(filter) makes sense, especially
> > > if we do other changes like get_nth().
> > >
> > > But imo not in this fix.
> >
> > Regardless, whatever lands will need backport adjustment for
> > refcount_*/atomic_* in -stable.
>
> yes, but this adjustment is trivial, and we will need it whatever we do
> in this fix,
>
> > Can you resend the two patches; I can send the backport to -stable manually...
>
> Not sure I understand... Do you mean this fix + untested "introduce get_nth_filter()" ?
>
> Can't we push this simple fix first? Then we can discuss the cleanups. Besides,
> the 2nd patch connects to Tycho's "[PATCH] ptrace, seccomp: add support for
> retrieving seccomp flags", otherwise it could be more simple.
Yes, I'll happily fold your fix into the next version of my patch. As
it stands now I'm just waiting on input about unrelated API feedback.
Cheers,
Tycho
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] seccomp: fix the usage of get/put_seccomp_filter() in seccomp_get_filter()
2017-09-22 15:22 ` Oleg Nesterov
2017-09-22 15:25 ` Tycho Andersen
@ 2017-09-26 20:15 ` Tycho Andersen
2017-09-27 6:07 ` Kees Cook
1 sibling, 1 reply; 15+ messages in thread
From: Tycho Andersen @ 2017-09-26 20:15 UTC (permalink / raw)
To: Oleg Nesterov
Cc: Kees Cook, Chris Salls, Andy Lutomirski, Will Drewry,
security@kernel.org, LKML
Hi,
On Fri, Sep 22, 2017 at 05:22:29PM +0200, Oleg Nesterov wrote:
> On 09/21, Kees Cook wrote:
> > Can you resend the two patches; I can send the backport to -stable manually...
>
> Not sure I understand... Do you mean this fix + untested "introduce get_nth_filter()" ?
Just want to make sure this doesn't get lost in the shuffle. If I
resend just Oleg's patch with the added __get_secomp_filter() instead
of open coded refcount, will that work for you Kees?
We can worry about the get_nth_filter implementation with the
PTRACE_SECCOMP_GET_METADATA series later.
Cheers,
Tycho
^ permalink raw reply [flat|nested] 15+ messages in thread
* Re: [PATCH] seccomp: fix the usage of get/put_seccomp_filter() in seccomp_get_filter()
2017-09-26 20:15 ` Tycho Andersen
@ 2017-09-27 6:07 ` Kees Cook
0 siblings, 0 replies; 15+ messages in thread
From: Kees Cook @ 2017-09-27 6:07 UTC (permalink / raw)
To: Tycho Andersen
Cc: Oleg Nesterov, Chris Salls, Andy Lutomirski, Will Drewry,
security@kernel.org, LKML
On Tue, Sep 26, 2017 at 10:15 PM, Tycho Andersen <tycho@docker.com> wrote:
> Hi,
>
> On Fri, Sep 22, 2017 at 05:22:29PM +0200, Oleg Nesterov wrote:
>> On 09/21, Kees Cook wrote:
>> > Can you resend the two patches; I can send the backport to -stable manually...
>>
>> Not sure I understand... Do you mean this fix + untested "introduce get_nth_filter()" ?
>
> Just want to make sure this doesn't get lost in the shuffle. If I
> resend just Oleg's patch with the added __get_secomp_filter() instead
> of open coded refcount, will that work for you Kees?
Yeah, this should be fine; thanks!
-Kees
>
> We can worry about the get_nth_filter implementation with the
> PTRACE_SECCOMP_GET_METADATA series later.
>
> Cheers,
>
> Tycho
--
Kees Cook
Pixel Security
^ permalink raw reply [flat|nested] 15+ messages in thread