| Message ID | 87bln2jhki.fsf@x220.int.ebiederm.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/7] binfmt: Move install_exec_creds after setup_new_exec to match binfmt_elf | expand |
On Tue, May 05, 2020 at 02:41:01PM -0500, Eric W. Biederman wrote: > > In 2016 Linus moved install_exec_creds immediately after > setup_new_exec, in binfmt_elf as a cleanup and as part of closing a > potential information leak. > > Perform the same cleanup for the other binary formats. > > Different binary formats doing the same things the same way makes exec > easier to reason about and easier to maintain. > > The binfmt_flagt bits were tested by Greg Ungerer <gerg@linux-m68k.org> > > Ref: 9f834ec18def ("binfmt_elf: switch to new creds when switching to new mm") > Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> Reviewed-by: Kees Cook <keescook@chromium.org>
One small nit: On 6/5/20 5:41 am, Eric W. Biederman wrote: > In 2016 Linus moved install_exec_creds immediately after > setup_new_exec, in binfmt_elf as a cleanup and as part of closing a > potential information leak. > > Perform the same cleanup for the other binary formats. > > Different binary formats doing the same things the same way makes exec > easier to reason about and easier to maintain. > > The binfmt_flagt bits were tested by Greg Ungerer <gerg@linux-m68k.org> ^^^^^ flat Regards Greg > Ref: 9f834ec18def ("binfmt_elf: switch to new creds when switching to new mm") > Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> > --- > arch/x86/ia32/ia32_aout.c | 3 +-- > fs/binfmt_aout.c | 2 +- > fs/binfmt_elf_fdpic.c | 2 +- > fs/binfmt_flat.c | 3 +-- > 4 files changed, 4 insertions(+), 6 deletions(-) > > diff --git a/arch/x86/ia32/ia32_aout.c b/arch/x86/ia32/ia32_aout.c > index 9bb71abd66bd..37b36a8ce5fa 100644 > --- a/arch/x86/ia32/ia32_aout.c > +++ b/arch/x86/ia32/ia32_aout.c > @@ -140,6 +140,7 @@ static int load_aout_binary(struct linux_binprm *bprm) > set_personality_ia32(false); > > setup_new_exec(bprm); > + install_exec_creds(bprm); > > regs->cs = __USER32_CS; > regs->r8 = regs->r9 = regs->r10 = regs->r11 = regs->r12 = > @@ -156,8 +157,6 @@ static int load_aout_binary(struct linux_binprm *bprm) > if (retval < 0) > return retval; > > - install_exec_creds(bprm); > - > if (N_MAGIC(ex) == OMAGIC) { > unsigned long text_addr, map_size; > > diff --git a/fs/binfmt_aout.c b/fs/binfmt_aout.c > index 8e8346a81723..ace587b66904 100644 > --- a/fs/binfmt_aout.c > +++ b/fs/binfmt_aout.c > @@ -162,6 +162,7 @@ static int load_aout_binary(struct linux_binprm * bprm) > set_personality(PER_LINUX); > #endif > setup_new_exec(bprm); > + install_exec_creds(bprm); > > current->mm->end_code = ex.a_text + > (current->mm->start_code = N_TXTADDR(ex)); > @@ -174,7 +175,6 @@ static int load_aout_binary(struct linux_binprm * bprm) > if (retval < 0) > return retval; > > - install_exec_creds(bprm); > > if (N_MAGIC(ex) == OMAGIC) { > unsigned long text_addr, map_size; > diff --git a/fs/binfmt_elf_fdpic.c b/fs/binfmt_elf_fdpic.c > index 240f66663543..6c94c6d53d97 100644 > --- a/fs/binfmt_elf_fdpic.c > +++ b/fs/binfmt_elf_fdpic.c > @@ -353,6 +353,7 @@ static int load_elf_fdpic_binary(struct linux_binprm *bprm) > current->personality |= READ_IMPLIES_EXEC; > > setup_new_exec(bprm); > + install_exec_creds(bprm); > > set_binfmt(&elf_fdpic_format); > > @@ -434,7 +435,6 @@ static int load_elf_fdpic_binary(struct linux_binprm *bprm) > current->mm->start_stack = current->mm->start_brk + stack_size; > #endif > > - install_exec_creds(bprm); > if (create_elf_fdpic_tables(bprm, current->mm, > &exec_params, &interp_params) < 0) > goto error; > diff --git a/fs/binfmt_flat.c b/fs/binfmt_flat.c > index 831a2b25ba79..1a1d1fcb893f 100644 > --- a/fs/binfmt_flat.c > +++ b/fs/binfmt_flat.c > @@ -541,6 +541,7 @@ static int load_flat_file(struct linux_binprm *bprm, > /* OK, This is the point of no return */ > set_personality(PER_LINUX_32BIT); > setup_new_exec(bprm); > + install_exec_creds(bprm); > } > > /* > @@ -963,8 +964,6 @@ static int load_flat_binary(struct linux_binprm *bprm) > } > } > > - install_exec_creds(bprm); > - > set_binfmt(&flat_format); > > #ifdef CONFIG_MMU >
Greg Ungerer <gerg@linux-m68k.org> writes: > One small nit: Good point. > On 6/5/20 5:41 am, Eric W. Biederman wrote: >> In 2016 Linus moved install_exec_creds immediately after >> setup_new_exec, in binfmt_elf as a cleanup and as part of closing a >> potential information leak. >> >> Perform the same cleanup for the other binary formats. >> >> Different binary formats doing the same things the same way makes exec >> easier to reason about and easier to maintain. >> >> The binfmt_flagt bits were tested by Greg Ungerer <gerg@linux-m68k.org> > ^^^^^ > flat > > Regards > Greg > > >> Ref: 9f834ec18def ("binfmt_elf: switch to new creds when switching to new mm") >> Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> >> --- >> arch/x86/ia32/ia32_aout.c | 3 +-- >> fs/binfmt_aout.c | 2 +- >> fs/binfmt_elf_fdpic.c | 2 +- >> fs/binfmt_flat.c | 3 +-- >> 4 files changed, 4 insertions(+), 6 deletions(-) >> >> diff --git a/arch/x86/ia32/ia32_aout.c b/arch/x86/ia32/ia32_aout.c >> index 9bb71abd66bd..37b36a8ce5fa 100644 >> --- a/arch/x86/ia32/ia32_aout.c >> +++ b/arch/x86/ia32/ia32_aout.c >> @@ -140,6 +140,7 @@ static int load_aout_binary(struct linux_binprm *bprm) >> set_personality_ia32(false); >> setup_new_exec(bprm); >> + install_exec_creds(bprm); >> regs->cs = __USER32_CS; >> regs->r8 = regs->r9 = regs->r10 = regs->r11 = regs->r12 = >> @@ -156,8 +157,6 @@ static int load_aout_binary(struct linux_binprm *bprm) >> if (retval < 0) >> return retval; >> - install_exec_creds(bprm); >> - >> if (N_MAGIC(ex) == OMAGIC) { >> unsigned long text_addr, map_size; >> diff --git a/fs/binfmt_aout.c b/fs/binfmt_aout.c >> index 8e8346a81723..ace587b66904 100644 >> --- a/fs/binfmt_aout.c >> +++ b/fs/binfmt_aout.c >> @@ -162,6 +162,7 @@ static int load_aout_binary(struct linux_binprm * bprm) >> set_personality(PER_LINUX); >> #endif >> setup_new_exec(bprm); >> + install_exec_creds(bprm); >> current->mm->end_code = ex.a_text + >> (current->mm->start_code = N_TXTADDR(ex)); >> @@ -174,7 +175,6 @@ static int load_aout_binary(struct linux_binprm * bprm) >> if (retval < 0) >> return retval; >> - install_exec_creds(bprm); >> if (N_MAGIC(ex) == OMAGIC) { >> unsigned long text_addr, map_size; >> diff --git a/fs/binfmt_elf_fdpic.c b/fs/binfmt_elf_fdpic.c >> index 240f66663543..6c94c6d53d97 100644 >> --- a/fs/binfmt_elf_fdpic.c >> +++ b/fs/binfmt_elf_fdpic.c >> @@ -353,6 +353,7 @@ static int load_elf_fdpic_binary(struct linux_binprm *bprm) >> current->personality |= READ_IMPLIES_EXEC; >> setup_new_exec(bprm); >> + install_exec_creds(bprm); >> set_binfmt(&elf_fdpic_format); >> @@ -434,7 +435,6 @@ static int load_elf_fdpic_binary(struct linux_binprm >> *bprm) >> current->mm->start_stack = current->mm->start_brk + stack_size; >> #endif >> - install_exec_creds(bprm); >> if (create_elf_fdpic_tables(bprm, current->mm, >> &exec_params, &interp_params) < 0) >> goto error; >> diff --git a/fs/binfmt_flat.c b/fs/binfmt_flat.c >> index 831a2b25ba79..1a1d1fcb893f 100644 >> --- a/fs/binfmt_flat.c >> +++ b/fs/binfmt_flat.c >> @@ -541,6 +541,7 @@ static int load_flat_file(struct linux_binprm *bprm, >> /* OK, This is the point of no return */ >> set_personality(PER_LINUX_32BIT); >> setup_new_exec(bprm); >> + install_exec_creds(bprm); >> } >> /* >> @@ -963,8 +964,6 @@ static int load_flat_binary(struct linux_binprm *bprm) >> } >> } >> - install_exec_creds(bprm); >> - >> set_binfmt(&flat_format); >> #ifdef CONFIG_MMU >>
diff --git a/arch/x86/ia32/ia32_aout.c b/arch/x86/ia32/ia32_aout.c index 9bb71abd66bd..37b36a8ce5fa 100644 --- a/arch/x86/ia32/ia32_aout.c +++ b/arch/x86/ia32/ia32_aout.c @@ -140,6 +140,7 @@ static int load_aout_binary(struct linux_binprm *bprm) set_personality_ia32(false); setup_new_exec(bprm); + install_exec_creds(bprm); regs->cs = __USER32_CS; regs->r8 = regs->r9 = regs->r10 = regs->r11 = regs->r12 = @@ -156,8 +157,6 @@ static int load_aout_binary(struct linux_binprm *bprm) if (retval < 0) return retval; - install_exec_creds(bprm); - if (N_MAGIC(ex) == OMAGIC) { unsigned long text_addr, map_size; diff --git a/fs/binfmt_aout.c b/fs/binfmt_aout.c index 8e8346a81723..ace587b66904 100644 --- a/fs/binfmt_aout.c +++ b/fs/binfmt_aout.c @@ -162,6 +162,7 @@ static int load_aout_binary(struct linux_binprm * bprm) set_personality(PER_LINUX); #endif setup_new_exec(bprm); + install_exec_creds(bprm); current->mm->end_code = ex.a_text + (current->mm->start_code = N_TXTADDR(ex)); @@ -174,7 +175,6 @@ static int load_aout_binary(struct linux_binprm * bprm) if (retval < 0) return retval; - install_exec_creds(bprm); if (N_MAGIC(ex) == OMAGIC) { unsigned long text_addr, map_size; diff --git a/fs/binfmt_elf_fdpic.c b/fs/binfmt_elf_fdpic.c index 240f66663543..6c94c6d53d97 100644 --- a/fs/binfmt_elf_fdpic.c +++ b/fs/binfmt_elf_fdpic.c @@ -353,6 +353,7 @@ static int load_elf_fdpic_binary(struct linux_binprm *bprm) current->personality |= READ_IMPLIES_EXEC; setup_new_exec(bprm); + install_exec_creds(bprm); set_binfmt(&elf_fdpic_format); @@ -434,7 +435,6 @@ static int load_elf_fdpic_binary(struct linux_binprm *bprm) current->mm->start_stack = current->mm->start_brk + stack_size; #endif - install_exec_creds(bprm); if (create_elf_fdpic_tables(bprm, current->mm, &exec_params, &interp_params) < 0) goto error; diff --git a/fs/binfmt_flat.c b/fs/binfmt_flat.c index 831a2b25ba79..1a1d1fcb893f 100644 --- a/fs/binfmt_flat.c +++ b/fs/binfmt_flat.c @@ -541,6 +541,7 @@ static int load_flat_file(struct linux_binprm *bprm, /* OK, This is the point of no return */ set_personality(PER_LINUX_32BIT); setup_new_exec(bprm); + install_exec_creds(bprm); } /* @@ -963,8 +964,6 @@ static int load_flat_binary(struct linux_binprm *bprm) } } - install_exec_creds(bprm); - set_binfmt(&flat_format); #ifdef CONFIG_MMU
In 2016 Linus moved install_exec_creds immediately after setup_new_exec, in binfmt_elf as a cleanup and as part of closing a potential information leak. Perform the same cleanup for the other binary formats. Different binary formats doing the same things the same way makes exec easier to reason about and easier to maintain. The binfmt_flagt bits were tested by Greg Ungerer <gerg@linux-m68k.org> Ref: 9f834ec18def ("binfmt_elf: switch to new creds when switching to new mm") Signed-off-by: "Eric W. Biederman" <ebiederm@xmission.com> --- arch/x86/ia32/ia32_aout.c | 3 +-- fs/binfmt_aout.c | 2 +- fs/binfmt_elf_fdpic.c | 2 +- fs/binfmt_flat.c | 3 +-- 4 files changed, 4 insertions(+), 6 deletions(-)