code review 4650067: 5l, 6l, 8l: Add a PT_LOAD PHDR entry for the PHDR.

5l, 6l, 8l: Add a PT_LOAD PHDR entry for the PHDR.

Per the TIS ELF spec, if a PHDR entry is present in the
program header table, it must be part of the memory image of
the program. Failure to do this makes elflint complain, and
causes some tools that manipulate ELF to crash.

[danderson, 2011-07-02 21:52:02]

Hello golang-dev@googlegroups.com (cc: dave@natulte.net,
golang-dev@googlegroups.com),

I'd like you to review this change to
https://go.googlecode.com/hg/

[danderson, 2011-07-02 21:57:25]

For reference, here's the diff of the output of `elflint` and `paxctl`,
running on a Go binary before/after this change:

--- before.txt  2011-07-02 14:29:33.847685849 -0700
+++ after.txt   2011-07-02 14:31:41.091685867 -0700
@@ -1,6 +1,5 @@
 % elflint main
-PHDR segment not contained in a loaded segment
 section [ 7] '.symtab': symbol 2: st_value out of bounds
 section [ 7] '.symtab': symbol 3: st_value out of bounds
 section [ 7] '.symtab': symbol 4: st_value out of bounds
 section [ 7] '.symtab': symbol 5: st_value out of bounds
@@ -833,5 +832,5 @@
 % paxctl -v main
 PaX control v0.5
 Copyright 2004,2005,2006,2007 PaX Team <pageexec@freemail.hu>

-file main is not a valid ELF executable (PT_PHDR is outside of first
PT_LOAD)
+file main does not have a PT_PAX_FLAGS program header, try conversion

(the new paxctl output is normal for a binary not built with a patched
binutils - but does indicate that paxctl successfully read the file)

I've tested this change on linux/386, and setting up a VM for linux/amd64
right now. The change is quite straightforward though, and implements an ELF
requirement from the base spec, not the architecture/OS specific sections,
so I don't expect any surprises.

- Dave

On Sat, Jul 2, 2011 at 14:52, <danderson@google.com> wrote:

> Reviewers: golang-dev_googlegroups.com,
>
> Message:
> Hello golang-dev@googlegroups.com (cc: dave@natulte.net,
> golang-dev@googlegroups.com),
>
> I'd like you to review this change to
> https://go.googlecode.com/hg/
>
>
> Description:
> 5l, 6l, 8l: Add a PT_LOAD PHDR entry for the PHDR.
>
> Per the TIS ELF spec, if a PHDR entry is present in the
> program header table, it must be part of the memory image of
> the program. Failure to do this makes elflint complain, and
> causes some tools that manipulate ELF to crash.
>
> Please review this at
http://codereview.appspot.com/**4650067/<http://codereview.appspot.com/4650067/>
>
> Affected files:
>  M src/cmd/5l/asm.c
>  M src/cmd/6l/asm.c
>  M src/cmd/8l/asm.c
>
>
> Index: src/cmd/5l/asm.c
> ==============================**==============================**=======
> --- a/src/cmd/5l/asm.c
> +++ b/src/cmd/5l/asm.c
> @@ -291,7 +291,7 @@ asmb(void)
>        int a, dynsym;
>        uint32 fo, symo, startva;
>        ElfEhdr *eh;
> -       ElfPhdr *ph, *pph;
> +       ElfPhdr *ph, *pph, *pphl;
>        ElfShdr *sh;
>        Section *sect;
>
> @@ -456,12 +456,21 @@ asmb(void)
>                /* program header info */
>                pph = newElfPhdr();
>                pph->type = PT_PHDR;
> -               pph->flags = PF_R + PF_X;
> +               pph->flags = PF_R;
>                pph->off = eh->ehsize;
>                pph->vaddr = INITTEXT - HEADR + pph->off;
>                pph->paddr = INITTEXT - HEADR + pph->off;
>                pph->align = INITRND;
>
> +               /* PHDR must be in a loaded segment */
> +               pphl = newElfPhdr();
> +               pphl->type = PT_LOAD;
> +               pphl->flags = PF_R;
> +               pphl->off = eh->ehsize;
> +               pphl->vaddr = INITTEXT - HEADR + pphl->off;
> +               pphl->paddr = INITTEXT - HEADR + pphl->off;
> +               pphl->align = INITRND;
> +
>                if(!debug['d']) {
>                        /* interpreter for dynamic linking */
>                        sh = newElfShdr(elfstr[**ElfStrInterp]);
> @@ -608,6 +617,8 @@ asmb(void)
>                if(pph != nil) {
>                        pph->filesz = eh->phnum * eh->phentsize;
>                        pph->memsz = pph->filesz;
> +                       pphl->filesz = pph->filesz;
> +                       pphl->memsz = pph->filesz;
>                }
>
>                seek(cout, 0, 0);
> Index: src/cmd/6l/asm.c
> ==============================**==============================**=======
> --- a/src/cmd/6l/asm.c
> +++ b/src/cmd/6l/asm.c
> @@ -696,7 +696,7 @@ asmb(void)
>        int a, dynsym;
>        vlong vl, startva, symo, machlink;
>        ElfEhdr *eh;
> -       ElfPhdr *ph, *pph;
> +       ElfPhdr *ph, *pph, *pphl;
>        ElfShdr *sh;
>        Section *sect;
>
> @@ -856,12 +856,21 @@ asmb(void)
>                /* program header info */
>                pph = newElfPhdr();
>                pph->type = PT_PHDR;
> -               pph->flags = PF_R + PF_X;
> +               pph->flags = PF_R;
>                pph->off = eh->ehsize;
>                pph->vaddr = INITTEXT - HEADR + pph->off;
>                pph->paddr = INITTEXT - HEADR + pph->off;
>                pph->align = INITRND;
>
> +               /* PHDR must be in a loaded segment */
> +               pphl = newElfPhdr();
> +               pphl->type = PT_LOAD;
> +               pphl->flags = PF_R;
> +               pphl->off = eh->ehsize;
> +               pphl->vaddr = INITTEXT - HEADR + pphl->off;
> +               pphl->paddr = INITTEXT - HEADR + pphl->off;
> +               pphl->align = INITRND;
> +
>                if(!debug['d']) {
>                        /* interpreter */
>                        sh = newElfShdr(elfstr[**ElfStrInterp]);
> @@ -1050,6 +1059,8 @@ asmb(void)
>
>                pph->filesz = eh->phnum * eh->phentsize;
>                pph->memsz = pph->filesz;
> +               pphl->filesz = pph->filesz;
> +               pphl->memsz = pph->filesz;
>
>                seek(cout, 0, 0);
>                a = 0;
> Index: src/cmd/8l/asm.c
> ==============================**==============================**=======
> --- a/src/cmd/8l/asm.c
> +++ b/src/cmd/8l/asm.c
> @@ -661,7 +661,7 @@ asmb(void)
>        int a, dynsym;
>        uint32 symo, startva, machlink;
>        ElfEhdr *eh;
> -       ElfPhdr *ph, *pph;
> +       ElfPhdr *ph, *pph, *pphl;
>        ElfShdr *sh;
>        Section *sect;
>        Sym *sym;
> @@ -926,12 +926,21 @@ asmb(void)
>                /* program header info */
>                pph = newElfPhdr();
>                pph->type = PT_PHDR;
> -               pph->flags = PF_R + PF_X;
> +               pph->flags = PF_R;
>                pph->off = eh->ehsize;
>                pph->vaddr = INITTEXT - HEADR + pph->off;
>                pph->paddr = INITTEXT - HEADR + pph->off;
>                pph->align = INITRND;
>
> +               /* PHDR must be in a loaded segment */
> +               pphl = newElfPhdr();
> +               pphl->type = PT_LOAD;
> +               pphl->flags = PF_R;
> +               pphl->off = eh->ehsize;
> +               pphl->vaddr = INITTEXT - HEADR + pphl->off;
> +               pphl->paddr = INITTEXT - HEADR + pphl->off;
> +               pphl->align = INITRND;
> +
>                if(!debug['d']) {
>                        /* interpreter */
>                        sh = newElfShdr(elfstr[**ElfStrInterp]);
> @@ -1124,6 +1133,8 @@ asmb(void)
>                if(pph != nil) {
>                        pph->filesz = eh->phnum * eh->phentsize;
>                        pph->memsz = pph->filesz;
> +                       pphl->filesz = pph->filesz;
> +                       pphl->memsz = pph->filesz;
>                }
>
>                seek(cout, 0, 0);
>
>
>

[danderson, 2011-07-02 23:51:27]

Tested on linux/amd64, same diff of elflint/paxctl.

- Dave

On Sat, Jul 2, 2011 at 14:57, David Anderson <danderson@google.com> wrote:

> For reference, here's the diff of the output of `elflint` and `paxctl`,
> running on a Go binary before/after this change:
>
> --- before.txt  2011-07-02 14:29:33.847685849 -0700
> +++ after.txt   2011-07-02 14:31:41.091685867 -0700
> @@ -1,6 +1,5 @@
>  % elflint main
> -PHDR segment not contained in a loaded segment
>  section [ 7] '.symtab': symbol 2: st_value out of bounds
>  section [ 7] '.symtab': symbol 3: st_value out of bounds
>  section [ 7] '.symtab': symbol 4: st_value out of bounds
>  section [ 7] '.symtab': symbol 5: st_value out of bounds
> @@ -833,5 +832,5 @@
>  % paxctl -v main
>  PaX control v0.5
>  Copyright 2004,2005,2006,2007 PaX Team <pageexec@freemail.hu>
>
> -file main is not a valid ELF executable (PT_PHDR is outside of first
> PT_LOAD)
> +file main does not have a PT_PAX_FLAGS program header, try conversion
>
> (the new paxctl output is normal for a binary not built with a patched
> binutils - but does indicate that paxctl successfully read the file)
>
> I've tested this change on linux/386, and setting up a VM for linux/amd64
> right now. The change is quite straightforward though, and implements an ELF
> requirement from the base spec, not the architecture/OS specific sections,
> so I don't expect any surprises.
>
> - Dave
>
> On Sat, Jul 2, 2011 at 14:52, <danderson@google.com> wrote:
>
>> Reviewers: golang-dev_googlegroups.com,
>>
>> Message:
>> Hello golang-dev@googlegroups.com (cc: dave@natulte.net,
>> golang-dev@googlegroups.com),
>>
>> I'd like you to review this change to
>> https://go.googlecode.com/hg/
>>
>>
>> Description:
>> 5l, 6l, 8l: Add a PT_LOAD PHDR entry for the PHDR.
>>
>> Per the TIS ELF spec, if a PHDR entry is present in the
>> program header table, it must be part of the memory image of
>> the program. Failure to do this makes elflint complain, and
>> causes some tools that manipulate ELF to crash.
>>
>> Please review this at
http://codereview.appspot.com/**4650067/<http://codereview.appspot.com/4650067/>
>>
>> Affected files:
>>  M src/cmd/5l/asm.c
>>  M src/cmd/6l/asm.c
>>  M src/cmd/8l/asm.c
>>
>>
>> Index: src/cmd/5l/asm.c
>> ==============================**==============================**=======
>> --- a/src/cmd/5l/asm.c
>> +++ b/src/cmd/5l/asm.c
>> @@ -291,7 +291,7 @@ asmb(void)
>>        int a, dynsym;
>>        uint32 fo, symo, startva;
>>        ElfEhdr *eh;
>> -       ElfPhdr *ph, *pph;
>> +       ElfPhdr *ph, *pph, *pphl;
>>        ElfShdr *sh;
>>        Section *sect;
>>
>> @@ -456,12 +456,21 @@ asmb(void)
>>                /* program header info */
>>                pph = newElfPhdr();
>>                pph->type = PT_PHDR;
>> -               pph->flags = PF_R + PF_X;
>> +               pph->flags = PF_R;
>>                pph->off = eh->ehsize;
>>                pph->vaddr = INITTEXT - HEADR + pph->off;
>>                pph->paddr = INITTEXT - HEADR + pph->off;
>>                pph->align = INITRND;
>>
>> +               /* PHDR must be in a loaded segment */
>> +               pphl = newElfPhdr();
>> +               pphl->type = PT_LOAD;
>> +               pphl->flags = PF_R;
>> +               pphl->off = eh->ehsize;
>> +               pphl->vaddr = INITTEXT - HEADR + pphl->off;
>> +               pphl->paddr = INITTEXT - HEADR + pphl->off;
>> +               pphl->align = INITRND;
>> +
>>                if(!debug['d']) {
>>                        /* interpreter for dynamic linking */
>>                        sh = newElfShdr(elfstr[**ElfStrInterp]);
>> @@ -608,6 +617,8 @@ asmb(void)
>>                if(pph != nil) {
>>                        pph->filesz = eh->phnum * eh->phentsize;
>>                        pph->memsz = pph->filesz;
>> +                       pphl->filesz = pph->filesz;
>> +                       pphl->memsz = pph->filesz;
>>                }
>>
>>                seek(cout, 0, 0);
>> Index: src/cmd/6l/asm.c
>> ==============================**==============================**=======
>> --- a/src/cmd/6l/asm.c
>> +++ b/src/cmd/6l/asm.c
>> @@ -696,7 +696,7 @@ asmb(void)
>>        int a, dynsym;
>>        vlong vl, startva, symo, machlink;
>>        ElfEhdr *eh;
>> -       ElfPhdr *ph, *pph;
>> +       ElfPhdr *ph, *pph, *pphl;
>>        ElfShdr *sh;
>>        Section *sect;
>>
>> @@ -856,12 +856,21 @@ asmb(void)
>>                /* program header info */
>>                pph = newElfPhdr();
>>                pph->type = PT_PHDR;
>> -               pph->flags = PF_R + PF_X;
>> +               pph->flags = PF_R;
>>                pph->off = eh->ehsize;
>>                pph->vaddr = INITTEXT - HEADR + pph->off;
>>                pph->paddr = INITTEXT - HEADR + pph->off;
>>                pph->align = INITRND;
>>
>> +               /* PHDR must be in a loaded segment */
>> +               pphl = newElfPhdr();
>> +               pphl->type = PT_LOAD;
>> +               pphl->flags = PF_R;
>> +               pphl->off = eh->ehsize;
>> +               pphl->vaddr = INITTEXT - HEADR + pphl->off;
>> +               pphl->paddr = INITTEXT - HEADR + pphl->off;
>> +               pphl->align = INITRND;
>> +
>>                if(!debug['d']) {
>>                        /* interpreter */
>>                        sh = newElfShdr(elfstr[**ElfStrInterp]);
>> @@ -1050,6 +1059,8 @@ asmb(void)
>>
>>                pph->filesz = eh->phnum * eh->phentsize;
>>                pph->memsz = pph->filesz;
>> +               pphl->filesz = pph->filesz;
>> +               pphl->memsz = pph->filesz;
>>
>>                seek(cout, 0, 0);
>>                a = 0;
>> Index: src/cmd/8l/asm.c
>> ==============================**==============================**=======
>> --- a/src/cmd/8l/asm.c
>> +++ b/src/cmd/8l/asm.c
>> @@ -661,7 +661,7 @@ asmb(void)
>>        int a, dynsym;
>>        uint32 symo, startva, machlink;
>>        ElfEhdr *eh;
>> -       ElfPhdr *ph, *pph;
>> +       ElfPhdr *ph, *pph, *pphl;
>>        ElfShdr *sh;
>>        Section *sect;
>>        Sym *sym;
>> @@ -926,12 +926,21 @@ asmb(void)
>>                /* program header info */
>>                pph = newElfPhdr();
>>                pph->type = PT_PHDR;
>> -               pph->flags = PF_R + PF_X;
>> +               pph->flags = PF_R;
>>                pph->off = eh->ehsize;
>>                pph->vaddr = INITTEXT - HEADR + pph->off;
>>                pph->paddr = INITTEXT - HEADR + pph->off;
>>                pph->align = INITRND;
>>
>> +               /* PHDR must be in a loaded segment */
>> +               pphl = newElfPhdr();
>> +               pphl->type = PT_LOAD;
>> +               pphl->flags = PF_R;
>> +               pphl->off = eh->ehsize;
>> +               pphl->vaddr = INITTEXT - HEADR + pphl->off;
>> +               pphl->paddr = INITTEXT - HEADR + pphl->off;
>> +               pphl->align = INITRND;
>> +
>>                if(!debug['d']) {
>>                        /* interpreter */
>>                        sh = newElfShdr(elfstr[**ElfStrInterp]);
>> @@ -1124,6 +1133,8 @@ asmb(void)
>>                if(pph != nil) {
>>                        pph->filesz = eh->phnum * eh->phentsize;
>>                        pph->memsz = pph->filesz;
>> +                       pphl->filesz = pph->filesz;
>> +                       pphl->memsz = pph->filesz;
>>                }
>>
>>                seek(cout, 0, 0);
>>
>>
>>
>

[iant2, 2011-07-03 04:25:41]

danderson@google.com writes:

> 5l, 6l, 8l: Add a PT_LOAD PHDR entry for the PHDR.
>
> Per the TIS ELF spec, if a PHDR entry is present in the
> program header table, it must be part of the memory image of
> the program. Failure to do this makes elflint complain, and
> causes some tools that manipulate ELF to crash.

How hard would it be to get the program headers into an existing PT_LOAD
segment, rather than introducing a new one.  A new one is OK but
avoiding the extra mmap at program startup time would be better.

Ian

[danderson, 2011-07-03 04:39:59]

On Sat, Jul 2, 2011 at 21:25, Ian Lance Taylor <iant@google.com> wrote:

> danderson@google.com writes:
>
> > 5l, 6l, 8l: Add a PT_LOAD PHDR entry for the PHDR.
> >
> > Per the TIS ELF spec, if a PHDR entry is present in the
> > program header table, it must be part of the memory image of
> > the program. Failure to do this makes elflint complain, and
> > causes some tools that manipulate ELF to crash.
>
> How hard would it be to get the program headers into an existing PT_LOAD
> segment, rather than introducing a new one.  A new one is OK but
> avoiding the extra mmap at program startup time would be better.
>

Relatively easy, assuming two things: that it's okay to also map the section
header table into the program image (it's between the PHDR and the current
text segment), and that it's okay to mark both those header tables R+X
instead of just R.

Poking in the headers of /bin/true built with GNU ld, looks like ld
typically maps the entire ELF header into memory along with the executable
segment, so I guess that's the canonical way to do it and my concerns are
unjustified. I'll make the change and update the code review.

- Dave


> Ian
>

[danderson, 2011-07-03 05:39:32]

On Sat, Jul 2, 2011 at 21:39, David Anderson <danderson@google.com> wrote:

> On Sat, Jul 2, 2011 at 21:25, Ian Lance Taylor <iant@google.com> wrote:
>
>> danderson@google.com writes:
>>
>> > 5l, 6l, 8l: Add a PT_LOAD PHDR entry for the PHDR.
>> >
>> > Per the TIS ELF spec, if a PHDR entry is present in the
>> > program header table, it must be part of the memory image of
>> > the program. Failure to do this makes elflint complain, and
>> > causes some tools that manipulate ELF to crash.
>>
>> How hard would it be to get the program headers into an existing PT_LOAD
>> segment, rather than introducing a new one.  A new one is OK but
>> avoiding the extra mmap at program startup time would be better.
>>
>
> Relatively easy, assuming two things: that it's okay to also map the
> section header table into the program image (it's between the PHDR and the
> current text segment), and that it's okay to mark both those header tables
> R+X instead of just R.
>
> Poking in the headers of /bin/true built with GNU ld, looks like ld
> typically maps the entire ELF header into memory along with the executable
> segment, so I guess that's the canonical way to do it and my concerns are
> unjustified. I'll make the change and update the code review.
>

Done.


>
> - Dave
>
>
>> Ian
>>
>
>

[rsc, 2011-07-12 18:37:51]

Thanks

http://codereview.appspot.com/4650067/diff/3/src/cmd/5l/asm.c
File src/cmd/5l/asm.c (right):

http://codereview.appspot.com/4650067/diff/3/src/cmd/5l/asm.c#newcode297
src/cmd/5l/asm.c:297: int textoff;
s/textoff/o/

http://codereview.appspot.com/4650067/diff/3/src/cmd/5l/asm.c#newcode466
src/cmd/5l/asm.c:466: /* PHDR must be in a loaded segment. Adjust the text
/* on line by itself in multiline comment.

[danderson, 2011-07-12 23:32:44]

Modifications made to all linkers.

http://codereview.appspot.com/4650067/diff/3/src/cmd/5l/asm.c
File src/cmd/5l/asm.c (right):

http://codereview.appspot.com/4650067/diff/3/src/cmd/5l/asm.c#newcode297
src/cmd/5l/asm.c:297: int textoff;
On 2011/07/12 18:37:52, rsc wrote:
> s/textoff/o/

Done.

http://codereview.appspot.com/4650067/diff/3/src/cmd/5l/asm.c#newcode466
src/cmd/5l/asm.c:466: /* PHDR must be in a loaded segment. Adjust the text
On 2011/07/12 18:37:52, rsc wrote:
> /* on line by itself in multiline comment.

Done.

[rsc, 2011-07-13 00:49:35]

LGTM

[rsc, 2011-07-13 00:49:59]

*** Submitted as http://code.google.com/p/go/source/detail?r=2131931a8b68 ***

5l, 6l, 8l: Add a PT_LOAD PHDR entry for the PHDR.

Per the TIS ELF spec, if a PHDR entry is present in the
program header table, it must be part of the memory image of
the program. Failure to do this makes elflint complain, and
causes some tools that manipulate ELF to crash.

R=iant, rsc
CC=dave, golang-dev
http://codereview.appspot.com/4650067

Committer: Russ Cox <rsc@golang.org>