Re: binfmt_elf PT_INTERP gets EFAULT if no PROT_WRITE

[James Morris <jmorris@namei.org>]

On Tue, 8 Sep 2009, Roland McGrath wrote:

> Good catch, John.  But I prefer a different fix.  
> 
> I've reproduced the bug with the test case John included,
> and verified that my change fixes it too.

Applied to
git://git.kernel.org/pub/scm/linux/kernel/git/jmorris/security-testing-2.6#next

(cc stable).

> 
> 
> Thanks,
> Roland
> 
> ---
> [PATCH] binfmt_elf: fix PT_INTERP bss handling
> 
> In fs/binfmt_elf.c, load_elf_interp() calls padzero() for .bss even if
> the PT_LOAD has no PROT_WRITE and no .bss.  This generates EFAULT.
> 
> Here is a small test case.  (Yes, there are other, useful PT_INTERP
> which have only .text and no .data/.bss.)
> 
> 	----- ptinterp.S
> 	_start: .globl _start
> 		 nop
> 		 int3
> 	-----
> 	$ gcc -m32 -nostartfiles -nostdlib -o ptinterp ptinterp.S
> 	$ gcc -m32 -Wl,--dynamic-linker=ptinterp -o hello hello.c
> 	$ ./hello
> 	Segmentation fault  # during execve() itself
> 
> 	After applying the patch:
> 	$ ./hello
> 	Trace trap  # user-mode execution after execve() finishes
> 
> If the ELF headers are actually self-inconsistent, then dying is fine.
> But having no PROT_WRITE segment is perfectly normal and correct if
> there is no segment with p_memsz > p_filesz (i.e. bss).  John Reiser
> suggested checking for PROT_WRITE in the bss logic.  I think it makes
> most sense to simply apply the bss logic only when there is bss.
> 
> This patch looks less trivial than it is due to some reindentation.
> It just moves the "if (last_bss > elf_bss) {" test up to include the
> partial-page bss logic as well as the more-pages bss logic.
> 
> Reported-by: John Reiser <jreiser@bitwagon.com>
> Signed-off-by: Roland McGrath <roland@redhat.com>
> ---
>  fs/binfmt_elf.c |   28 ++++++++++++++--------------
>  1 files changed, 14 insertions(+), 14 deletions(-)
> 
> diff --git a/fs/binfmt_elf.c b/fs/binfmt_elf.c
> index b7c1603..7c1e65d 100644
> --- a/fs/binfmt_elf.c
> +++ b/fs/binfmt_elf.c
> @@ -501,22 +501,22 @@ static unsigned long load_elf_interp(struct elfhdr *interp_elf_ex,
>  		}
>  	}
>  
> -	/*
> -	 * 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_close;
> -	}
> +	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_close;
> +		}
>  
> -	/* What we have mapped so far */
> -	elf_bss = ELF_PAGESTART(elf_bss + ELF_MIN_ALIGN - 1);
> +		/* What we have mapped so far */
> +		elf_bss = ELF_PAGESTART(elf_bss + ELF_MIN_ALIGN - 1);
>  
> -	/* Map the last of the bss segment */
> -	if (last_bss > elf_bss) {
> +		/* Map the last of the bss segment */
>  		down_write(&current->mm->mmap_sem);
>  		error = do_brk(elf_bss, last_bss - elf_bss);
>  		up_write(&current->mm->mmap_sem);
> 

-- 
James Morris
<jmorris@namei.org>