v6 of seccomp filter c/r

v6 of seccomp filter c/r

[Tycho Andersen]

Hi all,

Here's v6 of the seccomp filter c/r stuff. I've dropped the concept of a
seccomp fd entirely, in favor of just dumping the bpf data. This doesn't allow
us to restore correctly in all cases, though, as noted in the patch notes.

Note that this is now based on http://patchwork.ozlabs.org/patch/525492/ and
will need to be built with that patch applied. This gets rid of two incorrect
patches in a previous series and is a nicer API.

Thoughts welcome,

Tycho

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

[Tycho Andersen]

This patch adds support for dumping a process' (classic BPF) seccomp
filters via ptrace.

PTRACE_SECCOMP_GET_FILTER allows the tracer to dump the user's classic BPF
seccomp filters. addr should be an integer which represents the ith seccomp
filter (0 is the most recently installed filter). data should be a struct
sock_filter * with enough room for the ith filter, or NULL, in which case
the filter is not saved. The return value for this command is the number of
BPF instructions the program represents, or negative in the case of errors.
A command specific error is ENOENT, which indicates that there is no ith
filter in this seccomp tree.

A caveat with this approach is that there is no way to get explicitly at
the heirarchy of seccomp filters, and users need to memcmp() filters to
decide which are inherited. This means that a task which installs two of
the same filter can potentially confuse users of this interface.

Signed-off-by: Tycho Andersen <tycho.andersen@canonical.com>
CC: Kees Cook <keescook@chromium.org>
CC: Will Drewry <wad@chromium.org>
CC: Oleg Nesterov <oleg@redhat.com>
CC: Andy Lutomirski <luto@amacapital.net>
CC: Pavel Emelyanov <xemul@parallels.com>
CC: Serge E. Hallyn <serge.hallyn@ubuntu.com>
CC: Alexei Starovoitov <ast@kernel.org>
CC: Daniel Borkmann <daniel@iogearbox.net>
---
 include/linux/seccomp.h     | 11 +++++++++
 include/uapi/linux/ptrace.h |  2 ++
 kernel/ptrace.c             |  5 ++++
 kernel/seccomp.c            | 57 ++++++++++++++++++++++++++++++++++++++++++++-
 4 files changed, 74 insertions(+), 1 deletion(-)

diff --git a/include/linux/seccomp.h b/include/linux/seccomp.h
index f426503..8861b5b 100644
--- a/include/linux/seccomp.h
+++ b/include/linux/seccomp.h
@@ -95,4 +95,15 @@ static inline void get_seccomp_filter(struct task_struct *tsk)
 	return;
 }
 #endif /* CONFIG_SECCOMP_FILTER */
+
+#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;
+}
+#endif /* CONFIG_SECCOMP_FILTER && CONFIG_CHECKPOINT_RESTORE */
 #endif /* _LINUX_SECCOMP_H */
diff --git a/include/uapi/linux/ptrace.h b/include/uapi/linux/ptrace.h
index a7a6979..c9d0b21 100644
--- a/include/uapi/linux/ptrace.h
+++ b/include/uapi/linux/ptrace.h
@@ -23,6 +23,8 @@
 
 #define PTRACE_SYSCALL		  24
 
+#define PTRACE_SECCOMP_GET_FILTER	40
+
 /* 0x4200-0x4300 are reserved for architecture-independent additions.  */
 #define PTRACE_SETOPTIONS	0x4200
 #define PTRACE_GETEVENTMSG	0x4201
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;
+
+	spin_lock_irq(&current->sighand->siglock);
+	if (!capable(CAP_SYS_ADMIN) ||
+	    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;
+
+	ret = fprog->len;
+	if (!data)
+		goto out_task;
+
+	if (copy_to_user(data, fprog->filter, bpf_classic_proglen(fprog))) {
+		ret = -EFAULT;
+		goto out_task;
+	}
+
+out_task:
+	spin_unlock_irq(&task->sighand->siglock);
+
+out_self:
+	spin_unlock_irq(&current->sighand->siglock);
+	return ret;
+}
+#endif
-- 
2.5.0

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

[Daniel Borkmann]

On 10/07/2015 11:46 AM, Tycho Andersen wrote:
> This patch adds support for dumping a process' (classic BPF) seccomp
> filters via ptrace.
>
> PTRACE_SECCOMP_GET_FILTER allows the tracer to dump the user's classic BPF
> seccomp filters. addr should be an integer which represents the ith seccomp
> filter (0 is the most recently installed filter). data should be a struct
> sock_filter * with enough room for the ith filter, or NULL, in which case
> the filter is not saved. The return value for this command is the number of
> BPF instructions the program represents, or negative in the case of errors.
> A command specific error is ENOENT, which indicates that there is no ith
> filter in this seccomp tree.
>
> A caveat with this approach is that there is no way to get explicitly at
> the heirarchy of seccomp filters, and users need to memcmp() filters to
> decide which are inherited. This means that a task which installs two of
> the same filter can potentially confuse users of this interface.
>
> Signed-off-by: Tycho Andersen <tycho.andersen@canonical.com>
> CC: Kees Cook <keescook@chromium.org>
> CC: Will Drewry <wad@chromium.org>
> CC: Oleg Nesterov <oleg@redhat.com>
> CC: Andy Lutomirski <luto@amacapital.net>
> CC: Pavel Emelyanov <xemul@parallels.com>
> CC: Serge E. Hallyn <serge.hallyn@ubuntu.com>
> CC: Alexei Starovoitov <ast@kernel.org>
> CC: Daniel Borkmann <daniel@iogearbox.net>
> ---
>   include/linux/seccomp.h     | 11 +++++++++
>   include/uapi/linux/ptrace.h |  2 ++
>   kernel/ptrace.c             |  5 ++++
>   kernel/seccomp.c            | 57 ++++++++++++++++++++++++++++++++++++++++++++-
>   4 files changed, 74 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/seccomp.h b/include/linux/seccomp.h
> index f426503..8861b5b 100644
> --- a/include/linux/seccomp.h
> +++ b/include/linux/seccomp.h
> @@ -95,4 +95,15 @@ static inline void get_seccomp_filter(struct task_struct *tsk)
>   	return;
>   }
>   #endif /* CONFIG_SECCOMP_FILTER */
> +
> +#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.)

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

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?

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

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

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

> +	ret = fprog->len;
> +	if (!data)
> +		goto out_task;
> +
> +	if (copy_to_user(data, fprog->filter, bpf_classic_proglen(fprog))) {
> +		ret = -EFAULT;
> +		goto out_task;
> +	}
> +
> +out_task:
> +	spin_unlock_irq(&task->sighand->siglock);
> +
> +out_self:
> +	spin_unlock_irq(&current->sighand->siglock);
> +	return ret;
> +}
> +#endif
>

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

[Daniel Borkmann]

On 10/07/2015 12:25 PM, Daniel Borkmann wrote:
> On 10/07/2015 11:46 AM, Tycho Andersen wrote:
>> This patch adds support for dumping a process' (classic BPF) seccomp
>> filters via ptrace.
>>
>> PTRACE_SECCOMP_GET_FILTER allows the tracer to dump the user's classic BPF
>> seccomp filters. addr should be an integer which represents the ith seccomp
>> filter (0 is the most recently installed filter). data should be a struct
>> sock_filter * with enough room for the ith filter, or NULL, in which case
>> the filter is not saved. The return value for this command is the number of
>> BPF instructions the program represents, or negative in the case of errors.
>> A command specific error is ENOENT, which indicates that there is no ith
>> filter in this seccomp tree.
>>
>> A caveat with this approach is that there is no way to get explicitly at
>> the heirarchy of seccomp filters, and users need to memcmp() filters to
>> decide which are inherited. This means that a task which installs two of
>> the same filter can potentially confuse users of this interface.
>>
>> Signed-off-by: Tycho Andersen <tycho.andersen-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
>> CC: Kees Cook <keescook-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
>> CC: Will Drewry <wad-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
>> CC: Oleg Nesterov <oleg-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> CC: Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org>
>> CC: Pavel Emelyanov <xemul-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>
>> CC: Serge E. Hallyn <serge.hallyn-GeWIH/nMZzLQT0dZR+AlfA@public.gmane.org>
>> CC: Alexei Starovoitov <ast-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>> CC: Daniel Borkmann <daniel-FeC+5ew28dpmcu3hnIyYJQ@public.gmane.org>
>> ---
>>   include/linux/seccomp.h     | 11 +++++++++
>>   include/uapi/linux/ptrace.h |  2 ++
>>   kernel/ptrace.c             |  5 ++++
>>   kernel/seccomp.c            | 57 ++++++++++++++++++++++++++++++++++++++++++++-
>>   4 files changed, 74 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/linux/seccomp.h b/include/linux/seccomp.h
>> index f426503..8861b5b 100644
>> --- a/include/linux/seccomp.h
>> +++ b/include/linux/seccomp.h
>> @@ -95,4 +95,15 @@ static inline void get_seccomp_filter(struct task_struct *tsk)
>>       return;
>>   }
>>   #endif /* CONFIG_SECCOMP_FILTER */
>> +
>> +#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.)
>
>> +}
>> +#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?
>
> 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?
>
>> +    spin_lock_irq(&current->sighand->siglock);
>> +    if (!capable(CAP_SYS_ADMIN) ||
>
> The capability check should probably happen before taking the task's spinlock.
>
>> +        current->seccomp.mode != SECCOMP_MODE_DISABLED) {

Should this rather be: current->seccomp.mode == SECCOMP_MODE_DISABLED ?
So that you bail out when seccomp is not in use?

>> +        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. ;)
>
>> +    ret = fprog->len;
>> +    if (!data)
>> +        goto out_task;
>> +
>> +    if (copy_to_user(data, fprog->filter, bpf_classic_proglen(fprog))) {
>> +        ret = -EFAULT;
>> +        goto out_task;
>> +    }
>> +
>> +out_task:
>> +    spin_unlock_irq(&task->sighand->siglock);
>> +
>> +out_self:
>> +    spin_unlock_irq(&current->sighand->siglock);
>> +    return ret;
>> +}
>> +#endif
>>
>

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

[Tycho Andersen]

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

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

[Tycho Andersen]

On Wed, Oct 07, 2015 at 12:34:26PM +0200, Daniel Borkmann wrote:
> On 10/07/2015 12:25 PM, Daniel Borkmann wrote:
> >On 10/07/2015 11:46 AM, Tycho Andersen wrote:
> >>This patch adds support for dumping a process' (classic BPF) seccomp
> >>filters via ptrace.
> >>
> >>PTRACE_SECCOMP_GET_FILTER allows the tracer to dump the user's classic BPF
> >>seccomp filters. addr should be an integer which represents the ith seccomp
> >>filter (0 is the most recently installed filter). data should be a struct
> >>sock_filter * with enough room for the ith filter, or NULL, in which case
> >>the filter is not saved. The return value for this command is the number of
> >>BPF instructions the program represents, or negative in the case of errors.
> >>A command specific error is ENOENT, which indicates that there is no ith
> >>filter in this seccomp tree.
> >>
> >>A caveat with this approach is that there is no way to get explicitly at
> >>the heirarchy of seccomp filters, and users need to memcmp() filters to
> >>decide which are inherited. This means that a task which installs two of
> >>the same filter can potentially confuse users of this interface.
> >>
> >>Signed-off-by: Tycho Andersen <tycho.andersen@canonical.com>
> >>CC: Kees Cook <keescook@chromium.org>
> >>CC: Will Drewry <wad@chromium.org>
> >>CC: Oleg Nesterov <oleg@redhat.com>
> >>CC: Andy Lutomirski <luto@amacapital.net>
> >>CC: Pavel Emelyanov <xemul@parallels.com>
> >>CC: Serge E. Hallyn <serge.hallyn@ubuntu.com>
> >>CC: Alexei Starovoitov <ast@kernel.org>
> >>CC: Daniel Borkmann <daniel@iogearbox.net>
> >>---
> >>  include/linux/seccomp.h     | 11 +++++++++
> >>  include/uapi/linux/ptrace.h |  2 ++
> >>  kernel/ptrace.c             |  5 ++++
> >>  kernel/seccomp.c            | 57 ++++++++++++++++++++++++++++++++++++++++++++-
> >>  4 files changed, 74 insertions(+), 1 deletion(-)
> >>
> >>diff --git a/include/linux/seccomp.h b/include/linux/seccomp.h
> >>index f426503..8861b5b 100644
> >>--- a/include/linux/seccomp.h
> >>+++ b/include/linux/seccomp.h
> >>@@ -95,4 +95,15 @@ static inline void get_seccomp_filter(struct task_struct *tsk)
> >>      return;
> >>  }
> >>  #endif /* CONFIG_SECCOMP_FILTER */
> >>+
> >>+#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.)
> >
> >>+}
> >>+#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?
> >
> >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?
> >
> >>+    spin_lock_irq(&current->sighand->siglock);
> >>+    if (!capable(CAP_SYS_ADMIN) ||
> >
> >The capability check should probably happen before taking the task's spinlock.
> >
> >>+        current->seccomp.mode != SECCOMP_MODE_DISABLED) {
> 
> Should this rather be: current->seccomp.mode == SECCOMP_MODE_DISABLED ?
> So that you bail out when seccomp is not in use?

It's an or, so it should bail when seccomp is not disabled, i.e. when
seccomp is enabled.

Tycho

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

[Kees Cook]

On Wed, Oct 7, 2015 at 2:46 AM, Tycho Andersen
<tycho.andersen-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org> wrote:
> This patch adds support for dumping a process' (classic BPF) seccomp
> filters via ptrace.
>
> PTRACE_SECCOMP_GET_FILTER allows the tracer to dump the user's classic BPF
> seccomp filters. addr should be an integer which represents the ith seccomp
> filter (0 is the most recently installed filter). data should be a struct
> sock_filter * with enough room for the ith filter, or NULL, in which case
> the filter is not saved. The return value for this command is the number of
> BPF instructions the program represents, or negative in the case of errors.
> A command specific error is ENOENT, which indicates that there is no ith
> filter in this seccomp tree.
>
> A caveat with this approach is that there is no way to get explicitly at
> the heirarchy of seccomp filters, and users need to memcmp() filters to
> decide which are inherited. This means that a task which installs two of
> the same filter can potentially confuse users of this interface.
>
> Signed-off-by: Tycho Andersen <tycho.andersen-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
> CC: Kees Cook <keescook-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> CC: Will Drewry <wad-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> CC: Oleg Nesterov <oleg-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> CC: Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org>
> CC: Pavel Emelyanov <xemul-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>
> CC: Serge E. Hallyn <serge.hallyn-GeWIH/nMZzLQT0dZR+AlfA@public.gmane.org>
> CC: Alexei Starovoitov <ast-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> CC: Daniel Borkmann <daniel-FeC+5ew28dpmcu3hnIyYJQ@public.gmane.org>
> ---
>  include/linux/seccomp.h     | 11 +++++++++
>  include/uapi/linux/ptrace.h |  2 ++
>  kernel/ptrace.c             |  5 ++++
>  kernel/seccomp.c            | 57 ++++++++++++++++++++++++++++++++++++++++++++-
>  4 files changed, 74 insertions(+), 1 deletion(-)
>
> diff --git a/include/linux/seccomp.h b/include/linux/seccomp.h
> index f426503..8861b5b 100644
> --- a/include/linux/seccomp.h
> +++ b/include/linux/seccomp.h
> @@ -95,4 +95,15 @@ static inline void get_seccomp_filter(struct task_struct *tsk)
>         return;
>  }
>  #endif /* CONFIG_SECCOMP_FILTER */
> +
> +#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;
> +}
> +#endif /* CONFIG_SECCOMP_FILTER && CONFIG_CHECKPOINT_RESTORE */
>  #endif /* _LINUX_SECCOMP_H */
> diff --git a/include/uapi/linux/ptrace.h b/include/uapi/linux/ptrace.h
> index a7a6979..c9d0b21 100644
> --- a/include/uapi/linux/ptrace.h
> +++ b/include/uapi/linux/ptrace.h
> @@ -23,6 +23,8 @@
>
>  #define PTRACE_SYSCALL           24
>
> +#define PTRACE_SECCOMP_GET_FILTER      40
> +
>  /* 0x4200-0x4300 are reserved for architecture-independent additions.  */
>  #define PTRACE_SETOPTIONS      0x4200
>  #define PTRACE_GETEVENTMSG     0x4201
> 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);

Will the compiler do anything fancier here if this is defined "const"?

>
>         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;
> +
> +       spin_lock_irq(&current->sighand->siglock);
> +       if (!capable(CAP_SYS_ADMIN) ||
> +           current->seccomp.mode != SECCOMP_MODE_DISABLED) {
> +               ret = -EACCES;
> +               goto out_self;
> +       }
> +

Should we add a check that task is ptrace-stopped here too?

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

In thinking about this, I think we need to reverse the counter
(especially if we don't check for the process being stopped), since
subsequent calls could change which filter "0" points to. I think 0
should be the filter at the top of the tree. What do you think?

> +
> +       if (!filter) {
> +               ret = -ENOENT;
> +               goto out_task;
> +       }
> +
> +       fprog = filter->prog->orig_prog;
> +
> +       ret = fprog->len;
> +       if (!data)
> +               goto out_task;
> +
> +       if (copy_to_user(data, fprog->filter, bpf_classic_proglen(fprog))) {
> +               ret = -EFAULT;
> +               goto out_task;
> +       }
> +
> +out_task:
> +       spin_unlock_irq(&task->sighand->siglock);
> +
> +out_self:
> +       spin_unlock_irq(&current->sighand->siglock);
> +       return ret;
> +}
> +#endif
> --
> 2.5.0
>

-Kees

-- 
Kees Cook
Chrome OS Security

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

[Tycho Andersen]

On Wed, Oct 07, 2015 at 03:18:09PM -0700, Kees Cook wrote:
> On Wed, Oct 7, 2015 at 2:46 AM, Tycho Andersen
> <tycho.andersen-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org> wrote:
> > This patch adds support for dumping a process' (classic BPF) seccomp
> > filters via ptrace.
> >
> > PTRACE_SECCOMP_GET_FILTER allows the tracer to dump the user's classic BPF
> > seccomp filters. addr should be an integer which represents the ith seccomp
> > filter (0 is the most recently installed filter). data should be a struct
> > sock_filter * with enough room for the ith filter, or NULL, in which case
> > the filter is not saved. The return value for this command is the number of
> > BPF instructions the program represents, or negative in the case of errors.
> > A command specific error is ENOENT, which indicates that there is no ith
> > filter in this seccomp tree.
> >
> > A caveat with this approach is that there is no way to get explicitly at
> > the heirarchy of seccomp filters, and users need to memcmp() filters to
> > decide which are inherited. This means that a task which installs two of
> > the same filter can potentially confuse users of this interface.
> >
> > Signed-off-by: Tycho Andersen <tycho.andersen-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
> > CC: Kees Cook <keescook-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> > CC: Will Drewry <wad-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
> > CC: Oleg Nesterov <oleg-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
> > CC: Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org>
> > CC: Pavel Emelyanov <xemul-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>
> > CC: Serge E. Hallyn <serge.hallyn-GeWIH/nMZzLQT0dZR+AlfA@public.gmane.org>
> > CC: Alexei Starovoitov <ast-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
> > CC: Daniel Borkmann <daniel-FeC+5ew28dpmcu3hnIyYJQ@public.gmane.org>
> > ---
> >  include/linux/seccomp.h     | 11 +++++++++
> >  include/uapi/linux/ptrace.h |  2 ++
> >  kernel/ptrace.c             |  5 ++++
> >  kernel/seccomp.c            | 57 ++++++++++++++++++++++++++++++++++++++++++++-
> >  4 files changed, 74 insertions(+), 1 deletion(-)
> >
> > diff --git a/include/linux/seccomp.h b/include/linux/seccomp.h
> > index f426503..8861b5b 100644
> > --- a/include/linux/seccomp.h
> > +++ b/include/linux/seccomp.h
> > @@ -95,4 +95,15 @@ static inline void get_seccomp_filter(struct task_struct *tsk)
> >         return;
> >  }
> >  #endif /* CONFIG_SECCOMP_FILTER */
> > +
> > +#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;
> > +}
> > +#endif /* CONFIG_SECCOMP_FILTER && CONFIG_CHECKPOINT_RESTORE */
> >  #endif /* _LINUX_SECCOMP_H */
> > diff --git a/include/uapi/linux/ptrace.h b/include/uapi/linux/ptrace.h
> > index a7a6979..c9d0b21 100644
> > --- a/include/uapi/linux/ptrace.h
> > +++ b/include/uapi/linux/ptrace.h
> > @@ -23,6 +23,8 @@
> >
> >  #define PTRACE_SYSCALL           24
> >
> > +#define PTRACE_SECCOMP_GET_FILTER      40
> > +
> >  /* 0x4200-0x4300 are reserved for architecture-independent additions.  */
> >  #define PTRACE_SETOPTIONS      0x4200
> >  #define PTRACE_GETEVENTMSG     0x4201
> > 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);
> 
> Will the compiler do anything fancier here if this is defined "const"?

Not sure, but it certainly won't hurt anything, so I'll make the
change.

> >
> >         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;
> > +
> > +       spin_lock_irq(&current->sighand->siglock);
> > +       if (!capable(CAP_SYS_ADMIN) ||
> > +           current->seccomp.mode != SECCOMP_MODE_DISABLED) {
> > +               ret = -EACCES;
> > +               goto out_self;
> > +       }
> > +
> 
> Should we add a check that task is ptrace-stopped here too?

ptrace does this for us when it calls ptrace_check_attach(), so I
don't think we need to.

> > +       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--;
> > +       }
> 
> In thinking about this, I think we need to reverse the counter
> (especially if we don't check for the process being stopped), since
> subsequent calls could change which filter "0" points to. I think 0
> should be the filter at the top of the tree. What do you think?

The task does have to be stopped, but since I'm reversing it in
userspace anyway, and this would allow us to persist ids across
individual stops, and this would save everyone who uses this API a bit
of headache, I think that makes sense, so I'll make the change.

Any thoughts on Daniel's comments about error codes or whether or not
we need the current task's siglock?

Thanks,

Tycho

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

[Kees Cook]

On Wed, Oct 7, 2015 at 3:25 AM, Daniel Borkmann <daniel-FeC+5ew28dpmcu3hnIyYJQ@public.gmane.org> wrote:
> On 10/07/2015 11:46 AM, Tycho Andersen wrote:
>>
>> This patch adds support for dumping a process' (classic BPF) seccomp
>> filters via ptrace.
>>
>> PTRACE_SECCOMP_GET_FILTER allows the tracer to dump the user's classic BPF
>> seccomp filters. addr should be an integer which represents the ith
>> seccomp
>> filter (0 is the most recently installed filter). data should be a struct
>> sock_filter * with enough room for the ith filter, or NULL, in which case
>> the filter is not saved. The return value for this command is the number
>> of
>> BPF instructions the program represents, or negative in the case of
>> errors.
>> A command specific error is ENOENT, which indicates that there is no ith
>> filter in this seccomp tree.
>>
>> A caveat with this approach is that there is no way to get explicitly at
>> the heirarchy of seccomp filters, and users need to memcmp() filters to
>> decide which are inherited. This means that a task which installs two of
>> the same filter can potentially confuse users of this interface.
>>
>> Signed-off-by: Tycho Andersen <tycho.andersen-Z7WLFzj8eWMS+FvcfC7Uqw@public.gmane.org>
>> CC: Kees Cook <keescook-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
>> CC: Will Drewry <wad-F7+t8E8rja9g9hUCZPvPmw@public.gmane.org>
>> CC: Oleg Nesterov <oleg-H+wXaHxf7aLQT0dZR+AlfA@public.gmane.org>
>> CC: Andy Lutomirski <luto-kltTT9wpgjJwATOyAt5JVQ@public.gmane.org>
>> CC: Pavel Emelyanov <xemul-bzQdu9zFT3WakBO8gow8eQ@public.gmane.org>
>> CC: Serge E. Hallyn <serge.hallyn-GeWIH/nMZzLQT0dZR+AlfA@public.gmane.org>
>> CC: Alexei Starovoitov <ast-DgEjT+Ai2ygdnm+yROfE0A@public.gmane.org>
>> CC: Daniel Borkmann <daniel-FeC+5ew28dpmcu3hnIyYJQ@public.gmane.org>
>> ---
>>   include/linux/seccomp.h     | 11 +++++++++
>>   include/uapi/linux/ptrace.h |  2 ++
>>   kernel/ptrace.c             |  5 ++++
>>   kernel/seccomp.c            | 57
>> ++++++++++++++++++++++++++++++++++++++++++++-
>>   4 files changed, 74 insertions(+), 1 deletion(-)
>>
>> diff --git a/include/linux/seccomp.h b/include/linux/seccomp.h
>> index f426503..8861b5b 100644
>> --- a/include/linux/seccomp.h
>> +++ b/include/linux/seccomp.h
>> @@ -95,4 +95,15 @@ static inline void get_seccomp_filter(struct
>> task_struct *tsk)
>>         return;
>>   }
>>   #endif /* CONFIG_SECCOMP_FILTER */
>> +
>> +#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.)

I'm not sure what the convention should be here. Calling ptrace with
bad args returns -EINVAL, so distinguishing the new command being
missing vs the new command have invalid argument isn't obvious. I
don't think this should return -ENOTSUP since then we only know we've
got a kernel that is new enough to know what PTRACE_SECCOMP_GET_FILTER
is but that it was built without CONFIG_SECCOMP_FILTER and
CONFIG_CHECKPOINT_RESTORE. That's a narrow error condition, so
probably we should stick to -EINVAL.

At least for seccomp, the "is this supported" test has been to send
almost bad arguments and get an EFAULT out of it. That tells you the
feature exists.

In this case, I think -EINVAL makes sense, but I could be talked out of it.

>> +}
>> +#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?
>
> 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?
>
>> +       spin_lock_irq(&current->sighand->siglock);
>> +       if (!capable(CAP_SYS_ADMIN) ||
>
>
> The capability check should probably happen before taking the task's
> spinlock.

I don't have a strong opinion about this. Any races here are crazy
anyway, so I don't think there's a reason to optimize. I'd probably be
conservative and leave the spin lock as-is.

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

Oh yes, please. Gotta double-check !fprog.

>
>
>> +       ret = fprog->len;
>> +       if (!data)
>> +               goto out_task;
>> +
>> +       if (copy_to_user(data, fprog->filter, bpf_classic_proglen(fprog)))
>> {
>> +               ret = -EFAULT;
>> +               goto out_task;
>> +       }
>> +
>> +out_task:
>> +       spin_unlock_irq(&task->sighand->siglock);
>> +
>> +out_self:
>> +       spin_unlock_irq(&current->sighand->siglock);
>> +       return ret;
>> +}
>> +#endif
>>
>

-Kees

-- 
Kees Cook
Chrome OS Security