lkml.org 
[lkml]   [2022]   [May]   [18]   [last100]   RSS Feed
Views: [wrap][no wrap]   [headers]  [forward] 
 
Messages in this thread
/
Date
From
SubjectRe: [PATCH -next] exec: Remove redundant check in do_open_execat/uselib
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.


\
 
 \ /
  Last update: 2022-05-18 19:48    [W:0.434 / U:0.024 seconds]
©2003-2020 Jasper Spaans|hosted at Digital Ocean and TransIP|Read the blog|Advertise on this site