exec: Remove recursion from search_binary_handler

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.

Link: https://lkml.kernel.org/r/87sgfwyd84.fsf_-_@x220.int.ebiederm.org
Acked-by: Linus Torvalds <torvalds@linux-foundation.org>
Reviewed-by: Kees Cook <keescook@chromium.org>
Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com>

diff --git a/arch/alpha/kernel/binfmt_loader.c b/arch/alpha/kernel/binfmt_loader.c
index d712ba51d15adf17781056ed088e7e73574ef8fa..e4be7a543ecfd4dba7f50d2e7221a915a21897f9 100644 (file)
--- a/arch/alpha/kernel/binfmt_loader.c
+++ b/arch/alpha/kernel/binfmt_loader.c
@@ -19,10 +19,6 @@ static int load_binary(struct linux_binprm *bprm)
        if (bprm->loader)
                return -ENOEXEC;
 
-       allow_write_access(bprm->file);
-       fput(bprm->file);
-       bprm->file = NULL;
-
        loader = bprm->vma->vm_end - sizeof(void *);
 
        file = open_exec("/sbin/loader");
@@ -33,9 +29,9 @@ static int load_binary(struct linux_binprm *bprm)
        /* Remember if the application is TASO.  */
        bprm->taso = eh->ah.entry < 0x100000000UL;
 
-       bprm->file = file;
+       bprm->interpreter = file;
        bprm->loader = loader;
-       return search_binary_handler(bprm);
+       return 0;
 }
 
 static struct linux_binfmt loader_format = {

diff --git a/fs/binfmt_em86.c b/fs/binfmt_em86.c
index cedde2341ade28cea8bd434276b540ee3cf2fcd9..995883693cb224f6f85dd00212502115cbd3d6fe 100644 (file)
--- a/fs/binfmt_em86.c
+++ b/fs/binfmt_em86.c
@@ -48,10 +48,6 @@ static int load_em86(struct linux_binprm *bprm)
        if (bprm->interp_flags & BINPRM_FLAGS_PATH_INACCESSIBLE)
                return -ENOENT;
 
-       allow_write_access(bprm->file);
-       fput(bprm->file);
-       bprm->file = NULL;
-
        /* Unlike in the script case, we don't have to do any hairy
         * parsing to find our interpreter... it's hardcoded!
         */
@@ -89,9 +85,8 @@ static int load_em86(struct linux_binprm *bprm)
        if (IS_ERR(file))
                return PTR_ERR(file);
 
-       bprm->file = file;
-
-       return search_binary_handler(bprm);
+       bprm->interpreter = file;
+       return 0;
 }
 
 static struct linux_binfmt em86_format = {

diff --git a/fs/binfmt_misc.c b/fs/binfmt_misc.c
index ad2866f28f0cfa82002a5eed1e9e166b11d1e777..53968ea07b575e1fa89dd1625a3cf84b5f868d4a 100644 (file)
--- a/fs/binfmt_misc.c
+++ b/fs/binfmt_misc.c
@@ -159,18 +159,9 @@ static int load_misc_binary(struct linux_binprm *bprm)
                        goto ret;
        }
 
-       if (fmt->flags & MISC_FMT_OPEN_BINARY) {
-               /* Pass the open binary to the interpreter */
+       if (fmt->flags & MISC_FMT_OPEN_BINARY)
                bprm->have_execfd = 1;
-               bprm->executable = bprm->file;
 
-               allow_write_access(bprm->file);
-               bprm->file = NULL;
-       } else {
-               allow_write_access(bprm->file);
-               fput(bprm->file);
-               bprm->file = NULL;
-       }
        /* make argv[1] be the path to the binary */
        retval = copy_strings_kernel(1, &bprm->interp, bprm);
        if (retval < 0)
@@ -199,14 +190,11 @@ static int load_misc_binary(struct linux_binprm *bprm)
        if (IS_ERR(interp_file))
                goto ret;
 
-       bprm->file = interp_file;
+       bprm->interpreter = interp_file;
        if (fmt->flags & MISC_FMT_CREDENTIALS)
                bprm->preserve_creds = 1;
 
-       retval = search_binary_handler(bprm);
-       if (retval < 0)
-               goto ret;
-
+       retval = 0;
 ret:
        dput(fmt->dentry);
        return retval;

diff --git a/fs/binfmt_script.c b/fs/binfmt_script.c
index 85e0ef86eb1197170ed4e40bbd22a7cbd121d44a..0e8b953d12cf88cb4bda5e19e218f9f7a9999d15 100644 (file)
--- a/fs/binfmt_script.c
+++ b/fs/binfmt_script.c
@@ -93,11 +93,6 @@ static int load_script(struct linux_binprm *bprm)
        if (bprm->interp_flags & BINPRM_FLAGS_PATH_INACCESSIBLE)
                return -ENOENT;
 
-       /* Release since we are not mapping a binary into memory. */
-       allow_write_access(bprm->file);
-       fput(bprm->file);
-       bprm->file = NULL;
-
        /*
         * OK, we've parsed out the interpreter name and
         * (optional) argument.
@@ -138,8 +133,8 @@ static int load_script(struct linux_binprm *bprm)
        if (IS_ERR(file))
                return PTR_ERR(file);
 
-       bprm->file = file;
-       return search_binary_handler(bprm);
+       bprm->interpreter = file;
+       return 0;
 }
 
 static struct linux_binfmt script_format = {

diff --git a/fs/exec.c b/fs/exec.c
index 117ad8fc012be505adf94337823ee9b68e7ed567..c3c879a55d65496923392039053954de43573265 100644 (file)
--- a/fs/exec.c
+++ b/fs/exec.c
@@ -1710,16 +1710,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 = prepare_binprm(bprm);
        if (retval < 0)
                return retval;
@@ -1736,14 +1732,11 @@ 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 ||
-                   (retval != -ENOEXEC)) {
+               if (bprm->point_of_no_return || (retval != -ENOEXEC)) {
                        read_unlock(&binfmt_lock);
                        return retval;
                }
@@ -1762,12 +1755,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;
 
        /* Need to fetch pid before load_binary changes it */
        old_pid = current->pid;
@@ -1775,15 +1767,38 @@ 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);
-       if (ret >= 0) {
-               audit_bprm(bprm);
-               trace_sched_process_exec(current, old_pid, bprm);
-               ptrace_event(PTRACE_EVENT_EXEC, old_vpid);
-               proc_exec_connector(current);
+       /* 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);
+               if (ret < 0)
+                       return ret;
+               if (!bprm->interpreter)
+                       break;
+
+               exec = bprm->file;
+               bprm->file = bprm->interpreter;
+               bprm->interpreter = NULL;
+
+               allow_write_access(exec);
+               if (unlikely(bprm->have_execfd)) {
+                       if (bprm->executable) {
+                               fput(exec);
+                               return -ENOEXEC;
+                       }
+                       bprm->executable = exec;
+               } else
+                       fput(exec);
        }
 
-       return ret;
+       audit_bprm(bprm);
+       trace_sched_process_exec(current, old_pid, bprm);
+       ptrace_event(PTRACE_EVENT_EXEC, old_vpid);
+       proc_exec_connector(current);
+       return 0;
 }
 
 /*

diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h
index 653508b258151356d9021c73b75774f209a5d808..7fc05929c967a0c82411ec8083ff0687ae69378b 100644 (file)
--- a/include/linux/binfmts.h
+++ b/include/linux/binfmts.h
@@ -50,8 +50,8 @@ struct linux_binprm {
 #ifdef __alpha__
        unsigned int taso:1;
 #endif
-       unsigned int recursion_depth; /* only for search_binary_handler() */
        struct file * executable; /* Executable to pass to the interpreter */
+       struct file * interpreter;
        struct file * file;
        struct cred *cred;      /* new credentials */
        int unsafe;             /* how unsafe this exec is (mask of LSM_UNSAFE_*) */
@@ -117,7 +117,6 @@ static inline void insert_binfmt(struct linux_binfmt *fmt)
 extern void unregister_binfmt(struct linux_binfmt *);
 
 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);