fs/exec.c | 22 +--------------------- 1 file changed, 1 insertion(+), 21 deletions(-)
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.1On 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.
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
在 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.
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?
© 2016 - 2026 Red Hat, Inc.