Re: [PATCH v6] seccomp, ptrace: add support for dumping seccomp filters

Hi Daniel,

On Wed, Oct 07, 2015 at 12:25:39PM +0200, Daniel Borkmann wrote:

On 10/07/2015 11:46 AM, Tycho Andersen wrote:

quoted

+
+#if defined(CONFIG_SECCOMP_FILTER) && defined(CONFIG_CHECKPOINT_RESTORE)
+extern long seccomp_get_filter(struct task_struct *task, long n,
+			       void __user *data);
+#else
+static inline long seccomp_get_filter(struct task_struct *task,
+				      long n, void __user *data)
+{
+	return -EINVAL;

Nit: -ENOTSUP would probably be the better choice? -EINVAL might just
be confusing to users? (Would be unclear to them whether there's actual
support of dumping or whether it's just an invalid argument.)

Fine with me, the rest of the seccomp functions in this file use
-EINVAL, so I'm just copying that. Kees?

quoted

+}
+#endif /* CONFIG_SECCOMP_FILTER && CONFIG_CHECKPOINT_RESTORE */
 #endif /* _LINUX_SECCOMP_H */

...

quoted

diff --git a/kernel/ptrace.c b/kernel/ptrace.c
index 787320d..b760bae 100644
--- a/kernel/ptrace.c
+++ b/kernel/ptrace.c

@@ -1016,6 +1016,11 @@ int ptrace_request(struct task_struct *child, long request,
 		break;
 	}
 #endif
+
+	case PTRACE_SECCOMP_GET_FILTER:
+		ret = seccomp_get_filter(child, addr, datavp);
+		break;
+
 	default:
 		break;
 	}

diff --git a/kernel/seccomp.c b/kernel/seccomp.c
index 06858a7..c8a4564 100644
--- a/kernel/seccomp.c
+++ b/kernel/seccomp.c

@@ -347,6 +347,7 @@ static struct seccomp_filter *seccomp_prepare_filter(struct sock_fprog *fprog)
 {
 	struct seccomp_filter *sfilter;
 	int ret;
+	bool save_orig = config_enabled(CONFIG_CHECKPOINT_RESTORE);

 	if (fprog->len == 0 || fprog->len > BPF_MAXINSNS)
 		return ERR_PTR(-EINVAL);

@@ -370,7 +371,7 @@ static struct seccomp_filter *seccomp_prepare_filter(struct sock_fprog *fprog)
 		return ERR_PTR(-ENOMEM);

 	ret = bpf_prog_create_from_user(&sfilter->prog, fprog,
-					seccomp_check_filter, false);
+					seccomp_check_filter, save_orig);
 	if (ret < 0) {
 		kfree(sfilter);
 		return ERR_PTR(ret);

@@ -867,3 +868,57 @@ long prctl_set_seccomp(unsigned long seccomp_mode, char __user *filter)
 	/* prctl interface doesn't have flags, so they are always zero. */
 	return do_seccomp(op, 0, uargs);
 }
+
+#if defined(CONFIG_SECCOMP_FILTER) && defined(CONFIG_CHECKPOINT_RESTORE)
+long seccomp_get_filter(struct task_struct *task, long n, void __user *data)
+{
+	struct seccomp_filter *filter;
+	struct sock_fprog_kern *fprog;
+	long ret;
+
+	if (n < 0)
+		return -EINVAL;

I would probably give 'n' a better name, maybe 'filter_off' to denote an
offset in the task's filter list?

Ok, I can make that change.

So, it's called as seccomp_get_filter(child, addr, datavp), and addr is
an unsigned long in ptrace_request(). Any reasons why making this 'long n'
with adding this above check?

No, I think just an oversight; I'll switch it to unsigned.

quoted

+	spin_lock_irq(&current->sighand->siglock);
+	if (!capable(CAP_SYS_ADMIN) ||

The capability check should probably happen before taking the task's spinlock.

We (probably) don't need the lock at all since we're just reading, but
seccomp_may_assign_mode() asserts that things are locked before it
checks current->seccomp.mode, so I lock here as well (and that's the
only reason to lock), so if we lock after, we don't need to lock at
all.

quoted

+	    current->seccomp.mode != SECCOMP_MODE_DISABLED) {
+		ret = -EACCES;
+		goto out_self;
+	}
+
+	spin_lock_irq(&task->sighand->siglock);
+	if (task->seccomp.mode != SECCOMP_MODE_FILTER) {
+		ret = -EINVAL;
+		goto out_task;
+	}
+
+	filter = task->seccomp.filter;
+	while (n > 0 && filter) {
+		filter = filter->prev;
+		n--;
+	}
+
+	if (!filter) {
+		ret = -ENOENT;
+		goto out_task;
+	}
+
+	fprog = filter->prog->orig_prog;

You could add this check ...

  https://git.kernel.org/cgit/linux/kernel/git/davem/net.git/commit/?id=93d08b6966cf730ea669d4d98f43627597077153

... here as well, so we don't get surprises in future. ;)

Ok :). Then we need to bikeshed which error code, though. I proposed
EMEDIUMTYPE before, but I'm happy to use whatever.

Thanks for the review.

Tycho