Re: [PATCH v3 1/2] seccomp: hoist out filter resolving logic

[Kees Cook]

On Wed, Oct 11, 2017 at 8:39 AM, Tycho Andersen <tycho@xxxxxxxxxx> wrote:
> Hoist out the nth filter resolving logic that ptrace uses into a new
> function. We'll use this in the next patch to implement the new
> PTRACE_SECCOMP_GET_FILTER_FLAGS command.
>
> v3: * significantly revamp get_nth_filter logic (Oleg)
>     * rebase on 4.14-rc4, using the new __{get,put}_seccomp_filter
>       primitives
>
> Signed-off-by: Tycho Andersen <tycho@xxxxxxxxxx>
> CC: Kees Cook <keescook@xxxxxxxxxxxx>
> CC: Andy Lutomirski <luto@xxxxxxxxxxxxxx>
> CC: Oleg Nesterov <oleg@xxxxxxxxxx>

Sorry for the giant delay in reviewing these. This all looks good,
thanks! Applying for -next.

-Kees

> ---
>  kernel/seccomp.c | 77 +++++++++++++++++++++++++++++++++-----------------------
>  1 file changed, 45 insertions(+), 32 deletions(-)
>
> diff --git a/kernel/seccomp.c b/kernel/seccomp.c
> index bb3a38005b9c..2e1568261ac4 100644
> --- a/kernel/seccomp.c
> +++ b/kernel/seccomp.c
> @@ -977,49 +977,68 @@ 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;
> -
> -       if (!capable(CAP_SYS_ADMIN) ||
> -           current->seccomp.mode != SECCOMP_MODE_DISABLED) {
> -               return -EACCES;
> -       }
> +       struct seccomp_filter *orig, *filter;
> +       unsigned long count;
>
> +       /*
> +        * Note: this is only correct because the caller should be the (ptrace)
> +        * tracer of the task, otherwise lock_task_sighand is needed.
> +        */
>         spin_lock_irq(&task->sighand->siglock);
> +
>         if (task->seccomp.mode != SECCOMP_MODE_FILTER) {
> -               ret = -EINVAL;
> -               goto out;
> +               spin_unlock_irq(&task->sighand->siglock);
> +               return ERR_PTR(-EINVAL);
>         }
>
> -       filter = task->seccomp.filter;
> -       while (filter) {
> -               filter = filter->prev;
> +       orig = task->seccomp.filter;
> +       __get_seccomp_filter(orig);
> +       spin_unlock_irq(&task->sighand->siglock);
> +
> +       count = 0;
> +       for (filter = orig; filter; filter = filter->prev)
>                 count++;
> -       }
>
>         if (filter_off >= count) {
> -               ret = -ENOENT;
> +               filter = ERR_PTR(-ENOENT);
>                 goto out;
>         }
> -       count -= filter_off;
>
> -       filter = task->seccomp.filter;
> -       while (filter && count > 1) {
> -               filter = filter->prev;
> +       count -= filter_off;
> +       for (filter = orig; filter && 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;
> +               filter = ERR_PTR(-ENOENT);
>                 goto out;
>         }
>
> +       __get_seccomp_filter(filter);
> +
> +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
> @@ -1034,17 +1053,11 @@ long seccomp_get_filter(struct task_struct *task, unsigned long filter_off,
>         if (!data)
>                 goto out;
>
> -       __get_seccomp_filter(filter);
> -       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
> --
> 2.11.0
>



-- 
Kees Cook
Pixel Security