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 10:46:01 -0700
From:   Andrew Morton <akpm@linux-foundation.org>
To:     Zhihao Cheng <chengzhihao1@huawei.com>
Cc:     <keescook@chromium.org>, <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: <20220518104601.fc21907008231b60a0e54a8e@linux-foundation.org>
In-Reply-To: <20220518081227.1278192-1-chengzhihao1@huawei.com>
References: <20220518081227.1278192-1-chengzhihao1@huawei.com>
Mime-Version: 1.0
Content-Type: text/plain; charset=US-ASCII
Content-Transfer-Encoding: 7bit
Precedence: bulk

On Wed, 18 May 2022 16:12:27 +0800 Zhihao Cheng <chengzhihao1@huawei.com> wrote:

> There is a false positive WARNON happening in execve(2)/uselib(2)
> syscalls with concurrent noexec-remount.
> 
>        execveat                           remount
> do_open_execat(path/bin)
>   do_filp_open
>     path_openat
>       do_open
>         may_open
>           path_noexec() // PASS
> 	                            remount(path->mnt, MS_NOEXEC)
> WARNON(path_noexec(&file->f_path)) // path_noexec() checks fail

You're saying this is a race condition?  A concurrent remount causes
this warning?

> Since may_open() has already checked the same conditions, fix it by
> removing 'S_ISREG' and 'path_noexec' check in do_open_execat()/uselib(2).
> 
> ...
>
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -141,16 +141,6 @@ SYSCALL_DEFINE1(uselib, const char __user *, library)
>  	if (IS_ERR(file))
>  		goto out;
>  
> -	/*
> -	 * 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.