From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1751185AbcFVWbw (ORCPT ); Wed, 22 Jun 2016 18:31:52 -0400 Received: from mail-wm0-f47.google.com ([74.125.82.47]:35828 "EHLO mail-wm0-f47.google.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1750740AbcFVWbu (ORCPT ); Wed, 22 Jun 2016 18:31:50 -0400 MIME-Version: 1.0 In-Reply-To: <1462963020-11097-1-git-send-email-hecmargi@upv.es> References: <1462963020-11097-1-git-send-email-hecmargi@upv.es> From: Kees Cook Date: Wed, 22 Jun 2016 15:31:46 -0700 X-Google-Sender-Auth: ARjKAwqvhhonxdqw0TU8D3aO4gk Message-ID: Subject: Re: [PATCH] Fix bss mapping for the interpreter in binfmt_elf To: Hector Marco-Gisbert Cc: LKML , "linux-fsdevel@vger.kernel.org" , Ismael Ripoll Ripoll , Alexander Viro , Andrew Morton Content-Type: text/plain; charset=UTF-8 Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org On Wed, May 11, 2016 at 3:37 AM, Hector Marco-Gisbert wrote: > 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: Heh, the bugs cancel each other out! :P > > > $ 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; > } Tangent: shouldn't this padzero() call happen even in the case where last_bss <= elf_bss? The elf_map call has total_size bumped up to page size, so junk may have been loaded from the file still, even if last_bss == elf_bss, etc. Maybe I'm missing something. > /* 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); I feel like do_brk should detect wrap-around and error out... > 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 > Acked-by: Ismael Ripoll Ripoll > --- > 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); This math is just ELF_PAGEALIGN(elf_bss)... > + last_bss = ELF_PAGESTART(last_bss + ELF_MIN_ALIGN - 1); I don't think this is correct, actually. It works for the less-than-a-single-page case, but not for the intended case. I don't like that this hides the PAGE_ALIGN() (on len) and PAGE_SHIFT (on addr) that happens inside do_brk either, and this will break if ELF_MIN_ALIGN != PAGE_SIZE. > > /* 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; > + } So, I think what's needed here are three patches: 1) move padzero() before the last_bss > elf_bss case, since the zero-filling of the ELF_PAGE should have nothing to do with the relationship of last_bss and elf_bss. AIUI, it's all about the relationship of elf_bss to the ELF_PAGE size. 2) handle the math on elf_bss vs last_bss correctly. I think this requires that both be ELF_PAGE aligned, since that's the expected granularity of the mappings. elf_bss already had alignment-based padding happen in, so the "start" of the vm_brk should be moved forward as done in the original code. However, since the "end" of the vm_brk will already be PAGE_ALIGNed then last_bss should get aligned here to avoid hiding it. Something like: elf_bss = ELF_PAGEALIGN(elf_bss); last_bss = ELF_PAGEALIGN(last_bss); And leave the vm_brk as-is since it already checks for size==0. 3) add code to do_brk() to check for these kinds of overflows, which shouldn't happen: static int do_brk(unsigned long addr, unsigned long request) { ... unsigned long len = PAGE_ALIGN(request); if (len < request) { WARN_ONCE(...); return -ENOMEM; } if (!len) return 0; ... How's that sound? -Kees -- Kees Cook Chrome OS & Brillo Security