| Message ID | 9b846b1f-a231-4f09-8c37-6bfb0d1e7b05@i-love.sakura.ne.jp (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [(repost)] umh: fix refcount underflow in fork_usermode_blob(). | expand |
On Fri, 27 Mar 2020 09:51:34 +0900 Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote: > Since free_bprm(bprm) always calls allow_write_access(bprm->file) and > fput(bprm->file) if bprm->file is set to non-NULL, __do_execve_file() > must call deny_write_access(file) and get_file(file) if called from > do_execve_file() path. Otherwise, use-after-free access can happen at > fput(file) in fork_usermode_blob(). > > general protection fault, probably for non-canonical address 0x6b6b6b6b6b6b6b6b: 0000 [#1] SMP DEBUG_PAGEALLOC > CPU: 3 PID: 4131 Comm: insmod Tainted: G O 5.6.0-rc5+ #978 > Hardware name: VMware, Inc. VMware Virtual Platform/440BX Desktop Reference Platform, BIOS 6.00 07/29/2019 > RIP: 0010:fork_usermode_blob+0xaa/0x190 This is rather old code - what casued this to be observed now? Some unusual userspace behaviour?
On Fri, Mar 27, 2020 at 09:51:34AM +0900, Tetsuo Handa wrote: > diff --git a/fs/exec.c b/fs/exec.c > index db17be51b112..ded3fa368dc7 100644 > --- a/fs/exec.c > +++ b/fs/exec.c > @@ -1761,11 +1761,17 @@ static int __do_execve_file(int fd, struct filename *filename, > check_unsafe_exec(bprm); > current->in_execve = 1; > > - if (!file) > + if (!file) { > file = do_open_execat(fd, filename, flags); > - retval = PTR_ERR(file); > - if (IS_ERR(file)) > - goto out_unmark; > + retval = PTR_ERR(file); > + if (IS_ERR(file)) > + goto out_unmark; > + } else { > + retval = deny_write_access(file); > + if (retval) > + goto out_unmark; > + get_file(file); > + } I still don't like it. The bug is real, but... *yeccchhhh* First of all, this assignment to "file" is misguiding - assignment to bprm->file would've been a lot easier to follow. Furthermore, the damn thing already has much too confusing cleanup logics. Why is out: if (bprm->mm) { acct_arg_size(bprm, 0); mmput(bprm->mm); } done on failure exit in this function and not in free_bprm(), while dropping bprm->file is in free_bprm()? Note that flush_old_exec() will zero bprm->mm (after it transfers the damn thing into current->mm), so we are fine here. And getting rid of that thing in __do_execve_file() simplifies the logics in there, especially if you take everything from this if (!file) up to retval = exec_binprm(bprm); into a new function. All those goto out_unmark/goto out turn into plain returns. And in __do_execve_file() we are left with .... current->in_execve = 1; retval = this_new_helper(whatever it needs passed to it); current->fs->in_exec = 0; current->in_execve = 0; if (!retval) { /* execve succeeded */ rseq_execve(current); acct_update_integrals(current); task_numa_free(current, false); } out_free: free_bprm(bprm); kfree(pathbuf); out_files: if (displaced) put_files_struct(displaced); out_ret: if (filename) putname(filename); return retval; which is a lot easier to follow. Especially if we lift the logics for freeing pathbuf into free_bprm() as well (will need a flag there, for "does ->filename need to be freed?"). And I really wonder if sched_exec() is called in the right place - what's special about the point after opening the binary to be and setting bprm->file to what we got?
On 2020/03/29 9:55, Andrew Morton wrote: > On Fri, 27 Mar 2020 09:51:34 +0900 Tetsuo Handa <penguin-kernel@i-love.sakura.ne.jp> wrote: > >> Since free_bprm(bprm) always calls allow_write_access(bprm->file) and >> fput(bprm->file) if bprm->file is set to non-NULL, __do_execve_file() >> must call deny_write_access(file) and get_file(file) if called from >> do_execve_file() path. Otherwise, use-after-free access can happen at >> fput(file) in fork_usermode_blob(). >> >> general protection fault, probably for non-canonical address 0x6b6b6b6b6b6b6b6b: 0000 [#1] SMP DEBUG_PAGEALLOC >> CPU: 3 PID: 4131 Comm: insmod Tainted: G O 5.6.0-rc5+ #978 >> Hardware name: VMware, Inc. VMware Virtual Platform/440BX Desktop Reference Platform, BIOS 6.00 07/29/2019 >> RIP: 0010:fork_usermode_blob+0xaa/0x190 > > This is rather old code - what casued this to be observed now? Some > unusual userspace behaviour? I'm attempting to fix a regression for TOMOYO caused by commit 51f39a1f0cea1cac ("syscalls: implement execveat() system call") in 3.19 that execve() request fails if fd argument is not AT_FDCWD, for TOMOYO needs to re-calculate "requested pathname using AT_SYMLINK_NOFOLLOW" from LSM hook. That regression was practically not a big problem because 99%+ of execve() request used AT_FDCWD, for in general executing a program involves opening external libraries which have to be accessible from mount namespace where execve() was requested. But commit 449325b52b7a6208 ("umh: introduce fork_usermode_blob() helper") in 4.18 was a rather unique change that allows file-less execution of a program, which means that file-less execution request fails because TOMOYO can never calculate "requested pathname using AT_SYMLINK_NOFOLLOW". This patch itself does not fix the regression for TOMOYO. But this patch fixes a memory corruption bug which should be applied regardless of the regression for TOMOYO. Although Al does not like this patch, I'd like to keep this patch minimal so that RedHat folks can easily backport this patch to RHEL 8 (which uses 4.18).
diff --git a/fs/exec.c b/fs/exec.c index db17be51b112..ded3fa368dc7 100644 --- a/fs/exec.c +++ b/fs/exec.c @@ -1761,11 +1761,17 @@ static int __do_execve_file(int fd, struct filename *filename, check_unsafe_exec(bprm); current->in_execve = 1; - if (!file) + if (!file) { file = do_open_execat(fd, filename, flags); - retval = PTR_ERR(file); - if (IS_ERR(file)) - goto out_unmark; + retval = PTR_ERR(file); + if (IS_ERR(file)) + goto out_unmark; + } else { + retval = deny_write_access(file); + if (retval) + goto out_unmark; + get_file(file); + } sched_exec();
Since free_bprm(bprm) always calls allow_write_access(bprm->file) and fput(bprm->file) if bprm->file is set to non-NULL, __do_execve_file() must call deny_write_access(file) and get_file(file) if called from do_execve_file() path. Otherwise, use-after-free access can happen at fput(file) in fork_usermode_blob(). general protection fault, probably for non-canonical address 0x6b6b6b6b6b6b6b6b: 0000 [#1] SMP DEBUG_PAGEALLOC CPU: 3 PID: 4131 Comm: insmod Tainted: G O 5.6.0-rc5+ #978 Hardware name: VMware, Inc. VMware Virtual Platform/440BX Desktop Reference Platform, BIOS 6.00 07/29/2019 RIP: 0010:fork_usermode_blob+0xaa/0x190 Signed-off-by: Tetsuo Handa <penguin-kernel@I-love.SAKURA.ne.jp> Fixes: 449325b52b7a6208 ("umh: introduce fork_usermode_blob() helper") Cc: <stable@vger.kernel.org> [4.18+] Cc: Alexei Starovoitov <ast@kernel.org> Cc: David S. Miller <davem@davemloft.net> Cc: Alexander Viro <viro@zeniv.linux.org.uk> --- fs/exec.c | 14 ++++++++++---- 1 file changed, 10 insertions(+), 4 deletions(-)