2008-02-11 18:27:51

by Abel Bernabeu

[permalink] [raw]
Subject: [Patch] Elf loader crash while zero-filling .bss

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);


2008-02-11 18:32:28

by Abel Bernabeu

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

2008/2/11, Abel Bernabeu <[email protected]>:
> 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)

2008-02-11 18:47:37

by Jiri Kosina

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

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

2008-02-11 18:53:45

by Sam Ravnborg

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

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

2008-02-11 19:28:53

by Jiri Kosina

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

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

2008-02-11 20:36:18

by H. Peter Anvin

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

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

2008-02-11 20:40:39

by Jiri Kosina

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

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

2008-02-11 22:14:53

by Andreas Schwab

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

Jiri Kosina <[email protected]> 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, [email protected]
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."