linux-kernel - Re: [PATCH] exec: load_script: kill the onstack interp[BINPRM_BUF

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives
Hash Suite: Windows password security audit tool. GUI, reports in PDF.
[<prev] [next>] [<thread-prev] [day] [month] [year] [list]
Message-ID: <CAGXu5jKpN2HkK1GvjSdGUccXhVP4_LPqZJSfvFaWK=WkwGf14Q@mail.gmail.com>
Date:   Mon, 18 Sep 2017 13:25:39 -0700
From:   Kees Cook <keescook@...omium.org>
To:     Oleg Nesterov <oleg@...hat.com>
Cc:     Andrew Morton <akpm@...ux-foundation.org>,
        Ben Woodard <woodard@...hat.com>,
        Travis Gummels <tgummels@...hat.com>, tdhooge@...l.gov,
        Jim Foraker <foraker1@...l.gov>,
        LKML <linux-kernel@...r.kernel.org>
Subject: Re: [PATCH] exec: load_script: kill the onstack interp[BINPRM_BUF_SIZE]
 array

On Mon, Sep 18, 2017 at 9:34 AM, Oleg Nesterov <oleg@...hat.com> wrote:
> load_script() can simply use i_name instead, it points into bprm->buf[]
> and nobody can change this memory until we call prepare_binprm().

It looks like I missed this after commit b66c59840175 ("exec: do not
leave bprm->interp on stack"), where it became possible to remove the
stack copy in load_script()'s interp variable entirely. Yay, we save a
strcpy in exec. :)

> The only complication is that we need to also change the signature of
> bprm_change_interp() but this change looks good too.

That's totally fine.

> While at it, do whitespace/style cleanups.

Yes please. The code could use some comments too, frankly. The string
manipulation around building the i_name vs i_arg pointers takes some
time to make sense of, IMO.

> NOTE: the real motivation for this change is that people want to increase
> BINPRM_BUF_SIZE, we need to change load_misc_binary() too but this looks
> more complicated because afaics it is very buggy.
>
> Signed-off-by: Oleg Nesterov <oleg@...hat.com>

Acked-by: Kees Cook <keescook@...omium.org>

-Kees

> ---
>  fs/binfmt_script.c      | 17 +++++++++--------
>  fs/exec.c               |  2 +-
>  include/linux/binfmts.h |  2 +-
>  3 files changed, 11 insertions(+), 10 deletions(-)
>
> diff --git a/fs/binfmt_script.c b/fs/binfmt_script.c
> index afdf4e3..7cde3f4 100644
> --- a/fs/binfmt_script.c
> +++ b/fs/binfmt_script.c
> @@ -19,7 +19,6 @@ static int load_script(struct linux_binprm *bprm)
>         const char *i_arg, *i_name;
>         char *cp;
>         struct file *file;
> -       char interp[BINPRM_BUF_SIZE];
>         int retval;
>
>         if ((bprm->buf[0] != '#') || (bprm->buf[1] != '!'))
> @@ -55,7 +54,7 @@ static int load_script(struct linux_binprm *bprm)
>                         break;
>         }
>         for (cp = bprm->buf+2; (*cp == ' ') || (*cp == '\t'); cp++);
> -       if (*cp == '\0')
> +       if (*cp == '\0')
>                 return -ENOEXEC; /* No interpreter name found */
>         i_name = cp;
>         i_arg = NULL;
> @@ -65,7 +64,6 @@ static int load_script(struct linux_binprm *bprm)
>                 *cp++ = '\0';
>         if (*cp)
>                 i_arg = cp;
> -       strcpy (interp, i_name);
>         /*
>          * OK, we've parsed out the interpreter name and
>          * (optional) argument.
> @@ -80,24 +78,27 @@ static int load_script(struct linux_binprm *bprm)
>         if (retval)
>                 return retval;
>         retval = copy_strings_kernel(1, &bprm->interp, bprm);
> -       if (retval < 0) return retval;
> +       if (retval < 0)
> +               return retval;
>         bprm->argc++;
>         if (i_arg) {
>                 retval = copy_strings_kernel(1, &i_arg, bprm);
> -               if (retval < 0) return retval;
> +               if (retval < 0)
> +                       return retval;
>                 bprm->argc++;
>         }
>         retval = copy_strings_kernel(1, &i_name, bprm);
> -       if (retval) return retval;
> +       if (retval)
> +               return retval;
>         bprm->argc++;
> -       retval = bprm_change_interp(interp, bprm);
> +       retval = bprm_change_interp(i_name, bprm);
>         if (retval < 0)
>                 return retval;
>
>         /*
>          * OK, now restart the process with the interpreter's dentry.
>          */
> -       file = open_exec(interp);
> +       file = open_exec(i_name);
>         if (IS_ERR(file))
>                 return PTR_ERR(file);
>
> diff --git a/fs/exec.c b/fs/exec.c
> index 01a9fb9..8a665ec 100644
> --- a/fs/exec.c
> +++ b/fs/exec.c
> @@ -1429,7 +1429,7 @@ static void free_bprm(struct linux_binprm *bprm)
>         kfree(bprm);
>  }
>
> -int bprm_change_interp(char *interp, struct linux_binprm *bprm)
> +int bprm_change_interp(const char *interp, struct linux_binprm *bprm)
>  {
>         /* If a binfmt changed the interp, free it first. */
>         if (bprm->interp != bprm->filename)
> diff --git a/include/linux/binfmts.h b/include/linux/binfmts.h
> index fb44d61..18d05b5 100644
> --- a/include/linux/binfmts.h
> +++ b/include/linux/binfmts.h
> @@ -131,7 +131,7 @@ extern int setup_arg_pages(struct linux_binprm * bprm,
>                            int executable_stack);
>  extern int transfer_args_to_stack(struct linux_binprm *bprm,
>                                   unsigned long *sp_location);
> -extern int bprm_change_interp(char *interp, struct linux_binprm *bprm);
> +extern int bprm_change_interp(const char *interp, struct linux_binprm *bprm);
>  extern int copy_strings_kernel(int argc, const char *const *argv,
>                                struct linux_binprm *bprm);
>  extern int prepare_bprm_creds(struct linux_binprm *bprm);
> --
> 2.5.0
>
>



-- 
Kees Cook
Pixel Security