Received-SPF: softfail (google.com: domain of transitioning linux-kernel-owner@vger.kernel.org does not designate 23.128.96.19 as permitted sender) client-ip=23.128.96.19;
Date:   Wed, 18 May 2022 12:27:09 -0700
From:   Andrew Morton <akpm@linux-foundation.org>
To:     Kees Cook <keescook@chromium.org>
Cc:     Zhihao Cheng <chengzhihao1@huawei.com>, viro@zeniv.linux.org.uk,
        linux-mm@kvack.org, linux-fsdevel@vger.kernel.org,
        linux-kernel@vger.kernel.org, yukuai3@huawei.com
Subject: Re: [PATCH -next] exec: Remove redundant check in
 do_open_execat/uselib
Message-Id: <20220518122709.7fb5176967fb69324c260853@linux-foundation.org>
In-Reply-To: <202205181215.D448675BEA@keescook>
References: <20220518081227.1278192-1-chengzhihao1@huawei.com>
        <20220518104601.fc21907008231b60a0e54a8e@linux-foundation.org>
        <202205181215.D448675BEA@keescook>
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Wed, 18 May 2022 12:17:45 -0700 Kees Cook <keescook@chromium.org> wrote:

> > > -	/*
> > > -	 * may_open() has already checked for this, so it should be
> > > -	 * impossible to trip now. But we need to be extra cautious
> > > -	 * and check again at the very end too.
> > > -	 */
> > > -	error = -EACCES;
> > > -	if (WARN_ON_ONCE(!S_ISREG(file_inode(file)->i_mode) ||
> > > -			 path_noexec(&file->f_path)))
> > > -		goto exit;
> > > -
> > 
> > Maybe we should retain the `goto exit'.  The remount has now occurred,
> > so the execution attempt should be denied.  If so, the comment should
> > be updated to better explain what's happening.
> > 
> > I guess we'd still be racy against `mount -o exec', but accidentally
> > denying something seems less serious than accidentally permitting it.
> 
> I'd like to leave this as-is, since we _do_ want to find the cases where
> we're about to allow an exec and a very important security check was NOT
> handled.

In which case we don't want the "_ONCE".  If some app is hammering away
at this trying to hit a race window then the operator wants that log
flood.

Or,umm, fix the dang race?