Re: [PATCH -next] exec: Remove redundant check in do_open_execat/uselib

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
Date: Wed, 18 May 2022 12:27:09 -0700	[thread overview]
Message-ID: <20220518122709.7fb5176967fb69324c260853@linux-foundation.org> (raw)
In-Reply-To: <202205181215.D448675BEA@keescook>

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?