Re: MAP_FIXED for ELF mappings

[Baoquan He <bhe@redhat.com>]

On 10/04/17 at 09:50am, 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.

Sorry to interrupt. I tried below example.c and example2.c files and
compile and link them with fpie and pie. Seems in the final PIE
executable, the local global is pc relative. Means the data segment has
to be put after the code segment though PIE program. Then MAP_FIXED is a
good to have flag, especially we have counted in the total_size to
search an area to cover the whole dynamic program and get the load_bias.
It's no way to fail to get map agrea in case load_addr_set == 1.
So with MAP_FIXED set, we won't take time to search the mm vma rb
tree when load_addr_set == 1, but just return the specified addr directly,
looks more efficient.

########### example2.c
int local_global_var=0x10;
int local_global_func(void)
{
        return 0x40;
}
########## example.c
#include <stdio.h>  
extern int local_global_var;
extern int local_global_func(void);

int main(void)  
{  
        int x = local_global_func();  
        local_global_var= 0x20;  
 
        while(getchar()!= 'q')  
                continue;  
        return 0;  
}

$gcc -fpie -c example.c example2.c
$ls example*.o
example.o example2.o
$objdump -d -r example.o
...
0000000000000000 <main>:
   0:   55                      push   %rbp
   1:   48 89 e5                mov    %rsp,%rbp
   4:   48 83 ec 10             sub    $0x10,%rsp
   8:   e8 00 00 00 00          callq  d <main+0xd>
                        9: R_X86_64_PLT32       local_global_func-0x4
   d:   89 45 fc                mov    %eax,-0x4(%rbp)
  10:   c7 05 00 00 00 00 20    movl   $0x20,0x0(%rip)        # 1a
<main+0x1a>
  17:   00 00 00 
                        12: R_X86_64_PC32       local_global_var-0x8
  1a:   eb 01                   jmp    1d <main+0x1d>
  1c:   90                      nop
  1d:   e8 00 00 00 00          callq  22 <main+0x22>
                        1e: R_X86_64_PLT32      getchar-0x4
  22:   83 f8 71                cmp    $0x71,%eax
  25:   75 f5                   jne    1c <main+0x1c>
  27:   b8 00 00 00 00          mov    $0x0,%eax
  2c:   c9                      leaveq 
  2d:   c3                      retq
...
$gcc -pie -o test example.o example2.o
$objdump -d -r test
...
0000000000000655 <main>:
 655:   55                      push   %rbp
 656:   48 89 e5                mov    %rsp,%rbp
 659:   48 83 ec 10             sub    $0x10,%rsp
 65d:   e8 e8 ff ff ff          callq  64a <local_global_func>
 662:   89 45 fc                mov    %eax,-0x4(%rbp)
 665:   c7 05 b5 09 20 00 20    movl   $0x20,0x2009b5(%rip)
#201024 <local_global_var>
 66c:   00 00 00 
 66f:   eb 01                   jmp    672 <main+0x1d>
 671:   90                      nop
 672:   e8 a9 fe ff ff          callq  520 <getchar@plt>
 677:   83 f8 71                cmp    $0x71,%eax
 67a:   75 f5                   jne    671 <main+0x1c>
 67c:   b8 00 00 00 00          mov    $0x0,%eax
 681:   c9                      leaveq 
 682:   c3                      retq   
 683:   66 2e 0f 1f 84 00 00    nopw   %cs:0x0(%rax,%rax,1)
 68a:   00 00 00 
 68d:   0f 1f 00                nopl   (%rax)
...
 

> 
> 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