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]

Date:   Wed, 18 May 2022 10:46:01 -0700
From:   Andrew Morton <akpm@...ux-foundation.org>
To:     Zhihao Cheng <chengzhihao1@...wei.com>
Cc:     <keescook@...omium.org>, <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 16:12:27 +0800 Zhihao Cheng <chengzhihao1@...wei.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.