linux-kernel - Re: [PATCH] seccomp: fix the usage of get/put_seccomp_filter() in seccomp_get

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Wed, 20 Sep 2017 15:04:43 +0200
From:   Oleg Nesterov <oleg@...hat.com>
To:     Chris Salls <chrissalls5@...il.com>,
        Kees Cook <keescook@...omium.org>,
        Andy Lutomirski <luto@...capital.net>,
        Will Drewry <wad@...omium.org>
Cc:     security@...nel.org, linux-kernel@...r.kernel.org,
        Tycho Andersen <tycho@...ker.com>
Subject: Re: [PATCH] seccomp: fix the usage of get/put_seccomp_filter() in
 seccomp_get_filter()

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