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.
Fixes: ccbb18b67323 ("exec/binfmt_script: Don't modify bprm->buf and then return -ENOEXEC")
Cc: "Eric W. Biederman" <[email protected]>
Signed-off-by: Andrei Vagin <[email protected]>
---
fs/binfmt_script.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/fs/binfmt_script.c b/fs/binfmt_script.c
index 1b6625e95958..e242680f96e1 100644
--- a/fs/binfmt_script.c
+++ b/fs/binfmt_script.c
@@ -68,6 +68,9 @@ static int load_script(struct linux_binprm *bprm)
if (!next_terminator(i_end, buf_end))
return -ENOEXEC;
i_end = buf_end;
+ /* Trim zero bytes from i_end */
+ while (i_end[-1] == 0)
+ i_end--;
}
/* Trim any trailing spaces/tabs from i_end */
while (spacetab(i_end[-1]))
--
2.31.1
Andrei Vagin <[email protected]> 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?
> Fixes: ccbb18b67323 ("exec/binfmt_script: Don't modify bprm->buf and then return -ENOEXEC")
> Cc: "Eric W. Biederman" <[email protected]>
> Signed-off-by: Andrei Vagin <[email protected]>
> ---
> fs/binfmt_script.c | 3 +++
> 1 file changed, 3 insertions(+)
>
> diff --git a/fs/binfmt_script.c b/fs/binfmt_script.c
> index 1b6625e95958..e242680f96e1 100644
> --- a/fs/binfmt_script.c
> +++ b/fs/binfmt_script.c
> @@ -68,6 +68,9 @@ static int load_script(struct linux_binprm *bprm)
> if (!next_terminator(i_end, buf_end))
> return -ENOEXEC;
> i_end = buf_end;
> + /* Trim zero bytes from i_end */
> + while (i_end[-1] == 0)
> + i_end--;
> }
> /* Trim any trailing spaces/tabs from i_end */
> while (spacetab(i_end[-1]))
On Tue, Jun 15, 2021 at 12:33 PM Eric W. Biederman
<[email protected]> wrote:
>
> Andrei Vagin <[email protected]> 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.
>
> > Fixes: ccbb18b67323 ("exec/binfmt_script: Don't modify bprm->buf and then return -ENOEXEC")
> > Cc: "Eric W. Biederman" <[email protected]>
> > Signed-off-by: Andrei Vagin <[email protected]>
> > ---
> > fs/binfmt_script.c | 3 +++
> > 1 file changed, 3 insertions(+)
> >
> > diff --git a/fs/binfmt_script.c b/fs/binfmt_script.c
> > index 1b6625e95958..e242680f96e1 100644
> > --- a/fs/binfmt_script.c
> > +++ b/fs/binfmt_script.c
> > @@ -68,6 +68,9 @@ static int load_script(struct linux_binprm *bprm)
> > if (!next_terminator(i_end, buf_end))
> > return -ENOEXEC;
> > i_end = buf_end;
> > + /* Trim zero bytes from i_end */
> > + while (i_end[-1] == 0)
> > + i_end--;
> > }
> > /* Trim any trailing spaces/tabs from i_end */
> > while (spacetab(i_end[-1]))
Andrei Vagin <[email protected]> writes:
> On Tue, Jun 15, 2021 at 12:33 PM Eric W. Biederman
> <[email protected]> wrote:
>>
>> Andrei Vagin <[email protected]> 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
On Mon, Jun 21, 2021 at 02:27:47PM -0500, Eric W. Biederman wrote:
> Andrei Vagin <[email protected]> writes:
>
> > On Tue, Jun 15, 2021 at 12:33 PM Eric W. Biederman
> > <[email protected]> wrote:
> >>
> >> Andrei Vagin <[email protected]> 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