[Patch] Elf loader crash while zero-filling .bss

[Patch] Elf loader crash while zero-filling .bss

[Abel Bernabeu]

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, but please test it yourself...

set_brk(start, end) allocs just page aligned regions (by "colapsing"
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.


--- linux-2.6.22.10.orig/fs/binfmt_elf.c	2008-02-11 18:58:29.000000000 +0100
+++ linux-2.6.22.10.hacked/fs/binfmt_elf.c	2008-02-11 16:09:41.000000000 +0100
@@ -939,3 +939,3 @@ static int load_elf_binary(struct linux_
 	}
-	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);

Re: [Patch] Elf loader crash while zero-filling .bss

[Abel Bernabeu]

2008/2/11, 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, but please test it yourself...
>
> set_brk(start, end) allocs just page aligned regions (by "colapsing"
> 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).

Sorry this was an errata in my comment:  no space is allocated at all
because ELF_PAGEALIGN(elf_bss) == ELF_PAGEALIGN(elf_brk)

Re: [Patch] Elf loader crash while zero-filling .bss

[Jiri Kosina]

On Mon, 11 Feb 2008, Abel Bernabeu wrote:

> > set_brk(start, end) allocs just page aligned regions (by "colapsing"
> > 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).
> Sorry this was an errata in my comment:  no space is allocated at all
> because ELF_PAGEALIGN(elf_bss) == ELF_PAGEALIGN(elf_brk)

I also don't fully understand the "colapsing" part ... ELF_PAGEALIGN() 
really aligns the address to the nearest page boundary at the higher 
address ... so I don't understand the comment about colapsing to the start 
of the page that contains the address, it's seems to me that the real 
behavior is quite the opposite.

-- 
Jiri Kosina
SUSE Labs

Re: [Patch] Elf loader crash while zero-filling .bss

[Jiri Kosina]

On Mon, 11 Feb 2008, Abel Bernabeu wrote:

> In such a way that set_brk(0x0, 0x100) does not alloc any space at all. 
> There are just more ways to get no memory allocation than 
> set_brk(elf_bss, elf_bss) (the equalness condition i've changed).
> Sorry, the correct description for the patch may be:
> set_brk(start, end) allocs just page aligned regions (by "colapsing"
> both extremes to the END 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 because
> ELF_PAGEALIGN(elf_bss) == ELF_PAGEALIGN(elf_brk)

Now, the question is whether it is valid for ELF binary to not have the 
end of .bss section (if present at all) not page-aligned.

-- 
Jiri Kosina
SUSE Labs

Re: [Patch] Elf loader crash while zero-filling .bss

[H. Peter Anvin]

Jiri Kosina wrote:
> On Mon, 11 Feb 2008, Abel Bernabeu wrote:
> 
>> In such a way that set_brk(0x0, 0x100) does not alloc any space at all. 
>> There are just more ways to get no memory allocation than 
>> set_brk(elf_bss, elf_bss) (the equalness condition i've changed).
>> Sorry, the correct description for the patch may be:
>> set_brk(start, end) allocs just page aligned regions (by "colapsing"
>> both extremes to the END 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 because
>> ELF_PAGEALIGN(elf_bss) == ELF_PAGEALIGN(elf_brk)
> 
> Now, the question is whether it is valid for ELF binary to not have the 
> end of .bss section (if present at all) not page-aligned.
> 

Why wouldn't it be?  It would, however, be valid to the kernel to round 
it up to the next boundary.

	-hpa

Re: [Patch] Elf loader crash while zero-filling .bss

[Jiri Kosina]

On Mon, 11 Feb 2008, H. Peter Anvin wrote:


> > Now, the question is whether it is valid for ELF binary to not have the end
> > of .bss section (if present at all) not page-aligned.
> Why wouldn't it be?  It would, however, be valid to the kernel to round it up
> to the next boundary.

I wasn't immediately sure if there is nothing in ELF specification that 
would forbid that.

Then I think that Abel's patch is correct.

-- 
Jiri Kosina
SUSE Labs

Re: [Patch] Elf loader crash while zero-filling .bss

[Andreas Schwab]

Jiri Kosina <jkosina@suse.cz> writes:

> On Mon, 11 Feb 2008, H. Peter Anvin wrote:
>
>
>> > Now, the question is whether it is valid for ELF binary to not have the end
>> > of .bss section (if present at all) not page-aligned.
>> Why wouldn't it be?  It would, however, be valid to the kernel to round it up
>> to the next boundary.
>
> I wasn't immediately sure if there is nothing in ELF specification that 
> would forbid that.

The only requirement for a loadable segment is that its address is
congruent modulo alignment with the file offset.

Andreas.

-- 
Andreas Schwab, SuSE Labs, schwab@suse.de
SuSE Linux Products GmbH, Maxfeldstraße 5, 90409 Nürnberg, Germany
PGP key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

Re: [Patch] Elf loader crash while zero-filling .bss

[Sam Ravnborg]

On Mon, Feb 11, 2008 at 07:27:35PM +0100, Abel Bernabeu wrote:
> 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, but please test it yourself...
> 
> set_brk(start, end) allocs just page aligned regions (by "colapsing"
> 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).

What architecture was this?
Most architectures align .bss properly but it seems arm does not
and I guess this is needed.

As .bss was empty? in your case you did not trigger
any alignmnet by linker due to largest member in section => boom.

I do think your patch paper over the real bug.

	Sam