Andrei Vagin <avagin@xxxxxxxxx> writes: > On Tue, Jun 15, 2021 at 12:33 PM Eric W. Biederman > <ebiederm@xxxxxxxxxxxx> wrote: >> >> Andrei Vagin <avagin@xxxxxxxxx> writes: >> >> > Without this fix, if we try to run a script that contains only the >> > interpreter line, the interpreter is executed with one extra empty >> > argument. >> > >> > The code is written so that i_end has to be set to the end of valuable >> > data in the buffer. >> >> Out of curiosity how did you spot this change in behavior? > > gVisor tests started failing with this change: > https://github.com/google/gvisor/blob/5e05950c1c520724e2e03963850868befb95efeb/test/syscalls/linux/exec.cc#L307 > > We run these tests on Ubuntu 20.04 and this is the reason why we > caught this issue just a few days ago. I like where you are going, but starting at the end of the buffer there is the potential to skip deliberately embedded '\0' characters. While looking at this I realized that your patch should not have made a difference but there is a subtle bug in the logic of next_non_spacetab, that allowed your code to make it that far. Can you test my patch below? I think I have simplified the logic enough to prevent bugs from getting in. Eric diff --git a/fs/binfmt_script.c b/fs/binfmt_script.c index 1b6625e95958..7d204693326c 100644 --- a/fs/binfmt_script.c +++ b/fs/binfmt_script.c @@ -26,7 +26,7 @@ static inline const char *next_non_spacetab(const 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) + if (spacetab(*first)) return first; return NULL; } @@ -44,9 +44,9 @@ static int load_script(struct linux_binprm *bprm) /* * This section handles parsing the #! line into separate * interpreter path and argument strings. We must be careful - * because bprm->buf is not yet guaranteed to be NUL-terminated - * (though the buffer will have trailing NUL padding when the - * file size was smaller than the buffer size). + * because bprm->buf is not guaranteed to be NUL-terminated + * (the buffer will have trailing NUL padding when the file + * size was smaller than the buffer size). * * We do not want to exec a truncated interpreter path, so either * we find a newline (which indicates nothing is truncated), or @@ -57,33 +57,37 @@ static int load_script(struct linux_binprm *bprm) */ buf_end = bprm->buf + sizeof(bprm->buf) - 1; 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(i_end, buf_end)) - return -ENOEXEC; - i_end = buf_end; + if (i_end) { + /* Hide the trailing newline */ + i_end = i_end - 1; + } else { + /* Find the end of the text */ + i_end = memchr(bprm->buf + 2, '\0', sizeof(bprm->buf)); + i_end = i_end ? i_end - 1 : buf_end; } + /* Trim any trailing spaces/tabs from i_end */ - while (spacetab(i_end[-1])) + while (spacetab(i_end[0])) i_end--; /* Skip over leading spaces/tabs */ i_name = next_non_spacetab(bprm->buf+2, i_end); - if (!i_name || (i_name == i_end)) + if (!i_name) return -ENOEXEC; /* No interpreter name found */ /* Is there an optional argument? */ i_arg = NULL; i_sep = next_terminator(i_name, i_end); - if (i_sep && (*i_sep != '\0')) + if (i_sep) i_arg = next_non_spacetab(i_sep, i_end); + /* + * If there is no space/tab/NUL after the interpreter we must + * assume the interpreter path is truncated. + */ + if (!i_sep && (i_end == buf_end)) + 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
