From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: (majordomo@vger.kernel.org) by vger.kernel.org via listexpand id S1752671AbcFVQkx (ORCPT ); Wed, 22 Jun 2016 12:40:53 -0400 Received: from smtpsalv.cc.upv.es ([158.42.249.11]:39048 "EHLO smtpsalv.upv.es" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751848AbcFVQkv (ORCPT ); Wed, 22 Jun 2016 12:40:51 -0400 X-Greylist: delayed 722 seconds by postgrey-1.27 at vger.kernel.org; Wed, 22 Jun 2016 12:40:50 EDT Subject: Re: [PATCH] Fix bss mapping for the interpreter in binfmt_elf To: linux-kernel@vger.kernel.org References: <1462963020-11097-1-git-send-email-hecmargi@upv.es> Cc: linux-fsdevel@vger.kernel.org, Alexander Viro , kees Cook , Ismael Ripoll Ripoll From: Hector Marco-Gisbert X-Enigmail-Draft-Status: N1110 Message-ID: <576ABCB9.70900@upv.es> Date: Wed, 22 Jun 2016 18:28:41 +0200 User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:38.0) Gecko/20100101 Thunderbird/38.8.0 MIME-Version: 1.0 In-Reply-To: <1462963020-11097-1-git-send-email-hecmargi@upv.es> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit Sender: linux-kernel-owner@vger.kernel.org List-ID: X-Mailing-List: linux-kernel@vger.kernel.org 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 > 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); > + 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) From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from smtpsalv.cc.upv.es ([158.42.249.11]:39048 "EHLO smtpsalv.upv.es" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751848AbcFVQkv (ORCPT ); Wed, 22 Jun 2016 12:40:51 -0400 Subject: Re: [PATCH] Fix bss mapping for the interpreter in binfmt_elf To: linux-kernel@vger.kernel.org References: <1462963020-11097-1-git-send-email-hecmargi@upv.es> Cc: linux-fsdevel@vger.kernel.org, Alexander Viro , kees Cook , Ismael Ripoll Ripoll From: Hector Marco-Gisbert Message-ID: <576ABCB9.70900@upv.es> Date: Wed, 22 Jun 2016 18:28:41 +0200 MIME-Version: 1.0 In-Reply-To: <1462963020-11097-1-git-send-email-hecmargi@upv.es> Content-Type: text/plain; charset=windows-1252 Content-Transfer-Encoding: 8bit Sender: linux-fsdevel-owner@vger.kernel.org List-ID: 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 > 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); > + 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)