exec: load_script: kill the onstack interp[BINPRM_BUF_SIZE] array
Subsystems (2)
PATCH 1/1 exec: load_script: kill the onstack interp[BINPRM_BUF_SIZE] array
load_script() can simply use i_name instead, it points into bprm->buf[] and nobody can change this memory until we call prepare_binprm().
The only complication is that we need to also change the signature of bprm_change_interp() but this change looks good too.
While at it, do whitespace/style cleanups.
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.
--- fs/binfmt_script.c | 17 +++++++++-------- fs/exec.c | 2 +- include/linux/binfmts.h | 2 +- 3 files changed, 11 insertions(+), 10 deletions(-)
fs/binfmt_script.c +9 -8
@@ -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);
fs/exec.c +1 -1
@@ -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)
include/linux/binfmts.h +1 -1
@@ -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);
Signature · 1 line
2.5.0
Quoted 2 lines · Oleg Nesterov
load_script()can simply usei_nameinstead, it points into bprm->buf[] and nobody can change this memory until we callprepare_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. :)
Quoted 2 lines
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.
Quoted 1 line
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.
Quoted 5 lines
NOTE: the real motivation for this change is that people want to increase
BINPRM_BUF_SIZE, we need to changeload_misc_binary()too but this looks more complicated because afaics it is very buggy.Signed-off-by Oleg Nesterov <[email protected]>
-Kees
Quoted 98 lines
--- fs/binfmt_script.c | 17 +++++++++-------- fs/exec.c | 2 +- include/linux/binfmts.h | 2 +- 3 files changed, 11 insertions(+), 10 deletions(-)fs/binfmt_script.c +9
-8@@ -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);fs/exec.c +1
-1@@ -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)include/linux/binfmts.h +1
-2@@ -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
Signature · 2 lines
Kees Cook Pixel Security