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 PHC | |
Open Source and information security mailing list archives
| ||
|
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
Powered by blists - more mailing lists