Re: [mmotm:master 253/317] arch/mips/kernel/../../../fs/binfmt_elf.c:1140:7: error: 'elf_interpreter' undeclared; did you mean 'interpreter'?

[Kees Cook <keescook@chromium.org>]

On Wed, Apr 17, 2019 at 8:54 AM Kees Cook <keescook@chromium.org> wrote:
>
> On Wed, Apr 17, 2019 at 7:34 AM kbuild test robot <lkp@intel.com> wrote:
> >
> > tree:   git://git.cmpxchg.org/linux-mmotm.git master
> > head:   def6be39d5629b938faba788330db817d19a04da
> > commit: 8e5e08d49bf73afad16199d68c5e61a64f5df69d [253/317] fs/binfmt_elf.c: move brk out of mmap when doing direct loader exec
> > config: mips-fuloong2e_defconfig (attached as .config)
> > compiler: mips64el-linux-gnuabi64-gcc (Debian 7.2.0-11) 7.2.0
> > reproduce:
> >         wget https://raw.githubusercontent.com/intel/lkp-tests/master/sbin/make.cross -O ~/bin/make.cross
> >         chmod +x ~/bin/make.cross
> >         git checkout 8e5e08d49bf73afad16199d68c5e61a64f5df69d
> >         # save the attached .config to linux build tree
> >         GCC_VERSION=7.2.0 make.cross ARCH=mips
> >
> > All errors (new ones prefixed by >>):
> >
> >    In file included from arch/mips/kernel/binfmt_elfn32.c:106:0:
> >    arch/mips/kernel/../../../fs/binfmt_elf.c: In function 'load_elf_binary':
> > >> arch/mips/kernel/../../../fs/binfmt_elf.c:1140:7: error: 'elf_interpreter' undeclared (first use in this function); did you mean 'interpreter'?
> >      if (!elf_interpreter)
> >           ^~~~~~~~~~~~~~~
> >           interpreter
> >    arch/mips/kernel/../../../fs/binfmt_elf.c:1140:7: note: each undeclared identifier is reported only once for each function it appears in
>
> Whoa. That was unexpected (.c getting #included!)
>
> Especially since that's a local variable... I'll try to figure out
> what's happening...

I can't reproduce this on mips-linux-gnu-gcc (Ubuntu
7.3.0-27ubuntu1~18.04) 7.3.0

But I did notice that there is a kfree() _before_ other uses (though
none dereferencing) of elf_interpreter. Perhaps something in the
optimization pass removed the variable?

We could try this, which should likely be fixed regardless...

diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
index cdaa33f4a3ef..7682d47bd5f0 100644
--- a/fs/binfmt_elf.c
+++ b/fs/binfmt_elf.c
@@ -1100,7 +1100,6 @@ static int load_elf_binary(struct linux_binprm *bprm)

                allow_write_access(interpreter);
                fput(interpreter);
-               kfree(elf_interpreter);
        } else {
                elf_entry = loc->elf_ex.e_entry;
                if (BAD_ADDR(elf_entry)) {
@@ -1175,6 +1174,7 @@ static int load_elf_binary(struct linux_binprm *bprm)
        start_thread(regs, elf_entry, bprm->p);
        retval = 0;
 out:
+       kfree(elf_interpreter);
        kfree(loc);
 out_ret:
        return retval;


>
> -Kees
>
> >
> > vim +1140 arch/mips/kernel/../../../fs/binfmt_elf.c
> >
> >   1122
> >   1123          retval = create_elf_tables(bprm, &loc->elf_ex,
> >   1124                            load_addr, interp_load_addr);
> >   1125          if (retval < 0)
> >   1126                  goto out;
> >   1127          /* N.B. passed_fileno might not be initialized? */
> >   1128          current->mm->end_code = end_code;
> >   1129          current->mm->start_code = start_code;
> >   1130          current->mm->start_data = start_data;
> >   1131          current->mm->end_data = end_data;
> >   1132          current->mm->start_stack = bprm->p;
> >   1133
> >   1134          /*
> >   1135           * When executing a loader directly (ET_DYN without Interp), move
> >   1136           * the brk area out of the mmap region (since it grows up, and may
> >   1137           * collide early with the stack growing down), and into the unused
> >   1138           * ELF_ET_DYN_BASE region.
> >   1139           */
> > > 1140          if (!elf_interpreter)
> >   1141                  current->mm->brk = current->mm->start_brk = ELF_ET_DYN_BASE;
> >   1142
> >   1143          if ((current->flags & PF_RANDOMIZE) && (randomize_va_space > 1)) {
> >   1144                  current->mm->brk = current->mm->start_brk =
> >   1145                          arch_randomize_brk(current->mm);
> >   1146  #ifdef compat_brk_randomized
> >   1147                  current->brk_randomized = 1;
> >   1148  #endif
> >   1149          }
> >   1150
> >   1151          if (current->personality & MMAP_PAGE_ZERO) {
> >   1152                  /* Why this, you ask???  Well SVr4 maps page 0 as read-only,
> >   1153                     and some applications "depend" upon this behavior.
> >   1154                     Since we do not have the power to recompile these, we
> >   1155                     emulate the SVr4 behavior. Sigh. */
> >   1156                  error = vm_mmap(NULL, 0, PAGE_SIZE, PROT_READ | PROT_EXEC,
> >   1157                                  MAP_FIXED | MAP_PRIVATE, 0);
> >   1158          }
> >   1159
> >
> > ---
> > 0-DAY kernel test infrastructure                Open Source Technology Center
> > https://lists.01.org/pipermail/kbuild-all                   Intel Corporation
>
>
>
> --
> Kees Cook



-- 
Kees Cook