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>
Subject: Re: [PATCH] exec/binfmt_script: trip zero bytes from the buffer
Date: Mon, 21 Jun 2021 14:27:47 -0500 [thread overview]
Message-ID: <87pmwfggr0.fsf@disp2133> (raw)
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")
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
next prev parent reply other threads:[~2021-06-21 19:28 UTC|newest]
Thread overview: 5+ messages / expand[flat|nested] mbox.gz Atom feed top
2021-06-15 16:23 [PATCH] exec/binfmt_script: trip zero bytes from the buffer Andrei Vagin
2021-06-15 19:33 ` Eric W. Biederman
2021-06-15 22:35 ` Andrei Vagin
2021-06-21 19:27 ` Eric W. Biederman [this message]
2021-06-25 8:30 ` Andrei Vagin
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=87pmwfggr0.fsf@disp2133 \
--to=ebiederm@xmission.com \
--cc=avagin@gmail.com \
--cc=keescook@chromium.org \
--cc=linux-fsdevel@vger.kernel.org \
--cc=linux-kernel@vger.kernel.org \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).