exec/binfmt_script: Don't modify bprm->buf and then return -ENOEXEC

The return code -ENOEXEC serves to tell search_binary_handler that it
should continue searching for the binfmt to handle a given file.  This
makes return -ENOEXEC with a bprm->buf that is needed to continue the
search problematic.

The current binfmt_script manages to escape problems as it closes and
clears bprm->file before return -ENOEXEC with bprm->buf modified.
This prevents search_binary_handler from looping as it explicitly
handles a NULL bprm->file.

I plan on moving all of the bprm->file managment into fs/exec.c and out
of the binary handlers so this will become a problem.

Move closing bprm->file and the test for BINPRM_PATH_INACCESSIBLE
down below the last return of -ENOEXEC.

Introduce i_sep and i_end to track the end of the first argument and
the end of the parameters respectively.  Using those, constification
of all char * pointers, and the helpers next_terminator and
next_non_spacetab guarantee the parameter parsing will not modify
bprm->buf.

Only modify bprm->buf to terminate the strings i_arg and i_name with
'\0' for passing to copy_strings_kernel.

When replacing loops with next_non_spacetab and next_terminator care
has been take that the logic of the parsing code (short of replacing
characters by '\0') remains the same.

Link: https://lkml.kernel.org/r/874ksczru6.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/fs/binfmt_script.c b/fs/binfmt_script.c
index 8d718d8fd0fe7a32a5f29cf8faa4bb8f6e10157a..85e0ef86eb1197170ed4e40bbd22a7cbd121d44a 100644 (file)
--- a/fs/binfmt_script.c
+++ b/fs/binfmt_script.c
@@ -16,14 +16,14 @@
 #include <linux/fs.h>
 
 static inline bool spacetab(char c) { return c == ' ' || c == '\t'; }
-static inline char *next_non_spacetab(char *first, const char *last)
+static inline const char *next_non_spacetab(const char *first, const char *last)
 {
        for (; first <= last; first++)
                if (!spacetab(*first))
                        return first;
        return NULL;
 }
-static inline char *next_terminator(char *first, const char *last)
+static inline const char *next_terminator(const char *first, const char *last)
 {
        for (; first <= last; first++)
                if (spacetab(*first) || !*first)
@@ -33,8 +33,7 @@ static inline char *next_terminator(char *first, const char *last)
 
 static int load_script(struct linux_binprm *bprm)
 {
-       const char *i_arg, *i_name;
-       char *cp, *buf_end;
+       const char *i_name, *i_sep, *i_arg, *i_end, *buf_end;
        struct file *file;
        int retval;
 
@@ -42,20 +41,6 @@ static int load_script(struct linux_binprm *bprm)
        if ((bprm->buf[0] != '#') || (bprm->buf[1] != '!'))
                return -ENOEXEC;
 
-       /*
-        * If the script filename will be inaccessible after exec, typically
-        * because it is a "/dev/fd/<fd>/.." path against an O_CLOEXEC fd, give
-        * up now (on the assumption that the interpreter will want to load
-        * this file).
-        */
-       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;
-
        /*
         * This section handles parsing the #! line into separate
         * interpreter path and argument strings. We must be careful
@@ -71,39 +56,48 @@ static int load_script(struct linux_binprm *bprm)
         * parse them on its own.
         */
        buf_end = bprm->buf + sizeof(bprm->buf) - 1;
-       cp = strnchr(bprm->buf, sizeof(bprm->buf), '\n');
-       if (!cp) {
-               cp = next_non_spacetab(bprm->buf + 2, buf_end);
-               if (!cp)
+       i_end = strnchr(bprm->buf, sizeof(bprm->buf), '\n');
+       if (!i_end) {
+               i_end = next_non_spacetab(bprm->buf + 2, buf_end);
+               if (!i_end)
                        return -ENOEXEC; /* Entire buf is spaces/tabs */
                /*
                 * If there is no later space/tab/NUL we must assume the
                 * interpreter path is truncated.
                 */
-               if (!next_terminator(cp, buf_end))
+               if (!next_terminator(i_end, buf_end))
                        return -ENOEXEC;
-               cp = buf_end;
+               i_end = buf_end;
        }
-       /* NUL-terminate the buffer and any trailing spaces/tabs. */
-       *cp = '\0';
-       while (cp > bprm->buf) {
-               cp--;
-               if ((*cp == ' ') || (*cp == '\t'))
-                       *cp = '\0';
-               else
-                       break;
-       }
-       for (cp = bprm->buf+2; (*cp == ' ') || (*cp == '\t'); cp++);
-       if (*cp == '\0')
+       /* Trim any trailing spaces/tabs from i_end */
+       while (spacetab(i_end[-1]))
+               i_end--;
+
+       /* Skip over leading spaces/tabs */
+       i_name = next_non_spacetab(bprm->buf+2, i_end);
+       if (!i_name || (i_name == i_end))
                return -ENOEXEC; /* No interpreter name found */
-       i_name = cp;
+
+       /* Is there an optional argument? */
        i_arg = NULL;
-       for ( ; *cp && (*cp != ' ') && (*cp != '\t'); cp++)
-               /* nothing */ ;
-       while ((*cp == ' ') || (*cp == '\t'))
-               *cp++ = '\0';
-       if (*cp)
-               i_arg = cp;
+       i_sep = next_terminator(i_name, i_end);
+       if (i_sep && (*i_sep != '\0'))
+               i_arg = next_non_spacetab(i_sep, i_end);
+
+       /*
+        * If the script filename will be inaccessible after exec, typically
+        * because it is a "/dev/fd/<fd>/.." path against an O_CLOEXEC fd, give
+        * up now (on the assumption that the interpreter will want to load
+        * this file).
+        */
+       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.
@@ -121,7 +115,9 @@ static int load_script(struct linux_binprm *bprm)
        if (retval < 0)
                return retval;
        bprm->argc++;
+       *((char *)i_end) = '\0';
        if (i_arg) {
+               *((char *)i_sep) = '\0';
                retval = copy_strings_kernel(1, &i_arg, bprm);
                if (retval < 0)
                        return retval;