linux-kernel - [PATCH] Fix bss mapping for the interpreter in binfmt

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [thread-next>] [day] [month] [year] [list]

Message-Id: <1462963020-11097-1-git-send-email-hecmargi@upv.es>
Date:	Wed, 11 May 2016 12:37:00 +0200
From:	Hector Marco-Gisbert <hecmargi@....es>
To:	linux-kernel@...r.kernel.org
Cc:	linux-fsdevel@...r.kernel.org,
	Alexander Viro <viro@...iv.linux.org.uk>,
	kees Cook <keescook@...omium.org>,
	Ismael Ripoll Ripoll <iripoll@....es>,
	Hector Marco-Gisbert <hecmargi@....es>
Subject: [PATCH] Fix bss mapping for the interpreter in binfmt_elf

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@....es>
Acked-by: Ismael Ripoll Ripoll <iripoll@....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;
-- 
1.9.1