Re: [PATCH 3/3] seccomp: introduce read protection for struct seccomp
On Fri, Sep 28, 2018 at 11:54:22PM +0200, Jann Horn wrote: > On Fri, Sep 28, 2018 at 11:36 PM Tycho Andersen 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 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 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(>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
Re: [PATCH 3/3] seccomp: introduce read protection for struct seccomp
On Fri, Sep 28, 2018 at 11:54:22PM +0200, Jann Horn wrote: > On Fri, Sep 28, 2018 at 11:36 PM Tycho Andersen 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 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 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(>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
Re: [PATCH 3/3] seccomp: introduce read protection for struct seccomp
On Fri, Sep 28, 2018 at 11:36 PM Tycho Andersen 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 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 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(>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
On Fri, Sep 28, 2018 at 11:36 PM Tycho Andersen 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 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 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(>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
On Fri, Sep 28, 2018 at 11:10:48PM +0200, Jann Horn wrote: > On Fri, Sep 28, 2018 at 10:56 PM Tycho Andersen 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 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(>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
On Fri, Sep 28, 2018 at 11:10:48PM +0200, Jann Horn wrote: > On Fri, Sep 28, 2018 at 10:56 PM Tycho Andersen 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 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(>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
On Fri, Sep 28, 2018 at 10:56 PM Tycho Andersen 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 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
On Fri, Sep 28, 2018 at 10:56 PM Tycho Andersen 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 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
On Fri, Sep 28, 2018 at 10:33:34PM +0200, Jann Horn wrote: > On Fri, Sep 28, 2018 at 5:47 PM Tycho Andersen 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
On Fri, Sep 28, 2018 at 10:33:34PM +0200, Jann Horn wrote: > On Fri, Sep 28, 2018 at 5:47 PM Tycho Andersen 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
On Fri, Sep 28, 2018 at 5:47 PM Tycho Andersen 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
> Reported-by: Jann Horn
> CC: Kees Cook
> CC: Andy Lutomirski
> ---
> 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(>signal->cred_guard_mutex);
> + if (ret < 0)
> + return ret;
> +
> filter = get_nth_filter(task, filter_off);
> + mutex_unlock(>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(_off, data, sizeof(kmd.filter_off)))
> return -EFAULT;
>
> + ret = mutex_lock_killable(>signal->cred_guard_mutex);
> + if (ret < 0)
> + return ret;
> +
> filter = get_nth_filter(task, kmd.filter_off);
> + mutex_unlock(>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
On Fri, Sep 28, 2018 at 5:47 PM Tycho Andersen 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
> Reported-by: Jann Horn
> CC: Kees Cook
> CC: Andy Lutomirski
> ---
> 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(>signal->cred_guard_mutex);
> + if (ret < 0)
> + return ret;
> +
> filter = get_nth_filter(task, filter_off);
> + mutex_unlock(>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(_off, data, sizeof(kmd.filter_off)))
> return -EFAULT;
>
> + ret = mutex_lock_killable(>signal->cred_guard_mutex);
> + if (ret < 0)
> + return ret;
> +
> filter = get_nth_filter(task, kmd.filter_off);
> + mutex_unlock(>signal->cred_guard_mutex);
> if (IS_ERR(filter))
> return PTR_ERR(filter);
>
> --
> 2.17.1
>
[PATCH 3/3] seccomp: introduce read protection for struct seccomp
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
Reported-by: Jann Horn
CC: Kees Cook
CC: Andy Lutomirski
---
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(>signal->cred_guard_mutex);
+ if (ret < 0)
+ return ret;
+
filter = get_nth_filter(task, filter_off);
+ mutex_unlock(>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(_off, data, sizeof(kmd.filter_off)))
return -EFAULT;
+ ret = mutex_lock_killable(>signal->cred_guard_mutex);
+ if (ret < 0)
+ return ret;
+
filter = get_nth_filter(task, kmd.filter_off);
+ mutex_unlock(>signal->cred_guard_mutex);
if (IS_ERR(filter))
return PTR_ERR(filter);
--
2.17.1
[PATCH 3/3] seccomp: introduce read protection for struct seccomp
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
Reported-by: Jann Horn
CC: Kees Cook
CC: Andy Lutomirski
---
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(>signal->cred_guard_mutex);
+ if (ret < 0)
+ return ret;
+
filter = get_nth_filter(task, filter_off);
+ mutex_unlock(>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(_off, data, sizeof(kmd.filter_off)))
return -EFAULT;
+ ret = mutex_lock_killable(>signal->cred_guard_mutex);
+ if (ret < 0)
+ return ret;
+
filter = get_nth_filter(task, kmd.filter_off);
+ mutex_unlock(>signal->cred_guard_mutex);
if (IS_ERR(filter))
return PTR_ERR(filter);
--
2.17.1
