Groups | Search | Server Info | Keyboard shortcuts | Login | Register [http] [https] [nntp] [nntps]

Groups > linux.kernel > #1735755 > unrolled thread

[PATCH] seccomp: fix the usage of get/put_seccomp_filter() in seccomp_get_filter()

Back to article view | Back to linux.kernel

This discussion starts older than the indexed window; earlier articles aren't shown. The article labeled Started by below is the oldest one visible, not the original post.

Contents

  [PATCH] seccomp: fix the usage of get/put_seccomp_filter() in  seccomp_get_filter() Oleg Nesterov <oleg@redhat.com> - 2017-09-20 15:00 +0200
    Re: [PATCH] seccomp: fix the usage of get/put_seccomp_filter() in  seccomp_get_filter() Oleg Nesterov <oleg@redhat.com> - 2017-09-20 15:10 +0200
      Re: [PATCH] seccomp: fix the usage of get/put_seccomp_filter() in  seccomp_get_filter() Tycho Andersen <tycho@docker.com> - 2017-09-20 15:40 +0200
        introduce get_nth_filter() Oleg Nesterov <oleg@redhat.com> - 2017-09-20 18:00 +0200
          Re: introduce get_nth_filter() Oleg Nesterov <oleg@redhat.com> - 2017-09-20 18:20 +0200
      Re: [PATCH] seccomp: fix the usage of get/put_seccomp_filter() in seccomp_get_filter() Kees Cook <keescook@chromium.org> - 2017-09-20 20:50 +0200
        Re: [PATCH] seccomp: fix the usage of get/put_seccomp_filter() in  seccomp_get_filter() Oleg Nesterov <oleg@redhat.com> - 2017-09-21 13:40 +0200
    Re: [PATCH] seccomp: fix the usage of get/put_seccomp_filter() in  seccomp_get_filter() Tycho Andersen <tycho@docker.com> - 2017-09-20 15:30 +0200
    Re: [PATCH] seccomp: fix the usage of get/put_seccomp_filter() in seccomp_get_filter() Kees Cook <keescook@chromium.org> - 2017-09-20 20:40 +0200
      Re: [PATCH] seccomp: fix the usage of get/put_seccomp_filter() in  seccomp_get_filter() Oleg Nesterov <oleg@redhat.com> - 2017-09-21 13:00 +0200
        Re: [PATCH] seccomp: fix the usage of get/put_seccomp_filter() in seccomp_get_filter() Kees Cook <keescook@chromium.org> - 2017-09-21 22:00 +0200
          Re: [PATCH] seccomp: fix the usage of get/put_seccomp_filter() in  seccomp_get_filter() Oleg Nesterov <oleg@redhat.com> - 2017-09-22 17:30 +0200
            Re: [PATCH] seccomp: fix the usage of get/put_seccomp_filter() in  seccomp_get_filter() Tycho Andersen <tycho@docker.com> - 2017-09-22 17:30 +0200

#1735755 — [PATCH] seccomp: fix the usage of get/put_seccomp_filter() in seccomp_get_filter()

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;
 
 out:
-- 
2.5.0

[toc] | [next] | [standalone]

#1735764

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

[toc] | [prev] | [next] | [standalone]

#1735772

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
> 

[toc] | [prev] | [next] | [standalone]

#1735868 — introduce get_nth_filter()

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

[toc] | [prev] | [next] | [standalone]

#1735883 — Re: introduce get_nth_filter()

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

[toc] | [prev] | [next] | [standalone]

#1736005 — 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

[toc] | [prev] | [next] | [standalone]

#1736584

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.

[toc] | [prev] | [next] | [standalone]

#1735766

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
> 
> 

[toc] | [prev] | [next] | [standalone]

#1736001 — Re: [PATCH] seccomp: fix the usage of get/put_seccomp_filter() in seccomp_get_filter()

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

[toc] | [prev] | [next] | [standalone]

#1736536

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.

[toc] | [prev] | [next] | [standalone]

#1737043 — Re: [PATCH] seccomp: fix the usage of get/put_seccomp_filter() in seccomp_get_filter()

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

[toc] | [prev] | [next] | [standalone]

#1737578

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.

[toc] | [prev] | [next] | [standalone]

#1737581

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

[toc] | [prev] | [standalone]

Back to top | Article view | linux.kernel

csiph-web