Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 23.128.96.18 as permitted sender) client-ip=23.128.96.18;
From:   ebiederm@xmission.com (Eric W. Biederman)
To:     Andrei Vagin <avagin@gmail.com>
Cc:     LKML <linux-kernel@vger.kernel.org>,
        Kees Cook <keescook@chromium.org>,
        linux-fsdevel <linux-fsdevel@vger.kernel.org>
References: <20210615162346.16032-1-avagin@gmail.com>
        <877diuq5xb.fsf@disp2133>
        <CANaxB-zVMxxvt8c1XNKfy6-hAUoodxp=ChJpP_Rn5cTD=26p9w@mail.gmail.com>
Date:   Mon, 21 Jun 2021 14:27:47 -0500
In-Reply-To: <CANaxB-zVMxxvt8c1XNKfy6-hAUoodxp=ChJpP_Rn5cTD=26p9w@mail.gmail.com>
        (Andrei Vagin's message of "Tue, 15 Jun 2021 15:35:14 -0700")
Message-ID: <87pmwfggr0.fsf@disp2133>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/26.1 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
Subject: Re: [PATCH] exec/binfmt_script: trip zero bytes from the buffer
Precedence: bulk

Andrei Vagin <avagin@gmail.com> writes:

> On Tue, Jun 15, 2021 at 12:33 PM Eric W. Biederman
> <ebiederm@xmission.com> wrote:
>>
>> Andrei Vagin <avagin@gmail.com> 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