Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751648AbdITSkR (ORCPT ); Wed, 20 Sep 2017 14:40:17 -0400 Received: from mail-it0-f44.google.com ([209.85.214.44]:53094 "EHLO mail-it0-f44.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751367AbdITSkQ (ORCPT ); Wed, 20 Sep 2017 14:40:16 -0400 X-Google-Smtp-Source: AOwi7QBdvRBM61laI6N+PZbzgTQDp5m6KckLGKTKtf40anGY/TIOvZrChuUFOaGKs2uRemOt8i6CVN+RV2J4rCKnJQ4= MIME-Version: 1.0 In-Reply-To: <20170920130443.GA4445@redhat.com> References: <20170920125621.GA3599@redhat.com> <20170920130443.GA4445@redhat.com> From: Kees Cook Date: Wed, 20 Sep 2017 11:40:14 -0700 X-Google-Sender-Auth: 84KvpEeACkZ8JUFKVVKQJdcGNY4 Message-ID: Subject: Re: [PATCH] seccomp: fix the usage of get/put_seccomp_filter() in seccomp_get_filter() To: Oleg Nesterov Cc: Chris Salls , Andy Lutomirski , Will Drewry , "security@kernel.org" , LKML , Tycho Andersen Content-Type: text/plain; charset="UTF-8" Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org Content-Length: 3652 Lines: 118 On Wed, Sep 20, 2017 at 6:04 AM, 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? 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