[PATCH 1/3] seccomp: change return type of seccomp_get_metadata to int

[PATCH 1/3] seccomp: change return type of seccomp_get_metadata to int

[Tycho Andersen]

As Jann pointed out in another thread, ptrace_requiest() returns an int, so
it makes sense for seccomp_get_metdata() to return an int as well. The
return type of seccomp_get_metadata() is bounded by sizeof(kmd), so this
conversion is safe.

Signed-off-by: Tycho Andersen <tycho@tycho.ws>
Reported-by: Jann Horn <jannh@google.com>
CC: Kees Cook <keescook@chromium.org>
CC: Andy Lutomirski <luto@amacapital.net>
---
 include/linux/seccomp.h | 10 +++++-----
 kernel/seccomp.c        |  6 +++---
 2 files changed, 8 insertions(+), 8 deletions(-)

diff --git a/include/linux/seccomp.h b/include/linux/seccomp.h
index e5320f6c8654..af972549a7b4 100644
--- a/include/linux/seccomp.h
+++ b/include/linux/seccomp.h
@@ -96,17 +96,17 @@ static inline void get_seccomp_filter(struct task_struct *tsk)
 #if defined(CONFIG_SECCOMP_FILTER) && defined(CONFIG_CHECKPOINT_RESTORE)
 extern long seccomp_get_filter(struct task_struct *task,
 			       unsigned long filter_off, void __user *data);
-extern long seccomp_get_metadata(struct task_struct *task,
-				 unsigned long filter_off, void __user *data);
+extern int seccomp_get_metadata(struct task_struct *task,
+				unsigned long filter_off, void __user *data);
 #else
 static inline long seccomp_get_filter(struct task_struct *task,
 				      unsigned long n, void __user *data)
 {
 	return -EINVAL;
 }
-static inline long seccomp_get_metadata(struct task_struct *task,
-					unsigned long filter_off,
-					void __user *data)
+static inline int seccomp_get_metadata(struct task_struct *task,
+				       unsigned long filter_off,
+				       void __user *data)
 {
 	return -EINVAL;
 }
diff --git a/kernel/seccomp.c b/kernel/seccomp.c
index fd023ac24e10..9f3721849747 100644
--- a/kernel/seccomp.c
+++ b/kernel/seccomp.c
@@ -1068,10 +1068,10 @@ long seccomp_get_filter(struct task_struct *task, unsigned long filter_off,
 	return ret;
 }
 
-long seccomp_get_metadata(struct task_struct *task,
-			  unsigned long size, void __user *data)
+int seccomp_get_metadata(struct task_struct *task,
+			 unsigned long size, void __user *data)
 {
-	long ret;
+	int ret;
 	struct seccomp_filter *filter;
 	struct seccomp_metadata kmd = {};
 
-- 
2.17.1

[PATCH 2/3] seccomp: change return type of seccomp_get_filter to int

[Tycho Andersen]

As Jann pointed out in another thread, ptrace_requiest() returns an int, so
it makes sense for seccomp_get_filter() to return an int as well. The
return type of seccomp_get_filter() is bounded by the BPF_MAXINSNS check in
seccomp_prepare_filter(), so this conversion is safe.

Signed-off-by: Tycho Andersen <tycho@tycho.ws>
Reported-by: Jann Horn <jannh@google.com>
CC: Kees Cook <keescook@chromium.org>
CC: Andy Lutomirski <luto@amacapital.net>
---
 include/linux/seccomp.h | 6 +++---
 kernel/seccomp.c        | 6 +++---
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/include/linux/seccomp.h b/include/linux/seccomp.h
index af972549a7b4..8429bdda947a 100644
--- a/include/linux/seccomp.h
+++ b/include/linux/seccomp.h
@@ -94,13 +94,13 @@ static inline void get_seccomp_filter(struct task_struct *tsk)
 #endif /* CONFIG_SECCOMP_FILTER */
 
 #if defined(CONFIG_SECCOMP_FILTER) && defined(CONFIG_CHECKPOINT_RESTORE)
-extern long seccomp_get_filter(struct task_struct *task,
+extern int seccomp_get_filter(struct task_struct *task,
 			       unsigned long filter_off, void __user *data);
 extern int seccomp_get_metadata(struct task_struct *task,
 				unsigned long filter_off, void __user *data);
 #else
-static inline long seccomp_get_filter(struct task_struct *task,
-				      unsigned long n, void __user *data)
+static inline int seccomp_get_filter(struct task_struct *task,
+				     unsigned long n, void __user *data)
 {
 	return -EINVAL;
 }
diff --git a/kernel/seccomp.c b/kernel/seccomp.c
index 9f3721849747..ef80dd19f268 100644
--- a/kernel/seccomp.c
+++ b/kernel/seccomp.c
@@ -1030,12 +1030,12 @@ static struct seccomp_filter *get_nth_filter(struct task_struct *task,
 	return filter;
 }
 
-long seccomp_get_filter(struct task_struct *task, unsigned long filter_off,
-			void __user *data)
+int seccomp_get_filter(struct task_struct *task, unsigned long filter_off,
+		       void __user *data)
 {
 	struct seccomp_filter *filter;
 	struct sock_fprog_kern *fprog;
-	long ret;
+	int ret;
 
 	if (!capable(CAP_SYS_ADMIN) ||
 	    current->seccomp.mode != SECCOMP_MODE_DISABLED) {
-- 
2.17.1

[PATCH 3/3] seccomp: introduce read protection for struct seccomp

[Tycho Andersen]

As Jann pointed out, there is a race between SECCOMP_FILTER_FLAG_TSYNC and
the ptrace code that can inspect a filter of another process. Let's
introduce read locking into the two ptrace accesses so that we don't race.

Signed-off-by: Tycho Andersen <tycho@tycho.ws>
Reported-by: Jann Horn <jannh@google.com>
CC: Kees Cook <keescook@chromium.org>
CC: Andy Lutomirski <luto@amacapital.net>
---
 include/linux/seccomp.h |  4 ++--
 kernel/seccomp.c        | 10 ++++++++++
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/include/linux/seccomp.h b/include/linux/seccomp.h
index 8429bdda947a..30b27e898162 100644
--- a/include/linux/seccomp.h
+++ b/include/linux/seccomp.h
@@ -22,8 +22,8 @@ struct seccomp_filter;
  * @filter: must always point to a valid seccomp-filter or NULL as it is
  *          accessed without locking during system call entry.
  *
- *          @filter must only be accessed from the context of current as there
- *          is no read locking.
+ *          @filter is read-protected by task->signal->cred_guard_mutex when
+ *          outside of current context.
  */
 struct seccomp {
 	int mode;
diff --git a/kernel/seccomp.c b/kernel/seccomp.c
index ef80dd19f268..f65d47650ac1 100644
--- a/kernel/seccomp.c
+++ b/kernel/seccomp.c
@@ -1042,7 +1042,12 @@ int seccomp_get_filter(struct task_struct *task, unsigned long filter_off,
 		return -EACCES;
 	}
 
+	ret = mutex_lock_killable(&task->signal->cred_guard_mutex);
+	if (ret < 0)
+		return ret;
+
 	filter = get_nth_filter(task, filter_off);
+	mutex_unlock(&task->signal->cred_guard_mutex);
 	if (IS_ERR(filter))
 		return PTR_ERR(filter);
 
@@ -1088,7 +1093,12 @@ int seccomp_get_metadata(struct task_struct *task,
 	if (copy_from_user(&kmd.filter_off, data, sizeof(kmd.filter_off)))
 		return -EFAULT;
 
+	ret = mutex_lock_killable(&task->signal->cred_guard_mutex);
+	if (ret < 0)
+		return ret;
+
 	filter = get_nth_filter(task, kmd.filter_off);
+	mutex_unlock(&task->signal->cred_guard_mutex);
 	if (IS_ERR(filter))
 		return PTR_ERR(filter);
 
-- 
2.17.1

Re: [PATCH 3/3] seccomp: introduce read protection for struct seccomp

[Jann Horn]

On Fri, Sep 28, 2018 at 5:47 PM Tycho Andersen <tycho@tycho.ws> wrote:
> As Jann pointed out, there is a race between SECCOMP_FILTER_FLAG_TSYNC and
> the ptrace code that can inspect a filter of another process. Let's
> introduce read locking into the two ptrace accesses so that we don't race.

Hmm. Is that true? The ptrace code uses get_nth_filter(), which holds
the siglock while grabbing the seccomp filter and bumping its
refcount. And TSYNC happens from seccomp_set_mode_filter(), which
takes the siglock. So this looks okay to me?

> Signed-off-by: Tycho Andersen <tycho@tycho.ws>
> Reported-by: Jann Horn <jannh@google.com>
> CC: Kees Cook <keescook@chromium.org>
> CC: Andy Lutomirski <luto@amacapital.net>
> ---
>  include/linux/seccomp.h |  4 ++--
>  kernel/seccomp.c        | 10 ++++++++++
>  2 files changed, 12 insertions(+), 2 deletions(-)
>
> diff --git a/include/linux/seccomp.h b/include/linux/seccomp.h
> index 8429bdda947a..30b27e898162 100644
> --- a/include/linux/seccomp.h
> +++ b/include/linux/seccomp.h
> @@ -22,8 +22,8 @@ struct seccomp_filter;
>   * @filter: must always point to a valid seccomp-filter or NULL as it is
>   *          accessed without locking during system call entry.
>   *
> - *          @filter must only be accessed from the context of current as there
> - *          is no read locking.
> + *          @filter is read-protected by task->signal->cred_guard_mutex when
> + *          outside of current context.
>   */
>  struct seccomp {
>         int mode;
> diff --git a/kernel/seccomp.c b/kernel/seccomp.c
> index ef80dd19f268..f65d47650ac1 100644
> --- a/kernel/seccomp.c
> +++ b/kernel/seccomp.c
> @@ -1042,7 +1042,12 @@ int seccomp_get_filter(struct task_struct *task, unsigned long filter_off,
>                 return -EACCES;
>         }
>
> +       ret = mutex_lock_killable(&task->signal->cred_guard_mutex);
> +       if (ret < 0)
> +               return ret;
> +
>         filter = get_nth_filter(task, filter_off);
> +       mutex_unlock(&task->signal->cred_guard_mutex);
>         if (IS_ERR(filter))
>                 return PTR_ERR(filter);
>
> @@ -1088,7 +1093,12 @@ int seccomp_get_metadata(struct task_struct *task,
>         if (copy_from_user(&kmd.filter_off, data, sizeof(kmd.filter_off)))
>                 return -EFAULT;
>
> +       ret = mutex_lock_killable(&task->signal->cred_guard_mutex);
> +       if (ret < 0)
> +               return ret;
> +
>         filter = get_nth_filter(task, kmd.filter_off);
> +       mutex_unlock(&task->signal->cred_guard_mutex);
>         if (IS_ERR(filter))
>                 return PTR_ERR(filter);
>
> --
> 2.17.1
>

Re: [PATCH 3/3] seccomp: introduce read protection for struct seccomp

[Tycho Andersen]

On Fri, Sep 28, 2018 at 10:33:34PM +0200, Jann Horn wrote:
> On Fri, Sep 28, 2018 at 5:47 PM Tycho Andersen <tycho@tycho.ws> wrote:
> > As Jann pointed out, there is a race between SECCOMP_FILTER_FLAG_TSYNC and
> > the ptrace code that can inspect a filter of another process. Let's
> > introduce read locking into the two ptrace accesses so that we don't race.
> 
> Hmm. Is that true? The ptrace code uses get_nth_filter(), which holds
> the siglock while grabbing the seccomp filter and bumping its
> refcount. And TSYNC happens from seccomp_set_mode_filter(), which
> takes the siglock. So this looks okay to me?

Oh, yes, you're right. So I guess we should just change the comment to
say we're using siglock to represent the read lock.

Tycho

Re: [PATCH 3/3] seccomp: introduce read protection for struct seccomp

[Jann Horn]

On Fri, Sep 28, 2018 at 10:56 PM Tycho Andersen <tycho@tycho.ws> wrote:
>
> On Fri, Sep 28, 2018 at 10:33:34PM +0200, Jann Horn wrote:
> > On Fri, Sep 28, 2018 at 5:47 PM Tycho Andersen <tycho@tycho.ws> wrote:
> > > As Jann pointed out, there is a race between SECCOMP_FILTER_FLAG_TSYNC and
> > > the ptrace code that can inspect a filter of another process. Let's
> > > introduce read locking into the two ptrace accesses so that we don't race.
> >
> > Hmm. Is that true? The ptrace code uses get_nth_filter(), which holds
> > the siglock while grabbing the seccomp filter and bumping its
> > refcount. And TSYNC happens from seccomp_set_mode_filter(), which
> > takes the siglock. So this looks okay to me?
>
> Oh, yes, you're right. So I guess we should just change the comment to
> say we're using siglock to represent the read lock.

Hmm... actually, looking at this closer, I think you only need the
siglock for writing. As far as I can tell, any read (no matter if
current or non-current) can just use READ_ONCE(), because once a
seccomp filter is in a task's seccomp filter chain, it can't be freed
until the task reaches free_task() and calls put_seccomp_filter() from
there. And if the task whose seccomp filter you're trying to read can
reach free_task(), you have bigger problems.

Re: [PATCH 3/3] seccomp: introduce read protection for struct seccomp

[Tycho Andersen]

On Fri, Sep 28, 2018 at 11:10:48PM +0200, Jann Horn wrote:
> On Fri, Sep 28, 2018 at 10:56 PM Tycho Andersen <tycho@tycho.ws> wrote:
> >
> > On Fri, Sep 28, 2018 at 10:33:34PM +0200, Jann Horn wrote:
> > > On Fri, Sep 28, 2018 at 5:47 PM Tycho Andersen <tycho@tycho.ws> wrote:
> > > > As Jann pointed out, there is a race between SECCOMP_FILTER_FLAG_TSYNC and
> > > > the ptrace code that can inspect a filter of another process. Let's
> > > > introduce read locking into the two ptrace accesses so that we don't race.
> > >
> > > Hmm. Is that true? The ptrace code uses get_nth_filter(), which holds
> > > the siglock while grabbing the seccomp filter and bumping its
> > > refcount. And TSYNC happens from seccomp_set_mode_filter(), which
> > > takes the siglock. So this looks okay to me?
> >
> > Oh, yes, you're right. So I guess we should just change the comment to
> > say we're using siglock to represent the read lock.
> 
> Hmm... actually, looking at this closer, I think you only need the
> siglock for writing. As far as I can tell, any read (no matter if
> current or non-current) can just use READ_ONCE(), because once a
> seccomp filter is in a task's seccomp filter chain, it can't be freed
> until the task reaches free_task() and calls put_seccomp_filter() from
> there. And if the task whose seccomp filter you're trying to read can
> reach free_task(), you have bigger problems.

Ok; looks like get_nth_filter() took the siglock anyway. Since we get
the filters in these two functions in get_nth_filter(), I think it's
enough just to just,

diff --git a/kernel/seccomp.c b/kernel/seccomp.c
index f65d47650ac1..79d833ed4c34 100644
--- a/kernel/seccomp.c
+++ b/kernel/seccomp.c
@@ -1001,7 +1001,7 @@ static struct seccomp_filter *get_nth_filter(struct task_struct *task,
 		return ERR_PTR(-EINVAL);
 	}
 
-	orig = task->seccomp.filter;
+	orig = READ_ONCE(task->seccomp.filter);
 	__get_seccomp_filter(orig);
 	spin_unlock_irq(&task->sighand->siglock);
 

since once it's returned from get_nth_filter() we don't need to worry
about multiple accesses?

Tycho

Re: [PATCH 3/3] seccomp: introduce read protection for struct seccomp

[Jann Horn]

On Fri, Sep 28, 2018 at 11:36 PM Tycho Andersen <tycho@tycho.ws> wrote:
> On Fri, Sep 28, 2018 at 11:10:48PM +0200, Jann Horn wrote:
> > On Fri, Sep 28, 2018 at 10:56 PM Tycho Andersen <tycho@tycho.ws> wrote:
> > >
> > > On Fri, Sep 28, 2018 at 10:33:34PM +0200, Jann Horn wrote:
> > > > On Fri, Sep 28, 2018 at 5:47 PM Tycho Andersen <tycho@tycho.ws> wrote:
> > > > > As Jann pointed out, there is a race between SECCOMP_FILTER_FLAG_TSYNC and
> > > > > the ptrace code that can inspect a filter of another process. Let's
> > > > > introduce read locking into the two ptrace accesses so that we don't race.
> > > >
> > > > Hmm. Is that true? The ptrace code uses get_nth_filter(), which holds
> > > > the siglock while grabbing the seccomp filter and bumping its
> > > > refcount. And TSYNC happens from seccomp_set_mode_filter(), which
> > > > takes the siglock. So this looks okay to me?
> > >
> > > Oh, yes, you're right. So I guess we should just change the comment to
> > > say we're using siglock to represent the read lock.
> >
> > Hmm... actually, looking at this closer, I think you only need the
> > siglock for writing. As far as I can tell, any read (no matter if
> > current or non-current) can just use READ_ONCE(), because once a
> > seccomp filter is in a task's seccomp filter chain, it can't be freed
> > until the task reaches free_task() and calls put_seccomp_filter() from
> > there. And if the task whose seccomp filter you're trying to read can
> > reach free_task(), you have bigger problems.
>
> Ok; looks like get_nth_filter() took the siglock anyway. Since we get
> the filters in these two functions in get_nth_filter(), I think it's
> enough just to just,
>
> diff --git a/kernel/seccomp.c b/kernel/seccomp.c
> index f65d47650ac1..79d833ed4c34 100644
> --- a/kernel/seccomp.c
> +++ b/kernel/seccomp.c
> @@ -1001,7 +1001,7 @@ static struct seccomp_filter *get_nth_filter(struct task_struct *task,
>                 return ERR_PTR(-EINVAL);
>         }
>
> -       orig = task->seccomp.filter;
> +       orig = READ_ONCE(task->seccomp.filter);
>         __get_seccomp_filter(orig);
>         spin_unlock_irq(&task->sighand->siglock);

Huh? Now you're holding the siglock *and* you're using READ_ONCE()?
I'm not sure what you're trying to accomplish here.

> since once it's returned from get_nth_filter() we don't need to worry
> about multiple accesses?
>
> Tycho

Re: [PATCH 3/3] seccomp: introduce read protection for struct seccomp

[Tycho Andersen]

On Fri, Sep 28, 2018 at 11:54:22PM +0200, Jann Horn wrote:
> On Fri, Sep 28, 2018 at 11:36 PM Tycho Andersen <tycho@tycho.ws> wrote:
> > On Fri, Sep 28, 2018 at 11:10:48PM +0200, Jann Horn wrote:
> > > On Fri, Sep 28, 2018 at 10:56 PM Tycho Andersen <tycho@tycho.ws> wrote:
> > > >
> > > > On Fri, Sep 28, 2018 at 10:33:34PM +0200, Jann Horn wrote:
> > > > > On Fri, Sep 28, 2018 at 5:47 PM Tycho Andersen <tycho@tycho.ws> wrote:
> > > > > > As Jann pointed out, there is a race between SECCOMP_FILTER_FLAG_TSYNC and
> > > > > > the ptrace code that can inspect a filter of another process. Let's
> > > > > > introduce read locking into the two ptrace accesses so that we don't race.
> > > > >
> > > > > Hmm. Is that true? The ptrace code uses get_nth_filter(), which holds
> > > > > the siglock while grabbing the seccomp filter and bumping its
> > > > > refcount. And TSYNC happens from seccomp_set_mode_filter(), which
> > > > > takes the siglock. So this looks okay to me?
> > > >
> > > > Oh, yes, you're right. So I guess we should just change the comment to
> > > > say we're using siglock to represent the read lock.
> > >
> > > Hmm... actually, looking at this closer, I think you only need the
> > > siglock for writing. As far as I can tell, any read (no matter if
> > > current or non-current) can just use READ_ONCE(), because once a
> > > seccomp filter is in a task's seccomp filter chain, it can't be freed
> > > until the task reaches free_task() and calls put_seccomp_filter() from
> > > there. And if the task whose seccomp filter you're trying to read can
> > > reach free_task(), you have bigger problems.
> >
> > Ok; looks like get_nth_filter() took the siglock anyway. Since we get
> > the filters in these two functions in get_nth_filter(), I think it's
> > enough just to just,
> >
> > diff --git a/kernel/seccomp.c b/kernel/seccomp.c
> > index f65d47650ac1..79d833ed4c34 100644
> > --- a/kernel/seccomp.c
> > +++ b/kernel/seccomp.c
> > @@ -1001,7 +1001,7 @@ static struct seccomp_filter *get_nth_filter(struct task_struct *task,
> >                 return ERR_PTR(-EINVAL);
> >         }
> >
> > -       orig = task->seccomp.filter;
> > +       orig = READ_ONCE(task->seccomp.filter);
> >         __get_seccomp_filter(orig);
> >         spin_unlock_irq(&task->sighand->siglock);
> 
> Huh? Now you're holding the siglock *and* you're using READ_ONCE()?
> I'm not sure what you're trying to accomplish here.

Yes, let's just drop this patch all together.

Tycho