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

Kees Cook
2017-09-20T18:40:17Z

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