Re: [PATCH] Fix bss mapping for the interpreter in binfmt_elf

[Hector Marco-Gisbert <hecmargi@upv.es>]

Second try ...

Although it does not seem dangerous, I think it worth to fix this to avoid
potential future problems.

any thoughts on this ?


Thanks,
Hector.


> While working on a new ASLR for userspace we detected an error in the
> interpret loader.
> 
> The size of the bss section for some interpreters is not correctly
> calculated resulting in unnecessary calls to vm_brk() with enormous size
> values.
> 
> The bug appears when loading some interpreters with a small bss size. Once
> the last loadable segment has been loaded, the bss section is zeroed up to
> the page boundary and the elf_bss variable is updated to this new page
> boundary.  Because of this update (alignment), the last_bss could be less
> than elf_bss and the subtraction "last_bss - elf_bss" value could overflow.
> 
> Although it is quite easy to check this error, it has not been manifested
> because some peculiarities of the bug. Following is a brief description:
> 
> 
> $ size /lib32/ld-2.19.so 
>    text    data     bss     dec     hex filename
>  128456    2964     192  131612   2021c /lib32/ld-2.19.so
> 
> 
> An execution example with:
>   - last_bss: 0xb7794938
>   - elf_bss:  0xb7794878
> 
>         
> From fs/binfmt_elf.c:
> ---------------------------------------------------------------------------
>      if (last_bss > elf_bss) { 
>              /*
>               * Now fill out the bss section.  First pad the last page up
>               * to the page boundary, and then perform a mmap to make sure
>               * that there are zero-mapped pages up to and including the
>               * last bss page.
>               */
>              if (padzero(elf_bss)) {
>                      error = -EFAULT;
>                      goto out;
>              }
> 
>              /* What we have mapped so far */
>              elf_bss = ELF_PAGESTART(elf_bss + ELF_MIN_ALIGN - 1);
> 
>              <---------- elf_bss here is 0xb7795000
> 
>              /* Map the last of the bss segment */
>              error = vm_brk(elf_bss, last_bss - elf_bss); <-- overflow!
>              if (BAD_ADDR(error))
>                      goto out;
>      }
> 
>      error = load_addr;
> out:
>      return error;
> }
> ---------------------------------------------------------------------------
> 
> 
> The size value requested to the vm_brk() call (last_bss - elf_bss) is
> 0xfffffffffffff938 and internally this size is page aligned in the do_brk()
> function resulting in a 0 length request.
> 
> static unsigned long do_brk(unsigned long addr, unsigned long len)
> {
>         ...
>         len = PAGE_ALIGN(len);
>         if (!len)
>                 return addr;
> 
> 
> Since a segment of 0 bytes is perfectly valid, it returns the requested
> address to vm_brk() and because it is page aligned (done by the previous
> line to the vm_brk() call the "error" is not detected by
> "BAD_ADDR(error)" and the "load_elf_interp()" functions does not
> returns any error. Note that vm_brk() is not necessary at all.
> 
> In brief, if the end of the bss is in the same page than the last segment
> loaded then the size of the last of bss segment is incorrectly calculated.
> 
> 
> This patch set up to the page boundary of the last_bss variable and only do
> the vm_brk() call when necessary.
> 
> 
> Signed-off-by: Hector Marco-Gisbert <hecmargi@upv.es>
> Acked-by: Ismael Ripoll Ripoll <iripoll@upv.es>
> ---
>  fs/binfmt_elf.c | 12 ++++++++----
>  1 file changed, 8 insertions(+), 4 deletions(-)
> 
> diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
> index 81381cc..acfbdc2 100644
> --- a/fs/binfmt_elf.c
> +++ b/fs/binfmt_elf.c
> @@ -526,7 +526,7 @@ static unsigned long load_elf_interp(struct elfhdr *interp_elf_ex,
>  	int load_addr_set = 0;
>  	unsigned long last_bss = 0, elf_bss = 0;
>  	unsigned long error = ~0UL;
> -	unsigned long total_size;
> +	unsigned long total_size, size;
>  	int i;
>  
>  	/* First of all, some simple consistency checks */
> @@ -626,11 +626,15 @@ static unsigned long load_elf_interp(struct elfhdr *interp_elf_ex,
>  
>  		/* What we have mapped so far */
>  		elf_bss = ELF_PAGESTART(elf_bss + ELF_MIN_ALIGN - 1);
> +		last_bss = ELF_PAGESTART(last_bss + ELF_MIN_ALIGN - 1);
>  
>  		/* Map the last of the bss segment */
> -		error = vm_brk(elf_bss, last_bss - elf_bss);
> -		if (BAD_ADDR(error))
> -			goto out;
> +		size = last_bss - elf_bss;
> +		if (size) {
> +			error = vm_brk(elf_bss, size);
> +			if (BAD_ADDR(error))
> +				goto out;
> +		}
>  	}
>  
>  	error = load_addr;
> 

-- 
Dr. Hector Marco-Gisbert @ http://hmarco.org/
Cyber Security Researcher @ http://cybersecurity.upv.es
Universitat Politècnica de València (Spain)