Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::3:7 as permitted sender) client-ip=2620:137:e000::3:7;
From:   "Eric W. Biederman" <ebiederm@xmission.com>
To:     Kees Cook <keescook@chromium.org>
Cc:     Alexey Dobriyan <adobriyan@gmail.com>, akpm@linux-foundation.org,
        linux-kernel@vger.kernel.org, linux-doc@vger.kernel.org,
        Randy Dunlap <rdunlap@infradead.org>,
        Bagas Sanjaya <bagasdotme@gmail.com>,
        Jonathan Corbet <corbet@lwn.net>, linux-mm@kvack.org
References: <2acb586c-08a9-42d9-a41e-7986cc1383ea@p183>
        <e262ea00-a027-9073-812e-7e034d75e718@infradead.org>
        <c4233c97-306c-4db8-9667-34fc31ec4aed@p183>
        <87edp7jyu4.fsf@meer.lwn.net>
        <88d3f1bb-f4e0-4c40-9304-3843513a1262@p183>
        <202312061456.2103DA1@keescook>
Date:   Thu, 07 Dec 2023 09:03:45 -0600
In-Reply-To: <202312061456.2103DA1@keescook> (Kees Cook's message of "Wed, 6
        Dec 2023 14:58:23 -0800")
Message-ID: <874jgugilq.fsf@email.froward.int.ebiederm.org>
User-Agent: Gnus/5.13 (Gnus v5.13) Emacs/28.2 (gnu/linux)
MIME-Version: 1.0
Content-Type: text/plain
Subject: Re: [PATCH v3] ELF: document some de-facto PT_* ABI quirks
Precedence: bulk

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