linux-kernel - Re: [PATCH v2 8/8] exec: Remove recursion from search_binary

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <202005191320.230EFDFCB@keescook>
Date:   Tue, 19 May 2020 13:37:57 -0700
From:   Kees Cook <keescook@...omium.org>
To:     "Eric W. Biederman" <ebiederm@...ssion.com>
Cc:     linux-kernel@...r.kernel.org,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Oleg Nesterov <oleg@...hat.com>, Jann Horn <jannh@...gle.com>,
        Greg Ungerer <gerg@...ux-m68k.org>,
        Rob Landley <rob@...dley.net>,
        Bernd Edlinger <bernd.edlinger@...mail.de>,
        linux-fsdevel@...r.kernel.org, Al Viro <viro@...IV.linux.org.uk>,
        Alexey Dobriyan <adobriyan@...il.com>,
        Andrew Morton <akpm@...ux-foundation.org>,
        Casey Schaufler <casey@...aufler-ca.com>,
        linux-security-module@...r.kernel.org,
        James Morris <jmorris@...ei.org>,
        "Serge E. Hallyn" <serge@...lyn.com>,
        Andy Lutomirski <luto@...capital.net>
Subject: Re: [PATCH v2 8/8] exec: Remove recursion from search_binary_handler

On Mon, May 18, 2020 at 07:34:19PM -0500, Eric W. Biederman wrote:
> 
> Recursion in kernel code is generally a bad idea as it can overflow
> the kernel stack.  Recursion in exec also hides that the code is
> looping and that the loop changes bprm->file.
> 
> Instead of recursing in search_binary_handler have the methods that
> would recurse set bprm->interpreter and return 0.  Modify exec_binprm
> to loop when bprm->interpreter is set.  Consolidate all of the
> reassignments of bprm->file in that loop to make it clear what is
> going on.
> 
> The structure of the new loop in exec_binprm is that all errors return
> immediately, while successful completion (ret == 0 &&
> !bprm->interpreter) just breaks out of the loop and runs what
> exec_bprm has always run upon successful completion.
> 
> Fail if the an interpreter is being call after execfd has been set.
> The code has never properly handled an interpreter being called with
> execfd being set and with reassignments of bprm->file and the
> assignment of bprm->executable in generic code it has finally become
> possible to test and fail when if this problematic condition happens.
> 
> With the reassignments of bprm->file and the assignment of
> bprm->executable moved into the generic code add a test to see if
> bprm->executable is being reassigned.
> 
> In search_binary_handler remove the test for !bprm->file.  With all
> reassignments of bprm->file moved to exec_binprm bprm->file can never
> be NULL in search_binary_handler.
> 
> Signed-off-by: "Eric W. Biederman" <ebiederm@...ssion.com>

Lovely!

Reviewed-by: Kees Cook <keescook@...omium.org>

I spent some time following the file lifetimes of deny/allow_write_access()
and the fget/fput() paths. It all looks correct to me; it's tricky
(especially bprm->executable) but so very much cleaner than before. :)

The only suggestion I could come up with is more comments (surprise) to
help anyone new to this loop realize what the "common" path is (and
similarly, a compiler hint too):

diff --git a/fs/exec.c b/fs/exec.c
index a9f421ec9e27..738051a698e1 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1790,15 +1790,19 @@ static int exec_binprm(struct linux_binprm *bprm)
 	/* This allows 4 levels of binfmt rewrites before failing hard. */
 	for (depth = 0;; depth++) {
 		struct file *exec;
+
 		if (depth > 5)
 			return -ELOOP;
 
 		ret = search_binary_handler(bprm);
+		/* Unrecoverable error, give up. */
 		if (ret < 0)
 			return ret;
-		if (!bprm->interpreter)
+		/* Found final handler, start execution. */
+		if (likely(!bprm->interpreter))
 			break;
 
+		/* Found an interpreter, so try again and attempt to run it. */
 		exec = bprm->file;
 		bprm->file = bprm->interpreter;
 		bprm->interpreter = NULL;

-- 
Kees Cook