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

[Kees Cook]

On Wed, Sep 20, 2017 at 6:04 AM, Oleg Nesterov <oleg@xxxxxxxxxx> 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