From mboxrd@z Thu Jan  1 00:00:00 1970
Return-Path: <linux-kernel-owner@vger.kernel.org>
Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand
        id S1751661AbdIULby (ORCPT <rfc822;w@1wt.eu>);
        Thu, 21 Sep 2017 07:31:54 -0400
Received: from mx1.redhat.com ([209.132.183.28]:44496 "EHLO mx1.redhat.com"
        rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP
        id S1751436AbdIULbx (ORCPT <rfc822;linux-kernel@vger.kernel.org>);
        Thu, 21 Sep 2017 07:31:53 -0400
DMARC-Filter: OpenDMARC Filter v1.3.2 mx1.redhat.com 06D605F7A6
Authentication-Results: ext-mx10.extmail.prod.ext.phx2.redhat.com; dmarc=none (p=none dis=none) header.from=redhat.com
Authentication-Results: ext-mx10.extmail.prod.ext.phx2.redhat.com; spf=fail smtp.mailfrom=oleg@redhat.com
Date: Thu, 21 Sep 2017 13:31:50 +0200
From: Oleg Nesterov <oleg@redhat.com>
To: Kees Cook <keescook@chromium.org>
Cc: Chris Salls <chrissalls5@gmail.com>,
        Andy Lutomirski <luto@amacapital.net>, Will Drewry <wad@chromium.org>,
        "security@kernel.org" <security@kernel.org>,
        LKML <linux-kernel@vger.kernel.org>, Tycho Andersen <tycho@docker.com>
Subject: Re: [PATCH] seccomp: fix the usage of get/put_seccomp_filter() in
 seccomp_get_filter()
Message-ID: <20170921113150.GA1416@redhat.com>
References: <CAN-hQdds6zkYaGRJTrS5KOorvopoYnP4vBEfoKntS_8y4884Aw@mail.gmail.com>
 <20170920125621.GA3599@redhat.com>
 <20170920130443.GA4445@redhat.com>
 <CAGXu5jKb8YnyQtKe7HJY6XiC+GC2c5r3R9K=mTC7rON+OXnrSw@mail.gmail.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <CAGXu5jKb8YnyQtKe7HJY6XiC+GC2c5r3R9K=mTC7rON+OXnrSw@mail.gmail.com>
User-Agent: Mutt/1.5.24 (2015-08-30)
X-Greylist: Sender IP whitelisted, not delayed by milter-greylist-4.5.16 (mx1.redhat.com [10.5.110.39]); Thu, 21 Sep 2017 11:31:53 +0000 (UTC)
Sender: linux-kernel-owner@vger.kernel.org
List-ID: <linux-kernel.vger.kernel.org>
X-Mailing-List: linux-kernel@vger.kernel.org

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.