Re: A collection of LD_AUDIT bugs that are important for tools (with better formatting for this list)

[Ben Woodard <woodard@redhat.com>]

> On Jun 17, 2021, at 4:06 PM, Adhemerval Zanella <adhemerval.zanella@linaro.org> wrote:
> 
> 
> 
> On 17/06/2021 17:09, Florian Weimer wrote:
>> * Adhemerval Zanella:
>> 
>>> 
>>> 
>>> * SVE support: as indicated by Szabolcs SVE calls are marked with the 
>>>  STO_AARCH64_VARIANT_PCS and thus explicit not supported by dynamic loader. 

To me this sounds like partly a toolchain issue. The aarch64 PCS does define the ABI for SVE calls. I haven’t checked what GCC/binutils does in quite a while. It seems like the  STO_AARCH64_VARIANT_PCS was an expedient for SVE when it first came out and the semantics that it defines where all the registers are caller preserved makes it very difficult to implement around.

For LAV_CURRENT=2 what I planned to do was:

diff --git a/sysdeps/aarch64/bits/link.h b/sysdeps/aarch64/bits/link.h
index ca76087ee1..390b12a826 100644
--- a/sysdeps/aarch64/bits/link.h
+++ b/sysdeps/aarch64/bits/link.h
@@ -20,13 +20,24 @@
 # error "Never include <bits/link.h> directly; use <link.h> instead."
 #endif
 
+typedef struct La_sve_regs {
+  uint16_t    *lr_preg[3];
+  __uint128_t *lr_zreg[8];
+} La_sve_regs;
+
 /* Registers for entry into PLT on AArch64.  */
 typedef struct La_aarch64_regs
 {
   uint64_t    lr_xreg[9];
-  __uint128_t lr_vreg[8];
   uint64_t    lr_sp;
   uint64_t    lr_lr;
+  char    lr_sve; /* 0 - no SVE,
+                        1-16 length of the SVE registers in vq (128 bits) */
+  union {
+    /* when sve!=0 accessing the lr_vreg is undefined */
+    __uint128_t lr_vreg[8];
+    La_sve_regs lr_zreg;
+  };
 } La_aarch64_regs;
 
 /* Return values for calls from PLT on AArch64.  */
@@ -34,9 +45,14 @@ typedef struct La_aarch64_retval
 {
   /* Up to eight integer registers can be used for a return value.  */
   uint64_t    lrv_xreg[8];
-  /* Up to eight V registers can be used for a return value.  */
-  __uint128_t lrv_vreg[8];
-
+  char        lrv_sve; /* 0 - no SVE,
+                         1-16 length of the SVE registers in vq (128 bits) */
+  union{
+    /* Up to eight V registers can be used for a return value.
+       When sve!=0 accessing the lr_vreg is undefined */
+    __uint128_t lrv_vreg[8];
+    La_sve_regs lrv_zreg;
+  };
 } La_aarch64_retval;
 __BEGIN_DECLS
 
However, that would require toolchain support and another hint in st_other which separates SVE calls from other uses of STO_AARCH64_VARIANT_PCS like STO_AARCH64_VARIANT_SVE. Then the runtime linker could populate the lrv_sve with information from the lrv_vreg with the size of the vector registers from the processor’s registers. 

There are at least two problems with that approach. 
1) who allocates the lr_zreg pointers in the la_sve_regs and how long should they be? Do they always have to be allocated to be the max size 2048 bits?
2) I hadn’t worked out how to handle functions that return things in the SVE regs. Do we need two new flags in st_other? STO_AARCH64_VARIANT_SVE and STO_AARCH64_VARIANT_SVERET?

Then there was the question in the future could there be: big.LITTLE processors where some big processors had SVE registers of one length while the LITTLE processors had different ones? 

I kind of got into the: I don’t know what to do zone.


>>>  To support SVE, it would require save/restore all registers (but pass down 
>>>  arg and retval registers to pltentry/exit callbacks according to the base 
>>>  PCS). Another option would be to use different LAV_CURRENT version, one for
>>>  NEON and SVE with 128-bits and another for SVE larger than 256-bits.
>>>  This also has performance implications.
>> 
>> SVE support overlaps with the la_symbind issue quoted above because
>> those bindings are conceptually BIND_NOW.
> 
> There is additional issues where we need to define whether we will use
> a hacky solution to work around the ABI limitation or if we can design
> something between.  And it also has the performance implications, it
> will be hard not to notice potentially multiple KBs of load/store on
> each PLT call.
> 
>> 
>>> [1] https://patchwork.sourceware.org/project/glibc/patch/20200923160448.2321909-1-woodard@redhat.com/
>>> [2] https://sourceware.org/pipermail/libc-alpha/2020-September/117822.html
>>> 
>>>> ----------------------------------------------------------
>>>> LOW        | When auditing, a dlopen of a shared library
>>>>           | that uses R_X86_64_TLSDESC causes a SEGV. This
>>>>           | is reportedly fixed in glibc 2.34.
>>> 
>>> It should be fixed by BZ#27137 (8f7e09f4dbdb5c815a18b8285fbc5d5d7bc17d86),
>>> however it has regressed by 03e187a41d9.  We need the following fix and we
>>> definitely need a regression tests for this (I will probably use your
>>> auditor-test) as base:
>>> 
>>> diff --git a/elf/dl-open.c b/elf/dl-open.c
>>> index d2240d8747..d2638ebf05 100644
>>> --- a/elf/dl-open.c
>>> +++ b/elf/dl-open.c
>>> @@ -771,7 +771,7 @@ dl_open_worker (void *a)
>>>     {
>>>       struct link_map *libc_map = GL(dl_ns)[args->nsid].libc_map;
>>> #ifdef SHARED
>>> -      bool initial = libc_map->l_ns == LM_ID_BASE;
>>> +      bool initial = libc_map != NULL ? libc_map->l_ns == LM_ID_BASE : false;
>>> #else
>>>       /* In the static case, there is only one namespace, but it
>>>         contains a secondary libc (the primary libc is statically
>> 
>> Hmm, I'm surprised that this doesn't crash more widely.  Is it because
>> DT_NEEDED on ld.so preceeds libc.so if __tls_get_addr is used, or
>> something like that?
> 
> I will need to check why we haven't see it with our own testcase.
> I in case I will prepare a patch to fix it.
> 
>> 
>>> And you are correct about your assessment on the document, we *really* need
>>> more extensible tests for audit interface.  It would be really helpful if
>>> we could add more tests to exercise the real world usage from tools that
>>> rely on audit modules.
>> 
>> Agreed.  It would also be helpful to verify that the callbacks happened
>> in the expected order.  This is always a troublesome aspect of a
>> callback-based interface.
>> 
>>>> -  We want to use LD_AUDIT’s la_symbind32 and la_symbind64 to interpose
>>>>   wrappers around key functions, e.g. pthread_create. This enables a
>>>>   tool to intercept functions invoked through pointers obtained with
>>>>   dlsym, which preloaded wrappers can’t do. (Note: We don’t use
>>>>   la_symbind for interposition yet, but we plan to when it works
>>>>   everywhere.)
>>>> 
>>>> -  We need auditing to work when an application or a tool library (e.g.,
>>>>   Intel’s GT-Pin) opens a shared library with dlmopen.
>>>> 
>>>> -  We need auditing to work when opening a dynamic library with TLS
>>>>   dialect gnu2 relocations on x86_64 (R_X86_64_TLSDESC). We don’t have
>>>>   any special interest in such relocations; at present, they cause a
>>>>   SEGV when auditing and that must be avoided.
>>> 
>>> This should be fixed minus the regression above.
>> 
>> pthread_create interception becomes more difficult in glibc 2.34 because
>> the pthread_create symbol is no longer interposable.
>> 
>> We could make pthread_create interposable in the same we way do that
>> today for malloc.
> 
> But it is only a handfull call internall (POSIX timer for instance).
> I don't think
> 
>> 
>> pthread_create interposition on glibc 2.0 architectures needs version
>> information in la_symbind.  No 64-bit architecture except Alpha is that
>> old, so maybe this doesn't matter today.  (libstdc++ currently binds to
>> the pthread_create compatibility symbol on these architectures, so the
>> issue is quite visible, but a rebuild of GCC against glibc 2.34 will fix
>> that, too.  Then the default version will be used.)
>> 
>> One caveat: We should be able to make la_symbind work on current
>> distributions with their hardened build defaults, *however* PLT
>> enter/exit hooks will not work.  For the time being, you would have to
>> find a way to wrap the call yourself.  This is theoretically fixable
>> even without run-time code generation (Madhavan T. Venkataraman from
>> Microsoft has submitted patches in this area for libffi), but it's
>> entirely vaporware at this point.  This looks like a fairly isolated
>> project someone could work on without spending considerable time on
>> learning the internals of the dynamic loader.
> 
> Is this what your are referring [1]? It is not clear to me how the v2 of this
> does not require a trip to the kernel, does it use a vDSO to place the 
> trampolines (also the libffi.v2.txt link is broken)?
> 
> In any case, do you think it should be fixable by adding trampolines on
> extra mmap memory? 
> 
> [1] https://lkml.org/lkml/2020/9/16/643
> 
>> 
>>>> -  We want to add an auditor to an application at link time, noted in the
>>>>   DT_AUDIT entry of the dynamic section. Loading the DT_AUDIT entry as a
>>>>   program is launched enables our profiler to be injected into an
>>>>   application’s address space without a wrapper script that sets
>>>>   LD_AUDIT and LD_PRELOAD.
>>> 
>>> So if I understood correctly you are asking something like a DT_FILTER
>>> for DT_AUDIT?
>> 
>> Indeed, please let us know if the existing DT_AUDIT support does not
>> work for you.
>> 
>> Thanks,
>> Florian