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

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

[Zhihao Cheng]

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

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).

Fixes: 0fd338b2d2cdf8 ("exec: move path_noexec() check earlier")
Signed-off-by: Zhihao Cheng <chengzhihao1@huawei.com>
---
 fs/exec.c | 22 +---------------------
 1 file changed, 1 insertion(+), 21 deletions(-)

diff --git a/fs/exec.c b/fs/exec.c
index e3e55d5e0be1..0f8ea7e9e03c 100644
--- 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;
-
 	fsnotify_open(file);
 
 	error = -ENOEXEC;
@@ -169,7 +159,7 @@ SYSCALL_DEFINE1(uselib, const char __user *, library)
 			break;
 	}
 	read_unlock(&binfmt_lock);
-exit:
+
 	fput(file);
 out:
   	return error;
@@ -919,16 +909,6 @@ static struct file *do_open_execat(int fd, struct filename *name, int flags)
 	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.
-	 */
-	err = -EACCES;
-	if (WARN_ON_ONCE(!S_ISREG(file_inode(file)->i_mode) ||
-			 path_noexec(&file->f_path)))
-		goto exit;
-
 	err = deny_write_access(file);
 	if (err)
 		goto exit;
-- 
2.31.1

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

[Andrew Morton]

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.

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

[Kees Cook]

On Wed, May 18, 2022 at 10:46:01AM -0700, Andrew Morton wrote:
> 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

Did you encounter this in the real world?

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

It seems not an unreasonable thing to warn about. Perhaps since it's
technically reachable from userspace, it could be downgraded to
pr_warn(), but I certainly don't want to remove the checks.

> 
> > 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.

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.

-- 
Kees Cook

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

[Andrew Morton]

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?

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

[Zhihao Cheng]

在 2022/5/19 3:17, Kees Cook 写道:

>>> WARNON(path_noexec(&file->f_path)) // path_noexec() checks fail
> 
> Did you encounter this in the real world?
I found the problem by running fuzz test.(syzkaller)

Here is a brief reproducer.
1. Apply diff
2. Complie and run repo.c
diff
diff --git a/fs/exec.c b/fs/exec.c
index e3e55d5e0be1..388d38b87e9a 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -897,6 +897,7 @@ EXPORT_SYMBOL(transfer_args_to_stack);

  #endif /* CONFIG_MMU */

+#include <linux/delay.h>
  static struct file *do_open_execat(int fd, struct filename *name, int 
flags)
  {
  	struct file *file;
@@ -925,9 +926,15 @@ static struct file *do_open_execat(int fd, struct 
filename *name, int flags)
  	 * and check again at the very end too.
  	 */
  	err = -EACCES;
+	if (!strcmp(file->f_path.dentry->d_iname, "my_bin")) {
+		pr_err("wait ...\n");
+		msleep(3000);
+	}
  	if (WARN_ON_ONCE(!S_ISREG(file_inode(file)->i_mode) ||
-			 path_noexec(&file->f_path)))
+			 path_noexec(&file->f_path))) {
+		pr_err("exec %pd %d %d %s\n", file->f_path.dentry, 
file->f_path.mnt->mnt_flags & MNT_NOEXEC, 
file->f_path.mnt->mnt_sb->s_iflags & SB_I_NOEXEC, 
file->f_path.mnt->mnt_sb->s_type->name);
  		goto exit;
+	}

  	err = deny_write_access(file);
  	if (err)

repo.c
int main(void)
{
	int ret;

	system("umount temp 2>&1 > /dev/null");
	system("mount -t tmpfs none temp");
	system("echo 12312 > temp/my_bin && chmod +x temp/my_bin");
	ret = fork();
	if (ret < 0) {
		perror("fork fail");
		return 0;
	}
	if (ret == 0) {
		system("mount -oremount,noexec temp");
		exit(0);
	} else {
		execve("/root/temp/my_bin", NULL, 0);
		//syscall(__NR_uselib, "/root/temp/my_bin");
	}
	return 0;
}
> 
>>
>> You're saying this is a race condition?  A concurrent remount causes
>> this warning?
> 
> It seems not an unreasonable thing to warn about. Perhaps since it's
> technically reachable from userspace, it could be downgraded to
> pr_warn(), but I certainly don't want to remove the checks.


> 
> 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.
>I think removing redundant checking is okay,

do_open_execat/uselib has initialized the acc_mode and open_flag for 
exec file, the check is equivalent to check in may_open().

Remount(noexec) operations can alos happen after the latest check, 
double check has no means for the concurrent situation.

The MNT_NOEXEC flag only affects the open operation, it won't cause any 
problems that an opened bin file is executing in a non-exec mounted 
filesystem.