Re: MAP_FIXED for ELF mappings

[Baoquan He <bhe@redhat.com>]

On 10/04/17 at 05:17pm, Michal Hocko wrote:
> On Wed 04-10-17 23:12:38, Baoquan He wrote:
> > I made a clean up patch according to Oleg's suggestion. It's trying to
> > get an map area to cover total_size, then do mmap for for the 1st
> > program segment only. Not sure if this way is correct.
> > 
> > >From 40f231bb78a74caebcb4a898089a9fa5323be05f Mon Sep 17 00:00:00 2001
> > From: Baoquan He <bhe@redhat.com>
> > Date: Fri, 29 Sep 2017 21:35:30 +0800
> > Subject: [PATCH] binfmt_elf: Clean up the elf_map
> > 
> > Oleg pointed out that it's really ugly to do mmap of the total_size, then
> > unmap the region excluding the 1st segment. The right way should be search
> > an unmapped area which can cover region of total_size, then map the 1st
> > segment only.
> > 
> > And also update the code comment accordingly. In below commit, the relevant
> > code comment is not changed to cover the ELF binary image case.
> > commit a87938b2e2 ("fs/binfmt_elf.c: fix bug in loading of PIE binaries")
> > 
> > Signed-off-by: Baoquan He <bhe@redhat.com>
> > ---
> >  fs/binfmt_elf.c | 27 +++++++++++++++------------
> >  1 file changed, 15 insertions(+), 12 deletions(-)
> > 
> > diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
> > index 72b7ecba7ead..43a47b2aa3f6 100644
> > --- a/fs/binfmt_elf.c
> > +++ b/fs/binfmt_elf.c
> > @@ -357,22 +357,25 @@ static unsigned long elf_map(struct file *filep, unsigned long addr,
> >  		return addr;
> >  
> >  	/*
> > -	* total_size is the size of the ELF (interpreter) image.
> > -	* The _first_ mmap needs to know the full size, otherwise
> > -	* randomization might put this image into an overlapping
> > -	* position with the ELF binary image. (since size < total_size)
> > -	* So we first map the 'big' image - and unmap the remainder at
> > -	* the end. (which unmap is needed for ELF images with holes.)
> > +	* total_size is the size of the ELF binary image or the ELF loader
> > +	* image. For loader image, the _first_ mmap needs to know the full
> > +	* size, otherwise randomization might put image into an overlapping
> > +	* position with the ELF binary image.(since size < total_size)
> > +	* So we use total_size to get an area to cover the whole loader image,
> > +	* then map the 1st progment segment only with its own size. For binary
> > +	* image, similarly, the _first_ mmap also needs to know the full size,
> > +	* otherwise randomization might put image above mm->mmap_base.

Oh, no, here, it won't include the PIE binary case. Since it must be
from ELF_ET_DYN_BASE. Here should be the "ld.so program" case.

> >  	*/
> >  	if (total_size) {
> >  		total_size = ELF_PAGEALIGN(total_size);
> > -		map_addr = vm_mmap(filep, addr, total_size, prot, flags, off);
> > -		if (!BAD_ADDR(map_addr))
> > -			vm_munmap(map_addr+size, total_size-size);
> > -	} else
> > -		map_addr = vm_mmap(filep, addr, size, prot, flags, off);
> > +		addr = get_unmapped_area(file, addr, total_size, off, flags);
> 
> So how does this prevent clobbering an existing VMA when flags contains
> MAP_FIXED?

Earlier flush_old_exec() is called to clean all old VMAs. Here if
total_size is non-zero only if it's the 1st prgoram segment of dynamic
loader, either from load_elf_interp() or the "ld.so program" case. With
my understanding, it can't be meeting an existing VMA. Not sure if
it's correct. Currently, it haven't passed to ld.so to link other .so.

> 
> > +		if (offset_in_page(addr))
> > +			return addr;
> > +	}
> > +
> > +	map_addr = vm_mmap(filep, addr, size, prot, flags, off);
> >  
> > -	return(map_addr);
> > +	return (map_addr);
> >  }
> >  
> >  #endif /* !elf_map */
> > -- 
> > 2.5.5
> 
> -- 
> Michal Hocko
> SUSE Labs