Re: [PATCH v2 8/8] exec: Remove recursion from search_binary_handler

From: Kees Cook <keescook@chromium.org>
To: "Eric W. Biederman" <ebiederm@xmission.com>
Cc: linux-kernel@vger.kernel.org,
	Linus Torvalds <torvalds@linux-foundation.org>,
	Oleg Nesterov <oleg@redhat.com>, Jann Horn <jannh@google.com>,
	Greg Ungerer <gerg@linux-m68k.org>, Rob Landley <rob@landley.net>,
	Bernd Edlinger <bernd.edlinger@hotmail.de>,
	linux-fsdevel@vger.kernel.org, Al Viro <viro@ZenIV.linux.org.uk>,
	Alexey Dobriyan <adobriyan@gmail.com>,
	Andrew Morton <akpm@linux-foundation.org>,
	Casey Schaufler <casey@schaufler-ca.com>,
	linux-security-module@vger.kernel.org,
	James Morris <jmorris@namei.org>,
	"Serge E. Hallyn" <serge@hallyn.com>,
	Andy Lutomirski <luto@amacapital.net>
Subject: Re: [PATCH v2 8/8] exec: Remove recursion from search_binary_handler
Date: Tue, 19 May 2020 13:37:57 -0700	[thread overview]
Message-ID: <202005191320.230EFDFCB@keescook> (raw)
In-Reply-To: <87sgfwyd84.fsf_-_@x220.int.ebiederm.org>

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@xmission.com>

Lovely!

Reviewed-by: Kees Cook <keescook@chromium.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
