Messages in this thread Patch in this message | | | Subject | Re: [PATCH -next] exec: Remove redundant check in do_open_execat/uselib | From | Zhihao Cheng <> | Date | Thu, 19 May 2022 09:51:34 +0800 |
| |
在 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.
| |