linux-kernel - Re: [PATCH -next] exec: Remove redundant check in do_open

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-Id: <20220518122709.7fb5176967fb69324c260853@linux-foundation.org>
Date:   Wed, 18 May 2022 12:27:09 -0700
From:   Andrew Morton <akpm@...ux-foundation.org>
To:     Kees Cook <keescook@...omium.org>
Cc:     Zhihao Cheng <chengzhihao1@...wei.com>, viro@...iv.linux.org.uk,
        linux-mm@...ck.org, linux-fsdevel@...r.kernel.org,
        linux-kernel@...r.kernel.org, yukuai3@...wei.com
Subject: Re: [PATCH -next] exec: Remove redundant check in
 do_open_execat/uselib

On Wed, 18 May 2022 12:17:45 -0700 Kees Cook <keescook@...omium.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?