LinuxLists.cc - [PATCH] seccomp: fix the usage of get/put_seccomp_filter() in seccomp_get

[permalink] [raw]

Subject: Re: [PATCH] seccomp: fix the usage of get/put_seccomp_filter() in seccomp_get_filter()

On 09/20, Oleg Nesterov wrote:
>
> @@ -908,13 +912,13 @@ long seccomp_get_filter(struct task_struct *task, unsigned long filter_off,
> if (!data)
> goto out;
>
> - get_seccomp_filter(task);
> + refcount_inc(&filter->usage);
> spin_unlock_irq(&task->sighand->siglock);
>
> if (copy_to_user(data, fprog->filter, bpf_classic_proglen(fprog)))
> ret = -EFAULT;
>
> - put_seccomp_filter(task);
> + __put_seccomp_filter(filter);

This is the simple fix for -stable, but again, can't we simplify this
code? Afaics we can do get_seccomp_filter() at the start and drop siglock
right after that.

Something like the untested patch (on top of this one) below?

And I can't understand the SECCOMP_MODE_DISABLED check... shouldn't we
simply remove it?

Oleg.

--- x/kernel/seccomp.c
+++ x/kernel/seccomp.c
@@ -858,45 +858,36 @@ long prctl_set_seccomp(unsigned long seccomp_mode, char __user *filter)
long seccomp_get_filter(struct task_struct *task, unsigned long filter_off,
void __user *data)
{
- struct seccomp_filter *filter;
+ struct seccomp_filter *orig, *filter;
struct sock_fprog_kern *fprog;
+ unsigned long count;
long ret;
- unsigned long count = 0;

if (!capable(CAP_SYS_ADMIN) ||
current->seccomp.mode != SECCOMP_MODE_DISABLED) {
return -EACCES;
}

+ if (task->seccomp.mode != SECCOMP_MODE_FILTER)
+ return -EINVAL;
+
spin_lock_irq(&task->sighand->siglock);
- if (task->seccomp.mode != SECCOMP_MODE_FILTER) {
- ret = -EINVAL;
- goto out;
- }
+ get_seccomp_filter(task);
+ orig = task->seccomp.filter;
+ spin_unlock_irq(&task->sighand->siglock);

- filter = task->seccomp.filter;
- while (filter) {
- filter = filter->prev;
+ count = 0;
+ for (filter = orig; filter; filter = filter->prev)
count++;
- }

if (filter_off >= count) {
ret = -ENOENT;
goto out;
}
- count -= filter_off;

- filter = task->seccomp.filter;
- while (filter && count > 1) {
- filter = filter->prev;
+ count -= filter_off;
+ for (filter = orig; count > 1; filter = filter->prev)
count--;
- }
-
- if (WARN_ON(count != 1 || !filter)) {
- /* The filter tree shouldn't shrink while we're using it. */
- ret = -ENOENT;
- goto out;
- }

fprog = filter->prog->orig_prog;
if (!fprog) {
@@ -912,17 +903,11 @@ long seccomp_get_filter(struct task_struct *task, unsigned long filter_off,
if (!data)
goto out;

- refcount_inc(&filter->usage);
- spin_unlock_irq(&task->sighand->siglock);
-
if (copy_to_user(data, fprog->filter, bpf_classic_proglen(fprog)))
ret = -EFAULT;

- __put_seccomp_filter(filter);
- return ret;
-
out:
- spin_unlock_irq(&task->sighand->siglock);
+ __put_seccomp_filter(orig);
return ret;
}
#endif

2017-09-20 13:26:15

[permalink] [raw]

Subject: Re: [PATCH] seccomp: fix the usage of get/put_seccomp_filter() in seccomp_get_filter()

On Wed, Sep 20, 2017 at 02:56:21PM +0200, Oleg Nesterov wrote:
> As Chris explains, get_seccomp_filter() and put_seccomp_filter() can
> use the different filters, once we drop ->siglock task->seccomp.filter
> can be replaced by SECCOMP_FILTER_FLAG_TSYNC.
>
> Fixes: f8e529ed941b ("seccomp, ptrace: add support for dumping seccomp filters")
> Reported-by: Chris Salls <[email protected]>
> Cc: [email protected]
> Signed-off-by: Oleg Nesterov <[email protected]>

Ugh! Whoops.

Acked-by: Tycho Andersen <[email protected]>

> ---
> kernel/seccomp.c | 14 +++++++++-----
> 1 file changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/kernel/seccomp.c b/kernel/seccomp.c
> index 98b59b5..897f153 100644
> --- a/kernel/seccomp.c
> +++ b/kernel/seccomp.c
> @@ -476,10 +476,8 @@ static inline void seccomp_filter_free(struct seccomp_filter *filter)
> }
> }
>
> -/* put_seccomp_filter - decrements the ref count of tsk->seccomp.filter */
> -void put_seccomp_filter(struct task_struct *tsk)
> +static void __put_seccomp_filter(struct seccomp_filter *orig)
> {
> - struct seccomp_filter *orig = tsk->seccomp.filter;
> /* Clean up single-reference branches iteratively. */
> while (orig && refcount_dec_and_test(&orig->usage)) {
> struct seccomp_filter *freeme = orig;
> @@ -488,6 +486,12 @@ void put_seccomp_filter(struct task_struct *tsk)
> }
> }
>
> +/* put_seccomp_filter - decrements the ref count of tsk->seccomp.filter */
> +void put_seccomp_filter(struct task_struct *tsk)
> +{
> + __put_seccomp_filter(tsk->seccomp.filter);
> +}
> +
> static void seccomp_init_siginfo(siginfo_t *info, int syscall, int reason)
> {
> memset(info, 0, sizeof(*info));
> @@ -908,13 +912,13 @@ long seccomp_get_filter(struct task_struct *task, unsigned long filter_off,
> if (!data)
> goto out;
>
> - get_seccomp_filter(task);
> + refcount_inc(&filter->usage);
> spin_unlock_irq(&task->sighand->siglock);
>
> if (copy_to_user(data, fprog->filter, bpf_classic_proglen(fprog)))
> ret = -EFAULT;
>
> - put_seccomp_filter(task);
> + __put_seccomp_filter(filter);
> return ret;
>
> out:
> --
> 2.5.0
>
>

2017-09-20 13:37:33

[permalink] [raw]

Subject: Re: [PATCH] seccomp: fix the usage of get/put_seccomp_filter() in seccomp_get_filter()

On Wed, Sep 20, 2017 at 03:04:43PM +0200, Oleg Nesterov wrote:
> On 09/20, Oleg Nesterov wrote:
> >
> > @@ -908,13 +912,13 @@ long seccomp_get_filter(struct task_struct *task, unsigned long filter_off,
> > if (!data)
> > goto out;
> >
> > - get_seccomp_filter(task);
> > + refcount_inc(&filter->usage);
> > spin_unlock_irq(&task->sighand->siglock);
> >
> > if (copy_to_user(data, fprog->filter, bpf_classic_proglen(fprog)))
> > ret = -EFAULT;
> >
> > - put_seccomp_filter(task);
> > + __put_seccomp_filter(filter);
>
> This is the simple fix for -stable, but again, can't we simplify this
> code? Afaics we can do get_seccomp_filter() at the start and drop siglock
> right after that.
>
> Something like the untested patch (on top of this one) below?

Yes, this looks good to me, thanks.

> And I can't understand the SECCOMP_MODE_DISABLED check... shouldn't we
> simply remove it?

I think the idea was to prevent some interaction between
seccomp+ptrace+fork that we didn't understand. Since the user of this
code doesn't have seccomp filters attached, it was fine.

Thanks for cleaning this up, I'll be happy to test whatever final
patch we come up with.

Tycho

> Oleg.
>
>
> --- x/kernel/seccomp.c
> +++ x/kernel/seccomp.c
> @@ -858,45 +858,36 @@ long prctl_set_seccomp(unsigned long seccomp_mode, char __user *filter)
> long seccomp_get_filter(struct task_struct *task, unsigned long filter_off,
> void __user *data)
> {
> - struct seccomp_filter *filter;
> + struct seccomp_filter *orig, *filter;
> struct sock_fprog_kern *fprog;
> + unsigned long count;
> long ret;
> - unsigned long count = 0;
>
> if (!capable(CAP_SYS_ADMIN) ||
> current->seccomp.mode != SECCOMP_MODE_DISABLED) {
> return -EACCES;
> }
>
> + if (task->seccomp.mode != SECCOMP_MODE_FILTER)
> + return -EINVAL;
> +
> spin_lock_irq(&task->sighand->siglock);
> - if (task->seccomp.mode != SECCOMP_MODE_FILTER) {
> - ret = -EINVAL;
> - goto out;
> - }
> + get_seccomp_filter(task);
> + orig = task->seccomp.filter;
> + spin_unlock_irq(&task->sighand->siglock);
>
> - filter = task->seccomp.filter;
> - while (filter) {
> - filter = filter->prev;
> + count = 0;
> + for (filter = orig; filter; filter = filter->prev)
> count++;
> - }
>
> if (filter_off >= count) {
> ret = -ENOENT;
> goto out;
> }
> - count -= filter_off;
>
> - filter = task->seccomp.filter;
> - while (filter && count > 1) {
> - filter = filter->prev;
> + count -= filter_off;
> + for (filter = orig; count > 1; filter = filter->prev)
> count--;
> - }
> -
> - if (WARN_ON(count != 1 || !filter)) {
> - /* The filter tree shouldn't shrink while we're using it. */
> - ret = -ENOENT;
> - goto out;
> - }
>
> fprog = filter->prog->orig_prog;
> if (!fprog) {
> @@ -912,17 +903,11 @@ long seccomp_get_filter(struct task_struct *task, unsigned long filter_off,
> if (!data)
> goto out;
>
> - refcount_inc(&filter->usage);
> - spin_unlock_irq(&task->sighand->siglock);
> -
> if (copy_to_user(data, fprog->filter, bpf_classic_proglen(fprog)))
> ret = -EFAULT;
>
> - __put_seccomp_filter(filter);
> - return ret;
> -
> out:
> - spin_unlock_irq(&task->sighand->siglock);
> + __put_seccomp_filter(orig);
> return ret;
> }
> #endif
>

2017-09-20 15:59:41

[permalink] [raw]

Subject: introduce get_nth_filter()

On 09/20, Tycho Andersen wrote:
>
> Thanks for cleaning this up, I'll be happy to test whatever final
> patch we come up with.

Well, I just noticed you sent another "[PATCH] ptrace, seccomp: add support
for retrieving seccomp flags" today...

So if we need get_nth() helper please consider the UNTESTED change below
(on top of this fix). If you agree with this code, feel free to incorporate
it into your patch.

Oleg.

--- a/kernel/seccomp.c
+++ b/kernel/seccomp.c
@@ -855,49 +855,54 @@ long prctl_set_seccomp(unsigned long seccomp_mode, char __user *filter)
}

#if defined(CONFIG_SECCOMP_FILTER) && defined(CONFIG_CHECKPOINT_RESTORE)
-long seccomp_get_filter(struct task_struct *task, unsigned long filter_off,
- void __user *data)
+static struct seccomp_filter *
+get_nth_filter(struct task_struct *task, unsigned long filter_off)
{
- struct seccomp_filter *filter;
- struct sock_fprog_kern *fprog;
- long ret;
- unsigned long count = 0;
+ struct seccomp_filter *orig, *filter;
+ unsigned long count;

- if (!capable(CAP_SYS_ADMIN) ||
- current->seccomp.mode != SECCOMP_MODE_DISABLED) {
- return -EACCES;
- }
+ if (task->seccomp.mode != SECCOMP_MODE_FILTER)
+ return ERR_PTR(-EINVAL);

spin_lock_irq(&task->sighand->siglock);
- if (task->seccomp.mode != SECCOMP_MODE_FILTER) {
- ret = -EINVAL;
- goto out;
- }
+ get_seccomp_filter(task);
+ orig = task->seccomp.filter;
+ spin_unlock_irq(&task->sighand->siglock);

- filter = task->seccomp.filter;
- while (filter) {
- filter = filter->prev;
+ count = 0;
+ for (filter = orig; filter; filter = filter->prev)
count++;
- }

- if (filter_off >= count) {
- ret = -ENOENT;
+ filter = ERR_PTR(-ENOENT);
+ if (filter_off >= count)
goto out;
- }
- count -= filter_off;

- filter = task->seccomp.filter;
- while (filter && count > 1) {
- filter = filter->prev;
+ count -= filter_off;
+ for (filter = orig; count > 1; filter = filter->prev)
count--;
- }

- if (WARN_ON(count != 1 || !filter)) {
- /* The filter tree shouldn't shrink while we're using it. */
- ret = -ENOENT;
- goto out;
+ refcount_inc(&filter->usage);
+out:
+ __put_seccomp_filter(orig);
+ return filter;
+}
+
+long 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;
+
+ if (!capable(CAP_SYS_ADMIN) ||
+ current->seccomp.mode != SECCOMP_MODE_DISABLED) {
+ return -EACCES;
}

+ filter = get_nth_filter(task, filter_off);
+ if (IS_ERR(filter))
+ return PTR_ERR(filter);
+
fprog = filter->prog->orig_prog;
if (!fprog) {
/* This must be a new non-cBPF filter, since we save
@@ -912,17 +917,10 @@ long seccomp_get_filter(struct task_struct *task, unsigned long filter_off,
if (!data)
goto out;

- refcount_inc(&filter->usage);
- spin_unlock_irq(&task->sighand->siglock);
-
if (copy_to_user(data, fprog->filter, bpf_classic_proglen(fprog)))
ret = -EFAULT;
-
- __put_seccomp_filter(filter);
- return ret;
-
out:
- spin_unlock_irq(&task->sighand->siglock);
+ __put_seccomp_filter(filter);
return ret;
}
#endif

2017-09-20 16:14:29

[permalink] [raw]

Subject: Re: introduce get_nth_filter()

On 09/20, Oleg Nesterov wrote:
>
> On 09/20, Tycho Andersen wrote:
> >
> > Thanks for cleaning this up, I'll be happy to test whatever final
> > patch we come up with.
>
> Well, I just noticed you sent another "[PATCH] ptrace, seccomp: add support
> for retrieving seccomp flags" today...
>
> So if we need get_nth() helper please consider the UNTESTED change below
> (on top of this fix). If you agree with this code, feel free to incorporate
> it into your patch.

and probably we should shift the CAP_SYS_ADMIN/SECCOMP_MODE_DISABLED into
get_nth() too, see v2 below.

Perhaps it makes sense to add a comment to explain that spin_lock_irq(siglock)
is only correct because the caller is the tracer, and thus the TASK_TRACED
"task" can't exit. Otherwise we would need lock_task_sighand().

Oleg.

--- a/kernel/seccomp.c
+++ b/kernel/seccomp.c
@@ -855,48 +855,53 @@ long prctl_set_seccomp(unsigned long seccomp_mode, char __user *filter)
}

#if defined(CONFIG_SECCOMP_FILTER) && defined(CONFIG_CHECKPOINT_RESTORE)
-long seccomp_get_filter(struct task_struct *task, unsigned long filter_off,
- void __user *data)
+static struct seccomp_filter *
+get_nth_filter(struct task_struct *task, unsigned long filter_off)
{
- struct seccomp_filter *filter;
- struct sock_fprog_kern *fprog;
- long ret;
- unsigned long count = 0;
+ struct seccomp_filter *orig, *filter;
+ unsigned long count;

if (!capable(CAP_SYS_ADMIN) ||
current->seccomp.mode != SECCOMP_MODE_DISABLED) {
- return -EACCES;
+ return ERR_PTR(-EACCES);
}

+ if (task->seccomp.mode != SECCOMP_MODE_FILTER)
+ return ERR_PTR(-EINVAL);
+
spin_lock_irq(&task->sighand->siglock);
- if (task->seccomp.mode != SECCOMP_MODE_FILTER) {
- ret = -EINVAL;
- goto out;
- }
+ get_seccomp_filter(task);
+ orig = task->seccomp.filter;
+ spin_unlock_irq(&task->sighand->siglock);

- filter = task->seccomp.filter;
- while (filter) {
- filter = filter->prev;
+ count = 0;
+ for (filter = orig; filter; filter = filter->prev)
count++;
- }

- if (filter_off >= count) {
- ret = -ENOENT;
+ filter = ERR_PTR(-ENOENT);
+ if (filter_off >= count)
goto out;
- }
- count -= filter_off;

- filter = task->seccomp.filter;
- while (filter && count > 1) {
- filter = filter->prev;
+ count -= filter_off;
+ for (filter = orig; count > 1; filter = filter->prev)
count--;
- }

- if (WARN_ON(count != 1 || !filter)) {
- /* The filter tree shouldn't shrink while we're using it. */
- ret = -ENOENT;
- goto out;
- }
+ refcount_inc(&filter->usage);
+out:
+ __put_seccomp_filter(orig);
+ return filter;
+}
+
+long 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;
+
+ filter = get_nth_filter(task, filter_off);
+ if (IS_ERR(filter))
+ return PTR_ERR(filter);

fprog = filter->prog->orig_prog;
if (!fprog) {
@@ -912,17 +917,10 @@ long seccomp_get_filter(struct task_struct *task, unsigned long filter_off,
if (!data)
goto out;

- refcount_inc(&filter->usage);
- spin_unlock_irq(&task->sighand->siglock);
-
if (copy_to_user(data, fprog->filter, bpf_classic_proglen(fprog)))
ret = -EFAULT;
-
- __put_seccomp_filter(filter);
- return ret;
-
out:
- spin_unlock_irq(&task->sighand->siglock);
+ __put_seccomp_filter(filter);
return ret;
}
#endif

2017-09-20 18:36:33

[permalink] [raw]

Subject: Re: [PATCH] seccomp: fix the usage of get/put_seccomp_filter() in seccomp_get_filter()

On Wed, Sep 20, 2017 at 5:56 AM, Oleg Nesterov <[email protected]> wrote:
> As Chris explains, get_seccomp_filter() and put_seccomp_filter() can
> use the different filters, once we drop ->siglock task->seccomp.filter
> can be replaced by SECCOMP_FILTER_FLAG_TSYNC.
>
> Fixes: f8e529ed941b ("seccomp, ptrace: add support for dumping seccomp filters")
> Reported-by: Chris Salls <[email protected]>
> Cc: [email protected]
> Signed-off-by: Oleg Nesterov <[email protected]>
> ---
> kernel/seccomp.c | 14 +++++++++-----
> 1 file changed, 9 insertions(+), 5 deletions(-)
>
> diff --git a/kernel/seccomp.c b/kernel/seccomp.c
> index 98b59b5..897f153 100644
> --- a/kernel/seccomp.c
> +++ b/kernel/seccomp.c
> @@ -476,10 +476,8 @@ static inline void seccomp_filter_free(struct seccomp_filter *filter)
> }
> }
>
> -/* put_seccomp_filter - decrements the ref count of tsk->seccomp.filter */
> -void put_seccomp_filter(struct task_struct *tsk)
> +static void __put_seccomp_filter(struct seccomp_filter *orig)
> {
> - struct seccomp_filter *orig = tsk->seccomp.filter;
> /* Clean up single-reference branches iteratively. */
> while (orig && refcount_dec_and_test(&orig->usage)) {
> struct seccomp_filter *freeme = orig;
> @@ -488,6 +486,12 @@ void put_seccomp_filter(struct task_struct *tsk)
> }
> }
>
> +/* put_seccomp_filter - decrements the ref count of tsk->seccomp.filter */
> +void put_seccomp_filter(struct task_struct *tsk)
> +{
> + __put_seccomp_filter(tsk->seccomp.filter);
> +}
> +
> static void seccomp_init_siginfo(siginfo_t *info, int syscall, int reason)
> {
> memset(info, 0, sizeof(*info));
> @@ -908,13 +912,13 @@ long seccomp_get_filter(struct task_struct *task, unsigned long filter_off,
> if (!data)
> goto out;
>
> - get_seccomp_filter(task);
> + refcount_inc(&filter->usage);
> spin_unlock_irq(&task->sighand->siglock);
>
> if (copy_to_user(data, fprog->filter, bpf_classic_proglen(fprog)))
> ret = -EFAULT;
>
> - put_seccomp_filter(task);
> + __put_seccomp_filter(filter);
> return ret;

Given how reference counting is done for filters, I'd be happier with
leaving the get_seccomp_filter() as-is, and providing the
__put_seccomp_filter() as the only change here (i.e. don't open-code
the refcount_inc()).

-Kees

--
Kees Cook
Pixel Security

2017-09-20 18:40:17

[permalink] [raw]

Subject: Re: [PATCH] seccomp: fix the usage of get/put_seccomp_filter() in seccomp_get_filter()

On Wed, Sep 20, 2017 at 6:04 AM, Oleg Nesterov <[email protected]> wrote:
> On 09/20, Oleg Nesterov wrote:
>>
>> @@ -908,13 +912,13 @@ long seccomp_get_filter(struct task_struct *task, unsigned long filter_off,
>> if (!data)
>> goto out;
>>
>> - get_seccomp_filter(task);
>> + refcount_inc(&filter->usage);
>> spin_unlock_irq(&task->sighand->siglock);
>>
>> if (copy_to_user(data, fprog->filter, bpf_classic_proglen(fprog)))
>> ret = -EFAULT;
>>
>> - put_seccomp_filter(task);
>> + __put_seccomp_filter(filter);
>
> This is the simple fix for -stable, but again, can't we simplify this
> code? Afaics we can do get_seccomp_filter() at the start and drop siglock
> right after that.
>
> Something like the untested patch (on top of this one) below?

Yeah, I think this one looks good (modulo the -stable patch change).

> And I can't understand the SECCOMP_MODE_DISABLED check... shouldn't we
> simply remove it?

I like doing these sanity checks -- this isn't fast-path at all.

> --- x/kernel/seccomp.c
> +++ x/kernel/seccomp.c
> @@ -858,45 +858,36 @@ long prctl_set_seccomp(unsigned long seccomp_mode, char __user *filter)
> long seccomp_get_filter(struct task_struct *task, unsigned long filter_off,
> void __user *data)
> {
> - struct seccomp_filter *filter;
> + struct seccomp_filter *orig, *filter;
> struct sock_fprog_kern *fprog;
> + unsigned long count;
> long ret;
> - unsigned long count = 0;
>
> if (!capable(CAP_SYS_ADMIN) ||
> current->seccomp.mode != SECCOMP_MODE_DISABLED) {
> return -EACCES;
> }
>
> + if (task->seccomp.mode != SECCOMP_MODE_FILTER)
> + return -EINVAL;
> +
> spin_lock_irq(&task->sighand->siglock);
> - if (task->seccomp.mode != SECCOMP_MODE_FILTER) {
> - ret = -EINVAL;
> - goto out;
> - }
> + get_seccomp_filter(task);
> + orig = task->seccomp.filter;
> + spin_unlock_irq(&task->sighand->siglock);
>
> - filter = task->seccomp.filter;
> - while (filter) {
> - filter = filter->prev;
> + count = 0;
> + for (filter = orig; filter; filter = filter->prev)
> count++;
> - }
>
> if (filter_off >= count) {
> ret = -ENOENT;
> goto out;
> }
> - count -= filter_off;
>
> - filter = task->seccomp.filter;
> - while (filter && count > 1) {
> - filter = filter->prev;
> + count -= filter_off;
> + for (filter = orig; count > 1; filter = filter->prev)
> count--;
> - }
> -
> - if (WARN_ON(count != 1 || !filter)) {
> - /* The filter tree shouldn't shrink while we're using it. */
> - ret = -ENOENT;
> - goto out;
> - }

Similarly, there's no reason to remove this check either.

> fprog = filter->prog->orig_prog;
> if (!fprog) {
> @@ -912,17 +903,11 @@ long seccomp_get_filter(struct task_struct *task, unsigned long filter_off,
> if (!data)
> goto out;
>
> - refcount_inc(&filter->usage);
> - spin_unlock_irq(&task->sighand->siglock);
> -
> if (copy_to_user(data, fprog->filter, bpf_classic_proglen(fprog)))
> ret = -EFAULT;
>
> - __put_seccomp_filter(filter);
> - return ret;
> -
> out:
> - spin_unlock_irq(&task->sighand->siglock);
> + __put_seccomp_filter(orig);
> return ret;
> }
> #endif
>

-Kees

--
Kees Cook
Pixel Security

2017-09-21 10:57:17

[permalink] [raw]

Subject: Re: [PATCH] seccomp: fix the usage of get/put_seccomp_filter() in seccomp_get_filter()

On 09/20, Kees Cook wrote:
>
> On Wed, Sep 20, 2017 at 5:56 AM, Oleg Nesterov <[email protected]> wrote:
> > @@ -908,13 +912,13 @@ long seccomp_get_filter(struct task_struct *task, unsigned long filter_off,
> > if (!data)
> > goto out;
> >
> > - get_seccomp_filter(task);
> > + refcount_inc(&filter->usage);
> > spin_unlock_irq(&task->sighand->siglock);
> >
> > if (copy_to_user(data, fprog->filter, bpf_classic_proglen(fprog)))
> > ret = -EFAULT;
> >
> > - put_seccomp_filter(task);
> > + __put_seccomp_filter(filter);
> > return ret;
>
> Given how reference counting is done for filters, I'd be happier with
> leaving the get_seccomp_filter() as-is,

No, please note that filter != tsk->seccomp.filter, get_seccomp_filter()
won't work.

> (i.e. don't open-code
> the refcount_inc()).

agreed, probably another __get_seccomp_filter(filter) makes sense, especially
if we do other changes like get_nth().

But imo not in this fix.

Oleg.

2017-09-21 11:31:54

[permalink] [raw]

Subject: Re: [PATCH] seccomp: fix the usage of get/put_seccomp_filter() in seccomp_get_filter()

On 09/20, Kees Cook wrote:
>
> I like doing these sanity checks -- this isn't fast-path at all.

Yes, but see another "introduce get_nth_filter()" cleanup I sent, it is
similar but more suitable for Tycho's "retrieving seccomp flags" patch.

> > + for (filter = orig; count > 1; filter = filter->prev)
^^^^^^^^^
I just noticed that I forgot to replace this check with "count != 1".
Correctness wise this doesn't matter, but looks more clean.

> > count--;
> > - }
> > -
> > - if (WARN_ON(count != 1 || !filter)) {
> > - /* The filter tree shouldn't shrink while we're using it. */
> > - ret = -ENOENT;
> > - goto out;
> > - }
>
> Similarly, there's no reason to remove this check either.

Well, I disagree, but this is subjective so I won't insist.

Why do we want this WARN_ON() ? The sanity check can only fail if we have
a bug in 10 lines above. Lets look at the code after this cleanup,

count = 0;
for (filter = orig; filter; filter = filter->prev)
count++;

if (filter_off >= count)
goto out;

count -= filter_off;
for (filter = orig; count != 1; filter = filter->prev)
count--;

Do we want to check "count == 1" after the 2nd loop? I don't think so.
filter != NULL ? IMO makes no sense. Again, it can only be NULL if the
quoted code above is wrong, and in this case the next line

refcount_inc(&filter->usage);

will crash.

Oleg.

2017-09-21 19:51:42

[permalink] [raw]

Subject: Re: [PATCH] seccomp: fix the usage of get/put_seccomp_filter() in seccomp_get_filter()

On Thu, Sep 21, 2017 at 3:57 AM, Oleg Nesterov <[email protected]> wrote:
> On 09/20, Kees Cook wrote:
>>
>> On Wed, Sep 20, 2017 at 5:56 AM, Oleg Nesterov <[email protected]> wrote:
>> > @@ -908,13 +912,13 @@ long seccomp_get_filter(struct task_struct *task, unsigned long filter_off,
>> > if (!data)
>> > goto out;
>> >
>> > - get_seccomp_filter(task);
>> > + refcount_inc(&filter->usage);
>> > spin_unlock_irq(&task->sighand->siglock);
>> >
>> > if (copy_to_user(data, fprog->filter, bpf_classic_proglen(fprog)))
>> > ret = -EFAULT;
>> >
>> > - put_seccomp_filter(task);
>> > + __put_seccomp_filter(filter);
>> > return ret;
>>
>> Given how reference counting is done for filters, I'd be happier with
>> leaving the get_seccomp_filter() as-is,
>
> No, please note that filter != tsk->seccomp.filter, get_seccomp_filter()
> won't work.

Ah yes, sorry, you're right.

>> (i.e. don't open-code
>> the refcount_inc()).
>
> agreed, probably another __get_seccomp_filter(filter) makes sense, especially
> if we do other changes like get_nth().
>
> But imo not in this fix.

Regardless, whatever lands will need backport adjustment for
refcount_*/atomic_* in -stable.

Can you resend the two patches; I can send the backport to -stable manually...

-Kees

--
Kees Cook
Pixel Security

2017-09-22 15:22:33

[permalink] [raw]

Subject: Re: [PATCH] seccomp: fix the usage of get/put_seccomp_filter() in seccomp_get_filter()

On 09/21, Kees Cook wrote:
>
> On Thu, Sep 21, 2017 at 3:57 AM, Oleg Nesterov <[email protected]> wrote:
> > On 09/20, Kees Cook wrote:
> >>
> >> Given how reference counting is done for filters, I'd be happier with
> >> leaving the get_seccomp_filter() as-is,
> >
> > No, please note that filter != tsk->seccomp.filter, get_seccomp_filter()
> > won't work.
>
> Ah yes, sorry, you're right.
>
> >> (i.e. don't open-code
> >> the refcount_inc()).
> >
> > agreed, probably another __get_seccomp_filter(filter) makes sense, especially
> > if we do other changes like get_nth().
> >
> > But imo not in this fix.
>
> Regardless, whatever lands will need backport adjustment for
> refcount_*/atomic_* in -stable.

yes, but this adjustment is trivial, and we will need it whatever we do
in this fix,

> Can you resend the two patches; I can send the backport to -stable manually...

Not sure I understand... Do you mean this fix + untested "introduce get_nth_filter()" ?

Can't we push this simple fix first? Then we can discuss the cleanups. Besides,
the 2nd patch connects to Tycho's "[PATCH] ptrace, seccomp: add support for
retrieving seccomp flags", otherwise it could be more simple.

Oleg.

2017-09-22 15:25:47

[permalink] [raw]

Subject: Re: [PATCH] seccomp: fix the usage of get/put_seccomp_filter() in seccomp_get_filter()

On Fri, Sep 22, 2017 at 05:22:29PM +0200, Oleg Nesterov wrote:
> On 09/21, Kees Cook wrote:
> >
> > On Thu, Sep 21, 2017 at 3:57 AM, Oleg Nesterov <[email protected]> wrote:
> > > On 09/20, Kees Cook wrote:
> > >>
> > >> Given how reference counting is done for filters, I'd be happier with
> > >> leaving the get_seccomp_filter() as-is,
> > >
> > > No, please note that filter != tsk->seccomp.filter, get_seccomp_filter()
> > > won't work.
> >
> > Ah yes, sorry, you're right.
> >
> > >> (i.e. don't open-code
> > >> the refcount_inc()).
> > >
> > > agreed, probably another __get_seccomp_filter(filter) makes sense, especially
> > > if we do other changes like get_nth().
> > >
> > > But imo not in this fix.
> >
> > Regardless, whatever lands will need backport adjustment for
> > refcount_*/atomic_* in -stable.
>
> yes, but this adjustment is trivial, and we will need it whatever we do
> in this fix,
>
> > Can you resend the two patches; I can send the backport to -stable manually...
>
> Not sure I understand... Do you mean this fix + untested "introduce get_nth_filter()" ?
>
> Can't we push this simple fix first? Then we can discuss the cleanups. Besides,
> the 2nd patch connects to Tycho's "[PATCH] ptrace, seccomp: add support for
> retrieving seccomp flags", otherwise it could be more simple.

Yes, I'll happily fold your fix into the next version of my patch. As
it stands now I'm just waiting on input about unrelated API feedback.

Cheers,

Tycho

2017-09-26 20:15:40

[permalink] [raw]

Subject: Re: [PATCH] seccomp: fix the usage of get/put_seccomp_filter() in seccomp_get_filter()

Hi,

On Fri, Sep 22, 2017 at 05:22:29PM +0200, Oleg Nesterov wrote:
> On 09/21, Kees Cook wrote:
> > Can you resend the two patches; I can send the backport to -stable manually...
>
> Not sure I understand... Do you mean this fix + untested "introduce get_nth_filter()" ?

Just want to make sure this doesn't get lost in the shuffle. If I
resend just Oleg's patch with the added __get_secomp_filter() instead
of open coded refcount, will that work for you Kees?

We can worry about the get_nth_filter implementation with the
PTRACE_SECCOMP_GET_METADATA series later.

Cheers,

Tycho

2017-09-27 06:07:13