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

[Baoquan He <bhe@redhat.com>]

On 10/17/17 at 02:56pm, Michal Hocko wrote:
> On Tue 17-10-17 20:26:14, Baoquan He wrote:
> > Hi Michal,
> > 
> > Earlier I posted a patchset to clean up these in a different way, but
> > patches sent to your below mail address are all rejected.
> > 
> > <mhocko@kerne.org>: host mail.kerne.org[104.131.33.237] said: 454 4.1.1
> >     <mhocko@kerne.org>: Recipient address rejected: User unknown in virtual
> >     mailbox table (in reply to RCPT TO command)
> 
> yes, there was a typo in the email address which I've fixed up in the
> reply... Your cleanup was mostly about replacing vm_mmap by
> get_unmaped_area which is an independent issue to what I am trying to
> achieve here.

Oops. Yes, I must have typed your address by hand, but not copying
from the previous discussion thread. Sorry for that.

> 
> > On 10/16/17 at 03:44pm, Michal Hocko wrote:
> > > From: Michal Hocko <mhocko@suse.com>
> > > diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
> > > index 6466153f2bf0..09456e2add18 100644
> > > --- a/fs/binfmt_elf.c
> > > +++ b/fs/binfmt_elf.c
> > > @@ -341,6 +341,29 @@ create_elf_tables(struct linux_binprm *bprm, struct elfhdr *exec,
> > >  
> > >  #ifndef elf_map
> > >  
> > > +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);
> > > +		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,
> > >  	*/
> > >  	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);
> > 
> > Here we will still take the map total, then unmap the rest way.
> > 
> > I am wondering why we don't fix those issues we figured out, but add 
> > another level of wrapper elf_vm_mmap() to hack it.
> 
> Could you be more specific please?

In the current code, obviously the total size for PIE loading is
unnecessary since it's MAP_FIXED, and it's initial mapping from
ELF_ET_DYN_BASE, I can't see any chance it will fail to map. Though you
use a trick in this new elf_vm_mmap() to check if it failed on mapping,
in essence it's a MAP_FIXED.

And as I said before, then we will still have the
ungraceful 'mapping total'|'unmapping the rest' method.

If from a new code reader's point of view, the newly added elf_vm_mmap()
plus the existing elf_map() may confuse people more.

Just personnal opinion. Anyway, your patches are better, my patches have
been resting there for several days but no comment. :-) I may still need
strength the understanding about elf loading code.

Thanks
Baoquan

> 
> > So we will have
> > elf_map() -> elf_vm_mmap() -> vm_mmap(), not even counting
> > vm_mmap_pgoff(), then finally enter into do_mmap_pgoff(), to do the
> > maping for elf program.
> 
> I've added another level of helper to keep the code in elf_map saner. It
> is quite complex already.
> -- 
> Michal Hocko
> SUSE Labs