Re: [PATCH] ELF: document some de-facto PT_* ABI quirks

Re: [PATCH] ELF: document some de-facto PT_* ABI quirks

[Randy Dunlap]

Hi,

[adding linux-doc for other interested parties]


On 3/14/23 10:02, Alexey Dobriyan wrote:
> Turns out rules about PT_INTERP, PT_GNU_STACK and PT_GNU_PROPERTY
> segment headers are slightly different.
> 
> Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
> ---
> 
>  Documentation/ELF/ELF.rst |   28 ++++++++++++++++++++++++++++
>  1 file changed, 28 insertions(+)
> 
> new file mode 100644
> --- /dev/null
> +++ b/Documentation/ELF/ELF.rst
> @@ -0,0 +1,28 @@
> +.. SPDX-License-Identifier: GPL-2.0

According to Documentation/doc-guide/sphinx.rst, "=" underlines are used
for chapters (by convention).

And could the document have a title, like:

=========================
ELF header usage in Linux
=========================

(I just made that up. Feel free to change it. :)

Also, the .rst file should be added to some chapter in the current
documentation tree, such as under "Other documentation", so add this file name
to Documentation/staging/index.rst. In fact this file could live in
Documentation/staging instead of in Documentation/ELF/ (IMO of course).


> +
> +Definitions
> +===========
> +
> +"First" program header is the one with the smallest offset in the file:
> +e_phoff. "Last" program header is the one with the biggest offset:
> +e_phoff + (e_phnum - 1) * sizeof(Elf_Phdr).
> +
> +PT_INTERP
> +=========
> +
> +First PT_INTERP program header is used to locate the filename of ELF
> +interpreter. Other PT_INTERP headers are ignored (since Linux 2.4.11).
> +
> +PT_GNU_STACK
> +============
> +
> +Last PT_GNU_STACK program header defines userspace stack executability
> +(since Linux 2.6.6). Other PT_GNU_STACK headers are ignored.
> +
> +PT_GNU_PROPERTY
> +===============
> +
> +ELF interpreter's last PT_GNU_PROPERTY program header is used (since
> +Linux 5.8). If interpreter doesn't have one, then the last PT_GNU_PROPERTY
> +program header of an executable is used. Other PT_GNU_PROPERTY headers
> +are ignored.

Thanks.
-- 
~Randy

Re: [PATCH] ELF: document some de-facto PT_* ABI quirks

[Bagas Sanjaya]

[-- Attachment #1: Type: text/plain, Size: 1136 bytes --]

On Tue, Mar 14, 2023 at 07:34:11PM -0700, Randy Dunlap wrote:
> Hi,
> 
> [adding linux-doc for other interested parties]

Unfortunately akpm had already applied this doc as 60b38b7cbb295d ("ELF:
document some de-facto PT_* ABI quirks") while it being reviewed and
doesn't have any consensus yet.

> And could the document have a title, like:
> 
> =========================
> ELF header usage in Linux
> =========================

The current doc path is Documentation/ELF/ELF.rst, which means that
readers expect to find general info about the executable format, not
some sort of trivia/niche like this.

> 
> (I just made that up. Feel free to change it. :)
> 
> Also, the .rst file should be added to some chapter in the current
> documentation tree, such as under "Other documentation", so add this file name
> to Documentation/staging/index.rst. In fact this file could live in
> Documentation/staging instead of in Documentation/ELF/ (IMO of course).

If there are more ELF docs there then a separate directory may be
warranted.

Thanks.

-- 
An old man doll... just what I always wanted! - Clara

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 228 bytes --]

Re: [PATCH] ELF: document some de-facto PT_* ABI quirks

[Alexey Dobriyan]

On Fri, Mar 17, 2023 at 03:22:28PM +0700, Bagas Sanjaya wrote:
> On Tue, Mar 14, 2023 at 07:34:11PM -0700, Randy Dunlap wrote:
> > Hi,
> > 
> > [adding linux-doc for other interested parties]
> 
> Unfortunately akpm had already applied this doc as 60b38b7cbb295d ("ELF:
> document some de-facto PT_* ABI quirks") while it being reviewed and
> doesn't have any consensus yet.
> 
> > And could the document have a title, like:
> > 
> > =========================
> > ELF header usage in Linux
> > =========================
> 
> The current doc path is Documentation/ELF/ELF.rst, which means that
> readers expect to find general info about the executable format, not
> some sort of trivia/niche like this.

General info is in ELF spec. This document is intended to be Linux
specific stuff you won't find anywhere but source.

I'll write down overmapping rules as well.

> > (I just made that up. Feel free to change it. :)
> > 
> > Also, the .rst file should be added to some chapter in the current
> > documentation tree, such as under "Other documentation", so add this file name
> > to Documentation/staging/index.rst. In fact this file could live in
> > Documentation/staging instead of in Documentation/ELF/ (IMO of course).
> 
> If there are more ELF docs there then a separate directory may be
> warranted.

This is codification of what Linux has been doing for years:
e.g pre 2.4.11 executables with multiple PT_INTERP segments were
rejected.

Re: [PATCH] ELF: document some de-facto PT_* ABI quirks

[Bagas Sanjaya]

On 3/17/23 23:53, Alexey Dobriyan wrote:
>> The current doc path is Documentation/ELF/ELF.rst, which means that
>> readers expect to find general info about the executable format, not
>> some sort of trivia/niche like this.
> 
> General info is in ELF spec. This document is intended to be Linux
> specific stuff you won't find anywhere but source.
> 
> I'll write down overmapping rules as well.

In the same Documentation/ELF/ELF.rst?

And thus, the doc title should be "Linux-specific ELF notes", right?

-- 
An old man doll... just what I always wanted! - Clara

[PATCH v2] ELF: document some de-facto PT_* ABI quirks

[Alexey Dobriyan]

Turns out rules about PT_INTERP, PT_GNU_STACK and PT_GNU_PROPERTY
program headers are slightly different.

Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
---

	v2: integrate into documentation build system

 Documentation/ELF/ELF.rst   |   32 ++++++++++++++++++++++++++++++++
 Documentation/ELF/index.rst |   10 ++++++++++
 Documentation/index.rst     |    1 +
 3 files changed, 43 insertions(+)

new file mode 100644
--- /dev/null
+++ b/Documentation/ELF/ELF.rst
@@ -0,0 +1,32 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+=================================
+Linux-specific ELF idiosyncrasies
+=================================
+
+Definitions
+===========
+
+"First" program header is the one with the smallest offset in the file:
+e_phoff. "Last" program header is the one with the biggest offset:
+e_phoff + (e_phnum - 1) * sizeof(Elf_Phdr).
+
+PT_INTERP
+=========
+
+First PT_INTERP program header is used to locate the filename of ELF
+interpreter. Other PT_INTERP headers are ignored (since Linux 2.4.11).
+
+PT_GNU_STACK
+============
+
+Last PT_GNU_STACK program header defines userspace stack executability
+(since Linux 2.6.6). Other PT_GNU_STACK headers are ignored.
+
+PT_GNU_PROPERTY
+===============
+
+ELF interpreter's last PT_GNU_PROPERTY program header is used (since
+Linux 5.8). If interpreter doesn't have one, then the last PT_GNU_PROPERTY
+program header of an executable is used. Other PT_GNU_PROPERTY headers
+are ignored.
new file mode 100644
--- /dev/null
+++ b/Documentation/ELF/index.rst
@@ -0,0 +1,10 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+===
+ELF
+===
+
+.. toctree::
+   :maxdepth: 1
+
+   ELF
--- a/Documentation/index.rst
+++ b/Documentation/index.rst
@@ -113,6 +113,7 @@ to ReStructured Text format, or are simply too old.
    :maxdepth: 1
 
    staging/index
+   ELF/index
 
 
 Translations

Re: [PATCH v2] ELF: document some de-facto PT_* ABI quirks

[Randy Dunlap]

Hi Alexey,

On 3/26/23 09:49, Alexey Dobriyan wrote:
> Turns out rules about PT_INTERP, PT_GNU_STACK and PT_GNU_PROPERTY
> program headers are slightly different.
> 
> Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
> ---
> 
> 	v2: integrate into documentation build system
> 
>  Documentation/ELF/ELF.rst   |   32 ++++++++++++++++++++++++++++++++
>  Documentation/ELF/index.rst |   10 ++++++++++
>  Documentation/index.rst     |    1 +
>  3 files changed, 43 insertions(+)

I'm not sure that ELF merits its own subdirectory or that each item here
should be a chapter,  but this fixes all of the issues that I pointed out. Thanks.

Reviewed-by: Randy Dunlap <rdunlap@infradead.org>

> new file mode 100644
> --- /dev/null
> +++ b/Documentation/ELF/ELF.rst
> @@ -0,0 +1,32 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +=================================
> +Linux-specific ELF idiosyncrasies
> +=================================
> +
> +Definitions
> +===========
> +
> +"First" program header is the one with the smallest offset in the file:
> +e_phoff. "Last" program header is the one with the biggest offset:
> +e_phoff + (e_phnum - 1) * sizeof(Elf_Phdr).
> +
> +PT_INTERP
> +=========
> +
> +First PT_INTERP program header is used to locate the filename of ELF
> +interpreter. Other PT_INTERP headers are ignored (since Linux 2.4.11).
> +
> +PT_GNU_STACK
> +============
> +
> +Last PT_GNU_STACK program header defines userspace stack executability
> +(since Linux 2.6.6). Other PT_GNU_STACK headers are ignored.
> +
> +PT_GNU_PROPERTY
> +===============
> +
> +ELF interpreter's last PT_GNU_PROPERTY program header is used (since
> +Linux 5.8). If interpreter doesn't have one, then the last PT_GNU_PROPERTY
> +program header of an executable is used. Other PT_GNU_PROPERTY headers
> +are ignored.
> new file mode 100644
> --- /dev/null
> +++ b/Documentation/ELF/index.rst
> @@ -0,0 +1,10 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +===
> +ELF
> +===
> +
> +.. toctree::
> +   :maxdepth: 1
> +
> +   ELF
> --- a/Documentation/index.rst
> +++ b/Documentation/index.rst
> @@ -113,6 +113,7 @@ to ReStructured Text format, or are simply too old.
>     :maxdepth: 1
>  
>     staging/index
> +   ELF/index
>  
>  
>  Translations

-- 
~Randy

Re: [PATCH v2] ELF: document some de-facto PT_* ABI quirks

[Jonathan Corbet]

Alexey Dobriyan <adobriyan@gmail.com> writes:

> Turns out rules about PT_INTERP, PT_GNU_STACK and PT_GNU_PROPERTY
> program headers are slightly different.
>
> Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
> ---
>
> 	v2: integrate into documentation build system
>
>  Documentation/ELF/ELF.rst   |   32 ++++++++++++++++++++++++++++++++
>  Documentation/ELF/index.rst |   10 ++++++++++
>  Documentation/index.rst     |    1 +
>  3 files changed, 43 insertions(+)

I really don't want to add another top-level directory for a single
short file ... I'm trying to have fewer of those directories, not more.

This is essentially use-space ABI information; I think you should really
just drop a file into Documentation/userspace-api/.

Thanks,

jon

[PATCH v3] ELF: document some de-facto PT_* ABI quirks

[Alexey Dobriyan]

Turns out rules about PT_INTERP, PT_GNU_STACK and PT_GNU_PROPERTY
program headers are slightly different.

Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
---

	v3: move to Documentation/userspace-api/
	v2: integrate into documentation build system

 Documentation/userspace-api/ELF.rst   |   34 ++++++++++++++++++++++++++++++++++
 Documentation/userspace-api/index.rst |    1 +
 2 files changed, 35 insertions(+)

new file mode 100644
--- /dev/null
+++ b/Documentation/userspace-api/ELF.rst
@@ -0,0 +1,34 @@
+.. SPDX-License-Identifier: GPL-2.0
+
+=================================
+Linux-specific ELF idiosyncrasies
+=================================
+
+Definitions
+===========
+
+"First" program header is the one with the smallest offset in the file:
+e_phoff.
+
+"Last" program header is the one with the biggest offset in the file:
+e_phoff + (e_phnum - 1) * sizeof(Elf_Phdr).
+
+PT_INTERP
+=========
+
+First PT_INTERP program header is used to locate the filename of ELF
+interpreter. Other PT_INTERP headers are ignored (since Linux 2.4.11).
+
+PT_GNU_STACK
+============
+
+Last PT_GNU_STACK program header defines userspace stack executability
+(since Linux 2.6.6). Other PT_GNU_STACK headers are ignored.
+
+PT_GNU_PROPERTY
+===============
+
+ELF interpreter's last PT_GNU_PROPERTY program header is used (since
+Linux 5.8). If interpreter doesn't have one, then the last PT_GNU_PROPERTY
+program header of an executable is used. Other PT_GNU_PROPERTY headers
+are ignored.
--- a/Documentation/userspace-api/index.rst
+++ b/Documentation/userspace-api/index.rst
@@ -23,6 +23,7 @@ place where this information is gathered.
    spec_ctrl
    accelerators/ocxl
    ebpf/index
+   ELF
    ioctl/index
    iommu
    iommufd

Re: [PATCH v3] ELF: document some de-facto PT_* ABI quirks

[Jonathan Corbet]

Alexey Dobriyan <adobriyan@gmail.com> writes:

> Turns out rules about PT_INTERP, PT_GNU_STACK and PT_GNU_PROPERTY
> program headers are slightly different.
>
> Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
> ---
>
> 	v3: move to Documentation/userspace-api/
> 	v2: integrate into documentation build system
>
>  Documentation/userspace-api/ELF.rst   |   34 ++++++++++++++++++++++++++++++++++
>  Documentation/userspace-api/index.rst |    1 +
>  2 files changed, 35 insertions(+)

Applied, thanks.

jon

Re: [PATCH v3] ELF: document some de-facto PT_* ABI quirks

[Kees Cook]

*thread necromancy* Question below...

On Sat, Apr 15, 2023 at 08:37:29PM +0300, Alexey Dobriyan wrote:
> Turns out rules about PT_INTERP, PT_GNU_STACK and PT_GNU_PROPERTY
> program headers are slightly different.
> 
> Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
> ---
> 
> 	v3: move to Documentation/userspace-api/
> 	v2: integrate into documentation build system
> 
>  Documentation/userspace-api/ELF.rst   |   34 ++++++++++++++++++++++++++++++++++
>  Documentation/userspace-api/index.rst |    1 +
>  2 files changed, 35 insertions(+)
> 
> new file mode 100644
> --- /dev/null
> +++ b/Documentation/userspace-api/ELF.rst
> @@ -0,0 +1,34 @@
> +.. SPDX-License-Identifier: GPL-2.0
> +
> +=================================
> +Linux-specific ELF idiosyncrasies
> +=================================
> +
> +Definitions
> +===========
> +
> +"First" program header is the one with the smallest offset in the file:
> +e_phoff.
> +
> +"Last" program header is the one with the biggest offset in the file:
> +e_phoff + (e_phnum - 1) * sizeof(Elf_Phdr).
> +
> +PT_INTERP
> +=========
> +
> +First PT_INTERP program header is used to locate the filename of ELF
> +interpreter. Other PT_INTERP headers are ignored (since Linux 2.4.11).
> +
> +PT_GNU_STACK
> +============
> +
> +Last PT_GNU_STACK program header defines userspace stack executability
> +(since Linux 2.6.6). Other PT_GNU_STACK headers are ignored.
> +
> +PT_GNU_PROPERTY
> +===============
> +
> +ELF interpreter's last PT_GNU_PROPERTY program header is used (since
> +Linux 5.8). If interpreter doesn't have one, then the last PT_GNU_PROPERTY
> +program header of an executable is used. Other PT_GNU_PROPERTY headers
> +are ignored.

Should we perhaps solve some of these in some way? What would folks
prefer the behaviors be? (I like to have things been "as expected", but
it's not very obvious here for redundant headers...)

-- 
Kees Cook

Re: [PATCH v3] ELF: document some de-facto PT_* ABI quirks

[Eric W. Biederman]

Kees Cook <keescook@chromium.org> writes:

> *thread necromancy* Question below...
>
> On Sat, Apr 15, 2023 at 08:37:29PM +0300, Alexey Dobriyan wrote:
>> Turns out rules about PT_INTERP, PT_GNU_STACK and PT_GNU_PROPERTY
>> program headers are slightly different.
>> 
>> Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
>> ---
>> 
>> 	v3: move to Documentation/userspace-api/
>> 	v2: integrate into documentation build system
>> 
>>  Documentation/userspace-api/ELF.rst   |   34 ++++++++++++++++++++++++++++++++++
>>  Documentation/userspace-api/index.rst |    1 +
>>  2 files changed, 35 insertions(+)
>> 
>> new file mode 100644
>> --- /dev/null
>> +++ b/Documentation/userspace-api/ELF.rst
>> @@ -0,0 +1,34 @@
>> +.. SPDX-License-Identifier: GPL-2.0
>> +
>> +=================================
>> +Linux-specific ELF idiosyncrasies
>> +=================================
>> +
>> +Definitions
>> +===========
>> +
>> +"First" program header is the one with the smallest offset in the file:
>> +e_phoff.

Confusing e_phoff is the defined location of the array of program
headers.

Perhaps the "First" in that array with the lowest e_phnum?

>> +"Last" program header is the one with the biggest offset in the file:
>> +e_phoff + (e_phnum - 1) * sizeof(Elf_Phdr).

Ditto the "Last" in the array with the largest array index.

I nit pick this because it sounded at first like you were talking about
p_offset.  Which is a value contained in the program header entry.

>> +PT_INTERP
>> +=========
>> +
>> +First PT_INTERP program header is used to locate the filename of ELF
>> +interpreter. Other PT_INTERP headers are ignored (since Linux 2.4.11).
>> +
>> +PT_GNU_STACK
>> +============
>> +
>> +Last PT_GNU_STACK program header defines userspace stack executability
>> +(since Linux 2.6.6). Other PT_GNU_STACK headers are ignored.
>> +
>> +PT_GNU_PROPERTY
>> +===============
>> +
>> +ELF interpreter's last PT_GNU_PROPERTY program header is used (since
>> +Linux 5.8). If interpreter doesn't have one, then the last PT_GNU_PROPERTY
>> +program header of an executable is used. Other PT_GNU_PROPERTY headers
>> +are ignored.

A more interesting property to document is that PT_GNU_PROPERTY must
precede PT_INTERP in the linux implementation, otherwise we ignore it.

> Should we perhaps solve some of these in some way? What would folks
> prefer the behaviors be? (I like to have things been "as expected", but
> it's not very obvious here for redundant headers...)

All of these are really headers that should appear only once.

Quite frankly if we are going to do something with this my sense is that
we should fail the execve with a clear error code as userspace should
not be doing this, and accepting a malformed executable will hide
errors, and perhaps hide someone causing problems.

I really don't think having multiple copies of these headers with
different values is something we should encourage.

It looks like -ELIBBAD is the documented way to fail and report
a bad file format.


For PT_GNU_PROPTERTY perhaps we should accept it anywhere, instead of
silently ignoring it depending upon it's location?

I thinking change the code to talk one pass through the program headers
to identify the interesting headers, and then with the interesting
headers all identified we go do something with them.

Anyway just my opinion, but that is what it feels like to me.

Eric

Re: [PATCH v3] ELF: document some de-facto PT_* ABI quirks

[Alexey Dobriyan]

On Thu, Dec 07, 2023 at 09:03:45AM -0600, Eric W. Biederman wrote:
> Kees Cook <keescook@chromium.org> writes:
> 
> > *thread necromancy* Question below...
> >
> > On Sat, Apr 15, 2023 at 08:37:29PM +0300, Alexey Dobriyan wrote:
> >> Turns out rules about PT_INTERP, PT_GNU_STACK and PT_GNU_PROPERTY
> >> program headers are slightly different.
> >> 
> >> Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
> >> ---
> >> 
> >> 	v3: move to Documentation/userspace-api/
> >> 	v2: integrate into documentation build system
> >> 
> >>  Documentation/userspace-api/ELF.rst   |   34 ++++++++++++++++++++++++++++++++++
> >>  Documentation/userspace-api/index.rst |    1 +
> >>  2 files changed, 35 insertions(+)
> >> 
> >> new file mode 100644
> >> --- /dev/null
> >> +++ b/Documentation/userspace-api/ELF.rst
> >> @@ -0,0 +1,34 @@
> >> +.. SPDX-License-Identifier: GPL-2.0
> >> +
> >> +=================================
> >> +Linux-specific ELF idiosyncrasies
> >> +=================================
> >> +
> >> +Definitions
> >> +===========
> >> +
> >> +"First" program header is the one with the smallest offset in the file:
> >> +e_phoff.
> 
> Confusing e_phoff is the defined location of the array of program
> headers.
> 
> Perhaps the "First" in that array with the lowest e_phnum?
> 
> >> +"Last" program header is the one with the biggest offset in the file:
> >> +e_phoff + (e_phnum - 1) * sizeof(Elf_Phdr).
> 
> Ditto the "Last" in the array with the largest array index.
> 
> I nit pick this because it sounded at first like you were talking about
> p_offset.  Which is a value contained in the program header entry.
> 
> >> +PT_INTERP
> >> +=========
> >> +
> >> +First PT_INTERP program header is used to locate the filename of ELF
> >> +interpreter. Other PT_INTERP headers are ignored (since Linux 2.4.11).
> >> +
> >> +PT_GNU_STACK
> >> +============
> >> +
> >> +Last PT_GNU_STACK program header defines userspace stack executability
> >> +(since Linux 2.6.6). Other PT_GNU_STACK headers are ignored.
> >> +
> >> +PT_GNU_PROPERTY
> >> +===============
> >> +
> >> +ELF interpreter's last PT_GNU_PROPERTY program header is used (since
> >> +Linux 5.8). If interpreter doesn't have one, then the last PT_GNU_PROPERTY
> >> +program header of an executable is used. Other PT_GNU_PROPERTY headers
> >> +are ignored.
> 
> A more interesting property to document is that PT_GNU_PROPERTY must
> precede PT_INTERP in the linux implementation, otherwise we ignore it.
> 
> > Should we perhaps solve some of these in some way? What would folks
> > prefer the behaviors be? (I like to have things been "as expected", but
> > it's not very obvious here for redundant headers...)
> 
> All of these are really headers that should appear only once.

Yes.

> Quite frankly if we are going to do something with this my sense is that
> we should fail the execve with a clear error code as userspace should
> not be doing this, and accepting a malformed executable will hide
> errors, and perhaps hide someone causing problems.

Maybe do it for PT_GNU_PROPERTY which is relatively new.

> I really don't think having multiple copies of these headers with
> different values is something we should encourage.
> 
> It looks like -ELIBBAD is the documented way to fail and report
> a bad file format.

It is obvious you don't know how much will break.

> For PT_GNU_PROPTERTY perhaps we should accept it anywhere, instead of
> silently ignoring it depending upon it's location?
> 
> I thinking change the code to talk one pass through the program headers
> to identify the interesting headers, and then with the interesting
> headers all identified we go do something with them.
> 
> Anyway just my opinion, but that is what it feels like to me.

_Not_ checking for duplicates will result in the simplest and fastest exec.
which is what current code does.

Re: [PATCH v3] ELF: document some de-facto PT_* ABI quirks

[Eric W. Biederman]

Alexey Dobriyan <adobriyan@gmail.com> writes:

> On Thu, Dec 07, 2023 at 09:03:45AM -0600, Eric W. Biederman wrote:
>> Kees Cook <keescook@chromium.org> writes:
>> 
>> > *thread necromancy* Question below...
>> >
>> > On Sat, Apr 15, 2023 at 08:37:29PM +0300, Alexey Dobriyan wrote:
>> >> Turns out rules about PT_INTERP, PT_GNU_STACK and PT_GNU_PROPERTY
>> >> program headers are slightly different.
>> >> 
>> >> Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com>
>> >> ---
>> >> 
>> >> 	v3: move to Documentation/userspace-api/
>> >> 	v2: integrate into documentation build system
>> >> 
>> >>  Documentation/userspace-api/ELF.rst   |   34 ++++++++++++++++++++++++++++++++++
>> >>  Documentation/userspace-api/index.rst |    1 +
>> >>  2 files changed, 35 insertions(+)
>> >> 
>> >> new file mode 100644
>> >> --- /dev/null
>> >> +++ b/Documentation/userspace-api/ELF.rst
>> >> @@ -0,0 +1,34 @@
>> >> +.. SPDX-License-Identifier: GPL-2.0
>> >> +
>> >> +=================================
>> >> +Linux-specific ELF idiosyncrasies
>> >> +=================================
>> >> +
>> >> +Definitions
>> >> +===========
>> >> +
>> >> +"First" program header is the one with the smallest offset in the file:
>> >> +e_phoff.
>> 
>> Confusing e_phoff is the defined location of the array of program
>> headers.
>> 
>> Perhaps the "First" in that array with the lowest e_phnum?
>> 
>> >> +"Last" program header is the one with the biggest offset in the file:
>> >> +e_phoff + (e_phnum - 1) * sizeof(Elf_Phdr).
>> 
>> Ditto the "Last" in the array with the largest array index.
>> 
>> I nit pick this because it sounded at first like you were talking about
>> p_offset.  Which is a value contained in the program header entry.
>> 
>> >> +PT_INTERP
>> >> +=========
>> >> +
>> >> +First PT_INTERP program header is used to locate the filename of ELF
>> >> +interpreter. Other PT_INTERP headers are ignored (since Linux 2.4.11).
>> >> +
>> >> +PT_GNU_STACK
>> >> +============
>> >> +
>> >> +Last PT_GNU_STACK program header defines userspace stack executability
>> >> +(since Linux 2.6.6). Other PT_GNU_STACK headers are ignored.
>> >> +
>> >> +PT_GNU_PROPERTY
>> >> +===============
>> >> +
>> >> +ELF interpreter's last PT_GNU_PROPERTY program header is used (since
>> >> +Linux 5.8). If interpreter doesn't have one, then the last PT_GNU_PROPERTY
>> >> +program header of an executable is used. Other PT_GNU_PROPERTY headers
>> >> +are ignored.
>> 
>> A more interesting property to document is that PT_GNU_PROPERTY must
>> precede PT_INTERP in the linux implementation, otherwise we ignore it.
>> 
>> > Should we perhaps solve some of these in some way? What would folks
>> > prefer the behaviors be? (I like to have things been "as expected", but
>> > it's not very obvious here for redundant headers...)
>> 
>> All of these are really headers that should appear only once.
>
> Yes.
>
>> Quite frankly if we are going to do something with this my sense is that
>> we should fail the execve with a clear error code as userspace should
>> not be doing this, and accepting a malformed executable will hide
>> errors, and perhaps hide someone causing problems.
>
> Maybe do it for PT_GNU_PROPERTY which is relatively new.
>
>> I really don't think having multiple copies of these headers with
>> different values is something we should encourage.
>> 
>> It looks like -ELIBBAD is the documented way to fail and report
>> a bad file format.
>
> It is obvious you don't know how much will break.

My assumption is frankly that nothing will break.  My quick examination
of userspace binaries suggests that nothing is silly enough to duplicate
such headers.

Do you know of a binaries in userspace that duplicate these headers?

Without a documented ordering arguably anything that results in
these headers being duplicated is already buggy, and broken.

I can think of no use for duplicating these headers other than
as some kind of gadget in an exploit.  I don't see how such
a gadget would be useful currently.

>
>> For PT_GNU_PROPTERTY perhaps we should accept it anywhere, instead of
>> silently ignoring it depending upon it's location?
>> 
>> I thinking change the code to talk one pass through the program headers
>> to identify the interesting headers, and then with the interesting
>> headers all identified we go do something with them.
>> 
>> Anyway just my opinion, but that is what it feels like to me.
>
> _Not_ checking for duplicates will result in the simplest and fastest exec.
> which is what current code does.

Given that I/O is involved taking a pre-pass through the headers is
in the noise, and it might even make the code faster as it would
prime the code for the other passes.

The fastest of course would be to have the elf loader only look
at the first of any of these headers.

What got you wanting to document how we handle duplicates?

Eric

Re: [PATCH v3] ELF: document some de-facto PT_* ABI quirks

[Alexey Dobriyan]

On Sun, Dec 10, 2023 at 04:58:50PM -0600, Eric W. Biederman wrote:
> Alexey Dobriyan <adobriyan@gmail.com> writes:
> 
> > On Thu, Dec 07, 2023 at 09:03:45AM -0600, Eric W. Biederman wrote:

> >> Quite frankly if we are going to do something with this my sense is that
> >> we should fail the execve with a clear error code as userspace should
> >> not be doing this, and accepting a malformed executable will hide
> >> errors, and perhaps hide someone causing problems.
> >
> > Maybe do it for PT_GNU_PROPERTY which is relatively new.
> >
> >> I really don't think having multiple copies of these headers with
> >> different values is something we should encourage.
> >> 
> >> It looks like -ELIBBAD is the documented way to fail and report
> >> a bad file format.
> >
> > It is obvious you don't know how much will break.
> 
> My assumption is frankly that nothing will break.  My quick examination
> of userspace binaries suggests that nothing is silly enough to duplicate
> such headers.

Ha! Non-overlapping PT_LOAD segments is reasonable requirement (why would
you have them?) but it was reverted.

> Do you know of a binaries in userspace that duplicate these headers?
> 
> Without a documented ordering arguably anything that results in
> these headers being duplicated is already buggy, and broken.
> 
> I can think of no use for duplicating these headers other than
> as some kind of gadget in an exploit.  I don't see how such
> a gadget would be useful currently.
> 
> >
> >> For PT_GNU_PROPTERTY perhaps we should accept it anywhere, instead of
> >> silently ignoring it depending upon it's location?
> >> 
> >> I thinking change the code to talk one pass through the program headers
> >> to identify the interesting headers, and then with the interesting
> >> headers all identified we go do something with them.
> >> 
> >> Anyway just my opinion, but that is what it feels like to me.
> >
> > _Not_ checking for duplicates will result in the simplest and fastest exec.
> > which is what current code does.
> 
> Given that I/O is involved taking a pre-pass through the headers is
> in the noise, and it might even make the code faster as it would
> prime the code for the other passes.

Branches will evict other branches from branch predictor.
And it is always more code.

ELF is very rigid format. E.g segment headers can overlap everything
else and it is not a problem. Overmapped PT_LOAD segments aren't
a problem too (for the kernel).

These things should have been rejected from the very beginning.

I'd even argue kernel rejects too much:

		elf_entry = e_entry;
                if (BAD_ADDR(elf_entry)) {
                        retval = -EINVAL;
                        goto out_free_dentry;
                }

Why even check? If e_entry is bad than process will segfault and that's it.

		elf_ppnt->p_filesz > elf_ppnt->p_memsz

Again, why check, just map the minimum.

> The fastest of course would be to have the elf loader only look
> at the first of any of these headers.
> 
> What got you wanting to document how we handle duplicates?

I read ELF code too much and noticed that loops are slightly different,
that's all.