Re: [PATCH 1/2] fs, elf: drop MAP_FIXED usage from elf_map

[Michal Hocko]

[CCing metag people for the metag elf_map implementation specific. The thread
starts here http://lkml.kernel.org/r/20171016134446.19910-1-mhocko@xxxxxxxxxx]

On Mon 16-10-17 09:39:14, Kees Cook wrote:
> On Mon, Oct 16, 2017 at 6:44 AM, Michal Hocko <mhocko@xxxxxxxxxx> wrote:
[...]
> > +static unsigned long elf_vm_mmap(struct file *filep, unsigned long addr,
> > +               unsigned long size, int prot, int type, unsigned long off)
> > +{
> > +       unsigned long map_addr;
> > +
> > +       /*
> > +        * If caller requests the mapping at a specific place, make sure we fail
> > +        * rather than potentially clobber an existing mapping which can have
> > +        * security consequences (e.g. smash over the stack area).
> > +        */
> > +       map_addr = vm_mmap(filep, addr, size, prot, type & ~MAP_FIXED, off);
> > +       if (BAD_ADDR(map_addr))
> > +               return map_addr;
> > +
> > +       if ((type & MAP_FIXED) && map_addr != addr) {
> > +               pr_info("Uhuuh, elf segement at %p requested but the memory is mapped already\n",
> > +                               (void*)addr);
> 
> Is "info" loud enough? I actually think this should be a WARN_ONCE().

Does it make any sense to taint the kernel just because of this
condition? I can make it pr_error...

> > +               return -EAGAIN;
> > +       }
> > +
> > +       return map_addr;
> > +}
> > +
> >  static unsigned long elf_map(struct file *filep, unsigned long addr,
> >                 struct elf_phdr *eppnt, int prot, int type,
> >                 unsigned long total_size)
> > @@ -366,11 +389,11 @@ static unsigned long elf_map(struct file *filep, unsigned long addr,
> 
> elf_map is redirected on metag -- it should probably have its vm_mmap
> calls adjust too.

Thanks for spotting this. I am not really familiar with metag. It seems
to clear MAP_FIXED already
	tcm_tag = tcm_lookup_tag(addr);

	if (tcm_tag != TCM_INVALID_TAG)
		type &= ~MAP_FIXED;

So if there is a tag the flag is cleared. I do not understand this code
(and git log doesn't help) but why is this MAP_FIXED code really needed?

> >         */
> >         if (total_size) {
> >                 total_size = ELF_PAGEALIGN(total_size);
> > -               map_addr = vm_mmap(filep, addr, total_size, prot, type, off);
> > +               map_addr = elf_vm_mmap(filep, addr, total_size, prot, type, off);
> >                 if (!BAD_ADDR(map_addr))
> >                         vm_munmap(map_addr+size, total_size-size);
> >         } else
> > -               map_addr = vm_mmap(filep, addr, size, prot, type, off);
> > +               map_addr = elf_vm_mmap(filep, addr, size, prot, type, off);
> >
> >         return(map_addr);
> >  }
> > @@ -1215,7 +1238,7 @@ static int load_elf_library(struct file *file)
> >                 eppnt++;
> >
> >         /* Now use mmap to map the library into memory. */
> > -       error = vm_mmap(file,
> > +       error = elf_vm_mmap(file,
> >                         ELF_PAGESTART(eppnt->p_vaddr),
> >                         (eppnt->p_filesz +
> >                          ELF_PAGEOFFSET(eppnt->p_vaddr)),
> > --
> > 2.14.2
> >
> 
> Otherwise, yeah, this should be good.

Thanks!
-- 
Michal Hocko
SUSE Labs