[PATCH 3/5] exec: Remove recursion from search_binary_handler

From: ebiederm@xmission.com (Eric W. Biederman)
To: <linux-kernel@vger.kernel.org>
Cc: Linus Torvalds <torvalds@linux-foundation.org>,
	Oleg Nesterov <oleg@redhat.com>, Jann Horn <jannh@google.com>,
	Kees Cook <keescook@chromium.org>,
	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: [PATCH 3/5] exec: Remove recursion from search_binary_handler
Date: Sat, 09 May 2020 14:41:53 -0500	[thread overview]
Message-ID: <87eerszyim.fsf_-_@x220.int.ebiederm.org> (raw)
In-Reply-To: <87v9l4zyla.fsf_-_@x220.int.ebiederm.org> (Eric W. Biederman's message of "Sat, 09 May 2020 14:40:17 -0500")


Instead of recursing in search_binary_handler have the methods that
would recurse return a positive value, and simply loop in exec_binprm.

This is a trivial change as all of the methods that would recurse do
so as effectively the last thing they do.   Making this a trivial code
change.

Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>
---
 arch/alpha/kernel/binfmt_loader.c |  2 +-
 fs/binfmt_em86.c                  |  2 +-
 fs/binfmt_misc.c                  |  5 +----
 fs/binfmt_script.c                |  2 +-
 fs/exec.c                         | 20 +++++++++-----------
 include/linux/binfmts.h           |  2 --
 6 files changed, 13 insertions(+), 20 deletions(-)

diff --git a/arch/alpha/kernel/binfmt_loader.c b/arch/alpha/kernel/binfmt_loader.c
index a8d0d6e06526..a90c8b1d5498 100644
--- a/arch/alpha/kernel/binfmt_loader.c
+++ b/arch/alpha/kernel/binfmt_loader.c
@@ -38,7 +38,7 @@ static int load_binary(struct linux_binprm *bprm)
 	retval = prepare_binprm(bprm);
 	if (retval < 0)
 		return retval;
-	return search_binary_handler(bprm);
+	return 1; /* Search for the interpreter */
 }
 
 static struct linux_binfmt loader_format = {
diff --git a/fs/binfmt_em86.c b/fs/binfmt_em86.c
index 466497860c62..a9b9ac7f9bb0 100644
--- a/fs/binfmt_em86.c
+++ b/fs/binfmt_em86.c
@@ -95,7 +95,7 @@ static int load_em86(struct linux_binprm *bprm)
 	if (retval < 0)
 		return retval;
 
-	return search_binary_handler(bprm);
+	return 1; /* Search for the interpreter */
 }
 
 static struct linux_binfmt em86_format = {
diff --git a/fs/binfmt_misc.c b/fs/binfmt_misc.c
index cdb45829354d..127fae9c21ab 100644
--- a/fs/binfmt_misc.c
+++ b/fs/binfmt_misc.c
@@ -234,10 +234,7 @@ static int load_misc_binary(struct linux_binprm *bprm)
 	if (retval < 0)
 		goto error;
 
-	retval = search_binary_handler(bprm);
-	if (retval < 0)
-		goto error;
-
+	retval = 1; /* Search for the interpreter */
 ret:
 	dput(fmt->dentry);
 	return retval;
diff --git a/fs/binfmt_script.c b/fs/binfmt_script.c
index e9e6a6f4a35f..76a05696d376 100644
--- a/fs/binfmt_script.c
+++ b/fs/binfmt_script.c
@@ -146,7 +146,7 @@ static int load_script(struct linux_binprm *bprm)
 	retval = prepare_binprm(bprm);
 	if (retval < 0)
 		return retval;
-	return search_binary_handler(bprm);
+	return 1; /* Search for the interpreter */
 }
 
 static struct linux_binfmt script_format = {
diff --git a/fs/exec.c b/fs/exec.c
index 635b5085050c..8bbf5fa785a6 100644
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1690,16 +1690,12 @@ EXPORT_SYMBOL(remove_arg_zero);
 /*
  * cycle the list of binary formats handler, until one recognizes the image
  */
-int search_binary_handler(struct linux_binprm *bprm)
+static int search_binary_handler(struct linux_binprm *bprm)
 {
 	bool need_retry = IS_ENABLED(CONFIG_MODULES);
 	struct linux_binfmt *fmt;
 	int retval;
 
-	/* This allows 4 levels of binfmt rewrites before failing hard. */
-	if (bprm->recursion_depth > 5)
-		return -ELOOP;
-
 	retval = security_bprm_check(bprm);
 	if (retval)
 		return retval;
@@ -1712,10 +1708,7 @@ int search_binary_handler(struct linux_binprm *bprm)
 			continue;
 		read_unlock(&binfmt_lock);
 
-		bprm->recursion_depth++;
 		retval = fmt->load_binary(bprm);
-		bprm->recursion_depth--;
-
 		read_lock(&binfmt_lock);
 		put_binfmt(fmt);
 		if (bprm->point_of_no_return || !bprm->file ||
@@ -1738,12 +1731,11 @@ int search_binary_handler(struct linux_binprm *bprm)
 
 	return retval;
 }
-EXPORT_SYMBOL(search_binary_handler);
 
 static int exec_binprm(struct linux_binprm *bprm)
 {
 	pid_t old_pid, old_vpid;
-	int ret;
+	int ret, depth = 0;
 
 	/* Need to fetch pid before load_binary changes it */
 	old_pid = current->pid;
@@ -1751,7 +1743,13 @@ static int exec_binprm(struct linux_binprm *bprm)
 	old_vpid = task_pid_nr_ns(current, task_active_pid_ns(current->parent));
 	rcu_read_unlock();
 
-	ret = search_binary_handler(bprm);
+	do {
+		depth++;
+		ret = search_binary_handler(bprm);
+		/* This allows 4 levels of binfmt rewrites before failing hard. */
+		if ((ret > 0) && (depth > 5))
+			ret = -ELOOP;
+	} while (ret > 0);
 	if (ret >= 0) {
 		audit_bprm(bprm);
 		trace_sched_process_exec(current, old_pid, bprm);
diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h
index 42f760acfc2c..89f1135dcb75 100644
--- a/include/linux/binfmts.h
+++ b/include/linux/binfmts.h
@@ -47,7 +47,6 @@ struct linux_binprm {
 #ifdef __alpha__
 	unsigned int taso:1;
 #endif
-	unsigned int recursion_depth; /* only for search_binary_handler() */
 	struct file * file;
 	struct cred *cred;	/* new credentials */
 	int unsafe;		/* how unsafe this exec is (mask of LSM_UNSAFE_*) */
@@ -118,7 +117,6 @@ extern void unregister_binfmt(struct linux_binfmt *);
 
 extern int prepare_binprm(struct linux_binprm *);
 extern int __must_check remove_arg_zero(struct linux_binprm *);
-extern int search_binary_handler(struct linux_binprm *);
 extern int begin_new_exec(struct linux_binprm * bprm);
 extern void setup_new_exec(struct linux_binprm * bprm);
 extern void finalize_exec(struct linux_binprm *bprm);
-- 
2.25.0

