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

[Tycho Andersen <tycho.andersen@canonical.com>]

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:
> >+
> >+#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?

> >+}
> >+#endif /* CONFIG_SECCOMP_FILTER && CONFIG_CHECKPOINT_RESTORE */
> >  #endif /* _LINUX_SECCOMP_H */
> ...
> >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.

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

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