On Mon, Jun 21, 2021 at 02:27:47PM -0500, Eric W. Biederman wrote: > 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; Your patch changes the meaning of i_end. Now it points to the last symbol, but this function contains the line: *((char *)i_end) = '\0'; and it drops the last meaningful symbol. With the following tiny fix, my test passes: @@ -114,7 +115,7 @@ static int load_script(struct linux_binprm *bprm) if (retval < 0) return retval; bprm->argc++; - *((char *)i_end) = '\0'; + *((char *)(i_end + 1)) = '\0'; if (i_arg) { *((char *)i_sep) = '\0'; retval = copy_string_kernel(i_arg, bprm); Thanks, Andrei
