+ elf-loader-crash-while-zero-filling-bss.patch added to -mm tree

+ elf-loader-crash-while-zero-filling-bss.patch added to -mm tree

[akpm]

The patch titled
     Elf loader crash while zero-filling .bss
has been added to the -mm tree.  Its filename is
     elf-loader-crash-while-zero-filling-bss.patch

Before you just go and hit "reply", please:
   a) Consider who else should be cc'ed
   b) Prefer to cc a suitable mailing list as well
   c) Ideally: find the original patch on the mailing list and do a
      reply-to-all to that, adding suitable additional cc's

*** Remember to use Documentation/SubmitChecklist when testing your code ***

See http://www.zip.com.au/~akpm/linux/patches/stuff/added-to-mm.txt to find
out what to do about this

The current -mm tree may be found at http://userweb.kernel.org/~akpm/mmotm/

------------------------------------------------------
Subject: Elf loader crash while zero-filling .bss
From: "Abel Bernabeu" <abelbg@m2grp.com>

I've finally found a solution for the crash in load_binary_elf I
reported last week:

http://lkml.org/lkml/2008/1/30/171

The attached patch solves my problem.

set_brk(start, end) allocs just page aligned regions (by "collapsing" both
extremes to the start of the page in which they lay)...  That means than
even if both pointers are not equal there are still some chances that
set_brk has allocated no space at all because ELF_PAGEALIGN(elf_bss) ==	
ELF_PAGEALIGN(elf_brk).

So the condition was not correct.

Cc: Roland McGrath <roland@redhat.com>
Cc: Daniel Jacobowitz <dan@debian.org>
Cc: Jiri Kosina <jkosina@suse.cz>
Cc: "H. Peter Anvin" <hpa@zytor.com>
Cc: Andreas Schwab <schwab@suse.de>
Cc: <stable@kernel.org>
Signed-off-by: Andrew Morton <akpm@linux-foundation.org>
---

 fs/binfmt_elf.c |    3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff -puN fs/binfmt_elf.c~elf-loader-crash-while-zero-filling-bss fs/binfmt_elf.c
--- a/fs/binfmt_elf.c~elf-loader-crash-while-zero-filling-bss
+++ a/fs/binfmt_elf.c
@@ -893,7 +893,8 @@ static int load_elf_binary(struct linux_
 		send_sig(SIGKILL, current, 0);
 		goto out_free_dentry;
 	}
-	if (likely(elf_bss != elf_brk) && unlikely(padzero(elf_bss))) {
+	if (likely(ELF_PAGEALIGN(elf_bss) != ELF_PAGEALIGN(elf_brk)) &&
+			unlikely(padzero(elf_bss))) {
 		send_sig(SIGSEGV, current, 0);
 		retval = -EFAULT; /* Nobody gets to see this, but.. */
 		goto out_free_dentry;
_

Patches currently in -mm which might be from abelbg@m2grp.com are

elf-loader-crash-while-zero-filling-bss.patch

Re: + elf-loader-crash-while-zero-filling-bss.patch added to -mm tree

[Daniel Jacobowitz]

On Wed, Feb 13, 2008 at 12:15:06AM -0800, akpm@linux-foundation.org wrote:
> Subject: Elf loader crash while zero-filling .bss
> From: "Abel Bernabeu" <abelbg@m2grp.com>
> 
> I've finally found a solution for the crash in load_binary_elf I
> reported last week:
> 
> http://lkml.org/lkml/2008/1/30/171
> 
> The attached patch solves my problem.
> 
> set_brk(start, end) allocs just page aligned regions (by "collapsing" both
> extremes to the start of the page in which they lay)...  That means than
> even if both pointers are not equal there are still some chances that
> set_brk has allocated no space at all because ELF_PAGEALIGN(elf_bss) ==	
> ELF_PAGEALIGN(elf_brk).
> 
> So the condition was not correct.

This patch is wrong.

ELF_PAGEALIGN rounds up to the end of the page, not down to the start
of the page.  If elf_bss is in the middle of a page, set_brk allocates
any additional pages after the one already allocated.  elf_bss is the
start of the area that needs to be zero initialized, elf_brk is its
end.  So if elf_bss != elf_brk then there's garbage mapped in BSS
from the file and if you don't clear it some of your zero-initialized
variables won't be zero initialized at all.

In the linked message, set_brk is passed elf_bss so its actual
arguments are set_brk (0xa3801, 0x000a4ec8).  It should map one
page.  0xa3801 should be an already mapped page, and clear_user should
succeed in clearing it.

-- 
Daniel Jacobowitz
CodeSourcery

Re: + elf-loader-crash-while-zero-filling-bss.patch added to -mm tree

[Abel Bernabeu]

2008/2/13, Daniel Jacobowitz <dan@debian.org>:
> On Wed, Feb 13, 2008 at 12:15:06AM -0800, akpm@linux-foundation.org wrote:
>  > Subject: Elf loader crash while zero-filling .bss
>  > From: "Abel Bernabeu" <abelbg@m2grp.com>
>  >
>  > I've finally found a solution for the crash in load_binary_elf I
>  > reported last week:
>  >
>  > http://lkml.org/lkml/2008/1/30/171
>  >
>  > The attached patch solves my problem.
>  >
>  > set_brk(start, end) allocs just page aligned regions (by "collapsing" both
>  > extremes to the start of the page in which they lay)...  That means than
>  > even if both pointers are not equal there are still some chances that
>  > set_brk has allocated no space at all because ELF_PAGEALIGN(elf_bss) ==
>  > ELF_PAGEALIGN(elf_brk).
>  >
>  > So the condition was not correct.
>
>  This patch is wrong.
>
>  ELF_PAGEALIGN rounds up to the end of the page, not down to the start
>  of the page.  If elf_bss is in the middle of a page, set_brk allocates
>  any additional pages after the one already allocated.  elf_bss is the
>  start of the area that needs to be zero initialized, elf_brk is its
>  end.  So if elf_bss != elf_brk then there's garbage mapped in BSS
>  from the file and if you don't clear it some of your zero-initialized
>  variables won't be zero initialized at all.
>
>  In the linked message, set_brk is passed elf_bss so its actual
>  arguments are set_brk (0xa3801, 0x000a4ec8).  It should map one
>  page.  0xa3801 should be an already mapped page, and clear_user should
>  succeed in clearing it.

The bad news... Dan is rigth and I've made you loss your time because
of my initial missunderstanding of that piece of code.

Having your boss and the client continously asking if does Linux
already run on our board doesn't help to keep your mind clearness X)

The call to clear_user produced a page fault (it's true), but it was a
benign one (the some sort of page fault with permits load-on-demand of
the binary).

Then Rusell King's page fault handler
(arch/arm/mm/fault.c:do_DataAbort) manages to finally call the correct
non-architecture specific handler for load-on-demand.

This non-architecture specific handler allocs a physical page in which
clear_user can retry to write at a second try when the program
execution is restarted.

The "funny" thing is that now I am almost sure that I was playing with
Rusell's code and I accidentally left commented some of the code that
finally  calls this non architecture specific code.

The good news for "the community" is that now I am very sure I can
refactor the overpatched (but correct) portion of code I
missunderstood and send it back to you when is already tested. There
are some easy clean ups I can do in order to make Adrian Bunk happy :D

Yours, Abel.