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

(quoted diff) +0 -0

@@ -908,13 +912,13 @@ long seccomp_get_filter(struct task_struct *task, unsigned long filter_off,
 	if (!data)
 		goto out;

Quoted 12 lines · Oleg Nesterov

kernel/seccomp.c +1 -1

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

Review long seccomp_get_filter(struct task_struct *task, unsigned long filter_off, · line 921

Quoted 2 lines

(quoted diff) +1 -1

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

kernel/seccomp.c +13 -28

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

Thanks for cleaning this up, I'll be happy to test whatever final patch we come up with.

Quoted 3 lines · Tycho Andersen

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.

Quoted 12 lines · Oleg Nesterov

kernel/seccomp.c +1 -1

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

Review long seccomp_get_filter(struct task_struct *task, unsigned long filter_off, · line 921

Quoted 2 lines

(quoted diff) +1 -1

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

kernel/seccomp.c +10 -14

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

kernel/seccomp.c +0 -3

        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;

I like doing these sanity checks -- this isn't fast-path at all.

Quoted 1 line

+ for (filter = orig; count > 1; filter = filter->prev)

kernel/seccomp.c long prctl_set_seccomp(unsigned long seccomp_mode, char __user *filter) · line 899

Quoted 8 lines

                count--;

kernel/seccomp.c +0 -7

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

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 <[email protected]>

Cc [email protected]

Signed-off-by Oleg Nesterov <[email protected]>

---
 kernel/seccomp.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

kernel/seccomp.c +9 -5

index 98b59b5..897f153 100644
@@ -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:

Signature · 1 line

2.5.0

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 <[email protected]>

Cc [email protected]

Signed-off-by Oleg Nesterov <[email protected]>

---
 kernel/seccomp.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

kernel/seccomp.c +7 -3

index 98b59b5..897f153 100644
@@ -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;
 

Quoted 11 lines · Oleg Nesterov

kernel/seccomp.c +1 -1

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

kernel/seccomp.c long seccomp_get_filter(struct task_struct *task, unsigned long filter_off, · line 922

Quoted 3 lines

kernel/seccomp.c +1 -1

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

(i.e. don't open-code
the refcount_inc()).

Quoted 13 lines · Kees Cook

Quoted 11 lines · Oleg Nesterov

kernel/seccomp.c +1 -1

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

Quoted 6 lines

kernel/seccomp.c long seccomp_get_filter(struct task_struct *task, unsigned long filter_off, · line 922

Quoted 3 lines

kernel/seccomp.c +1 -1

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

Quoted 2 lines

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

Quoted 7 lines · Oleg Nesterov

Quoted 3 lines · Kees Cook

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.

Quoted 3 lines

Quoted 2 lines

(i.e. don't open-code
the refcount_inc()).

Quoted 4 lines

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

Quoted 15 lines · Kees Cook

Quoted 7 lines · Oleg Nesterov

Quoted 3 lines · Kees Cook

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.

Quoted 3 lines

Quoted 2 lines

(i.e. don't open-code
the refcount_inc()).

Quoted 7 lines

Quoted 4 lines

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,

Quoted 1 line

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.

Quoted 1 line · Kees Cook

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()" ?

Hi,

Quoted 4 lines · Oleg Nesterov

Quoted 1 line · Kees Cook

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