Re: [PATCH] exec/binfmt_script: trip zero bytes from the buffer

[Andrei Vagin]

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