Re: MAP_FIXED for ELF mappings

[Michal Hocko]

Dohh, screwed up From. Sorry for spamming.

On Wed 04-10-17 09:50:59, Michal Hocko wrote:
> Hi,
> while studying CVE-2017-1000253 and the MAP_FIXED usage in load_elf*
> code paths I have stumbled over MAP_FIXED usage for elf segments
> mapping. I am not really familiar with this area much so I might draw
> completely incorrect conclusions here but I am really wondering why we
> are doing MAP_FIXED there at all.
> 
> I can see why some segments really have to be mapped at a specific
> address but I wonder whether MAP_FIXED is the right tool to achieve
> that. It seems to me that MAP_FIXED is fundamentally dangerous because
> it unmaps any existing mapping. I assume that nothing should be really
> mapped in the requested range that early so we can only stumble over
> something when the address space randomization place things unexpectedly
> (which was the case of the above mentioned CVE AFAIU).
> 
> So my primary question is whether we can/should simply drop MAP_FIXED
> from elf_map at all. Instead we should test whether the mapping was
> successful for the requested address and fail otherwise. I realize that
> failing due to something that a user has no idea about sucks a lot but
> it seems to me safer to simply complain into the log and fail is a safer
> option.
> 
> Something like the following completely untested diff. Or am I
> completely missing the point of the MAP_FIXED purpose?
> ---
> 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);
>  	} 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)),
> -- 
> Michal Hocko
> SUSE Labs

-- 
Michal Hocko
SUSE Labs