linux-kernel - Re: [PATCH 1/2] fs, elf: drop MAP_FIXED usage from elf

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 linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Date:   Mon, 16 Oct 2017 21:00:47 +0200
From:   Michal Hocko <mhocko@...nel.org>
To:     Kees Cook <keescook@...omium.org>
Cc:     LKML <linux-kernel@...r.kernel.org>,
        Linus Torvalds <torvalds@...ux-foundation.org>,
        Jiri Kosina <jkosina@...e.cz>,
        Al Viro <viro@...iv.linux.org.uk>,
        Oleg Nesterov <oleg@...hat.com>,
        Ingo Molnar <mingo@...hat.com>, Baoquan He <bhe@...hat.com>,
        James Hogan <james.hogan@...tec.com>,
        linux-metag@...r.kernel.org
Subject: Re: [PATCH 1/2] fs, elf: drop MAP_FIXED usage from elf_map

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

On Mon 16-10-17 09:39:14, Kees Cook wrote:
> On Mon, Oct 16, 2017 at 6:44 AM, Michal Hocko <mhocko@...nel.org> 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