Re: Fwd: [PATCH v5 00/22] Some rtld-audit fixes

[Florian Weimer <fweimer@redhat.com>]

* Jonathon Anderson:

>>> Right now, we
>>> only require the program headers which we can obtain from
>>> getauxval(AT_PHDR), however this technique has questionable
>>> portability and robustness (getauxval returns an unsigned long, not a
>>> pointer).

>> A glibc port to an architecture where a long value cannot hold all
>> pointer values will have to provide an alternative interface similar to
>> getauxval, but that returns pointer values.

> I would go one step farther and say getauxval is already broken for
> any 64-bit architecture, unsigned long is only required to support 32
> bits as per the C standards. One of my greater fears is that some
> exotic compiler will cleverly allocate only 4 bytes of stack space for
> the return value, and we wouldn't know except for a subtle bug
> (dependent on optimization flags!) that crashes our entire tool with
> SEGVs in the auditor (where GDB doesn't give properly unwound call
> stacks).

If ported to such an architecture, glibc would need several changes to
accomodate this.  Newer architectures take this into account and do not
do funny things.  But Morello, as a capabilities-based architecture,
does not have this luxury, so they have to do something about this
interface.  But that is (still) an outlier.

I think the important point is that glibc interfaces do not need to be
fully API-compatible with future architecture requirements.  We can
change APIs for future ports.

>> Of course that's not the
>> only interface with this problem (ElfW(Addr) is an integer as well).

> AFAICT ElfW(Addr) is fine, it should always be an integer large enough
> to store a pointer on the host architecture (i.e. a uintptr_t). Unless
> I missed some specific arch where this doesn't work out to be the
> case?

Morello and other capabilities-based architectures.  Pointers need to
pointers there.  Weird architectures do not have uintptr_t.

>> makes the Morello glibc port quite interesting.  So I think *something*
>> like getauxval (AT_PHDR) will always be available, with pretty much
>> identical semantics.
>>
>>>  From an outside perspective the current l_addr semantic is fairly
>>> undocumented, the dladdr and dlinfo man pages define it vaguely as
>>> the "difference between the address in the ELF file and the address
>>> in memory." That sounds (to me at least) like l_addr should point to
>>> byte 0 in the file (the ELF header), and that seems to be correct in
>>> all but the non-PIE case.
>> I have struggled with this in the past.  I agree that it is confusing.
>> l_addr is the offset between virtual addresses in the program header of
>> the ELF object and the actual addresses in the process image.  This
>> offset happens to be 0 for ET_EXEC objects, and only there.

> This is a much clearer description of the semantic, it would be very
> helpful the man pages used that sentence (or one like it) wherever the 
> l_addr value is exposed in the API (link_map->l_addr and
> dl_phdr_info->dlpi_addr). It would also be very helpful if 
> Dl_info.dli_fbase was clearly documented as *not* l_addr but instead
> byte 0/ELF header in the process image.

I've made a note to update the manual pages.

> That does sound like the "correct" way out, but dl_iterate_phdr
> operates on the caller's namespace so one would need to inject a shim
> library to do the actual call.

Ugh, you are right.  It means we currently can't unwind across dlmopen
boundaries.

So please use getauxval (AT_PHDR) for now.  It is fully portable across
all present glibc targets.

>>> dladdr gets its value from link_map->l_map_start instead of l_addr,
>>> so the semantic we want is already present in a private field. It
>>> seems to me these two fields could be swapped with little issue, if
>>> altering the public semantic is not acceptable we could also be sated
>>> if l_map_start was made public.
>> Applications which know about the current semantics of l_addr will
>> break, though.  l_addr is also exposed to debuggers via the _r_debug
>> interface.  I really do not think we can make changes to l_addr.
>> We have a similar issue around l_name being "" for the main program, and
>> unfortuantely I will have to argue quite strongly against changing that.

> Is adding new public fields completely off the table?

To struct link_map?  We could probably pull it off, but it would be
years until such a change will be in the hands of the users.  There is
an internal structure that overlaps with the public struct link_map,
and some applications poke at the private bits at fixed offsets.

We've started not to strip ld.so downstream, so that these applications
can switch to DWARF data to avoid dependencies on fixed offsets, but
that has been a very recent change.

> If I can humor the impossible for a few moments longer, I personally
> have a difficult time believing that anyone actually uses 
> link_map->l_addr or link_map->l_name in a way that would break by
> changing their semantics for the main executable:

>  - The documentation hasn't improved for years so there can't be many
> users that care about (or even noticed) this case in particular.

"" for the main executable is widely known.  Usually code uses it to
implement a fallback on argv[0] or /proc/self/exe, though.

Changing l_addr will break the libgcc unwinder.  It uses l_addr to
relocate the program header (see the code I quoted previously).  Not
everyone uses the platform unwinder, and the libgcc unwinder is
sometimes linked statically.  This is different from the l_name change:
The l_addr would definitely cause widespread breakage.

>  - Every use case I can think of for obtaining a link_map from the dl*
> functions (dlinfo and dladdr1) will either already have the special 
> handling, or won't operate on the main executable, or likely won't opt
> to use l_addr (vs. dlsym or dli_fbase) or l_name (vs. dli_fname).

Some special-case the main executable based on l_name, I expect, which
is why I'm so reluctant to change l_name.  The GDB comment is actually
hinting strongly towards a "" convention (that Solaris broke).

> I am keeping my hopes low for changing the public semantics, but I
> hope that adding new fields for the missing data at least could fall
> into the "bug fixes" category.

We can in theory add new flags to dladdr1 or new requests to dlinfo.
These things do not impact ABI and are backportable.

Changing struct link_map is challenging, as I explained.  Adding new
functions definitely triggers deployment delays because those aren't
considered backportable.  Adding fields to existing structs is usually
impossible because these structs may have been used to define
application types, and new fields cause subsequent offsets to change.

Thanks,
Flroian