Re: [PATCH v5 05/22] elf: Fix initial-exec TLS access on audit modules (BZ #28096)

[Adhemerval Zanella <adhemerval.zanella@linaro.org>]

On 10/11/2021 10:23, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>> diff --git a/elf/Makefile b/elf/Makefile
>> index 4758cb23c4..861e83236e 100644
>> --- a/elf/Makefile
>> +++ b/elf/Makefile
> 
>> +$(objpfx)tst-audit21.out: $(objpfx)tst-auditmod21.so
>> +tst-audit21-ENV = LD_AUDIT=$(objpfx)tst-auditmod21.so
> 
> This is needed to fix a Hurd build failure:
> 
> $(objpfx)tst-audit21: $(shared-thread-library)

Ack.

> 
>> diff --git a/elf/dl-object.c b/elf/dl-object.c
>> index 1875599eb2..eb2158a84b 100644
>> --- a/elf/dl-object.c
>> +++ b/elf/dl-object.c
>> @@ -175,6 +175,9 @@ _dl_new_object (char *realname, const char *libname, int type,
>>  
>>    new->l_local_scope[0] = &new->l_searchlist;
>>  
>> +  if (mode & __RTLD_AUDIT)
>> +    new->l_dont_set_tls_static = 1;
>> +
>>    /* Determine the origin.  If allocating the link map for the main
>>       executable, the realname is not known and "".  In this case, the
>>       origin needs to be determined by other means.  However, in case
> 
>> diff --git a/elf/rtld.c b/elf/rtld.c
>> index 8953347b00..db1817655f 100644
>> --- a/elf/rtld.c
>> +++ b/elf/rtld.c
>> @@ -1055,6 +1055,8 @@ ERROR: audit interface '%s' requires version %d (maximum supported version %d);
>>  
>>    /* Mark the DSO as being used for auditing.  */
>>    dlmargs.map->l_auditing = 1;
>> +  /* Mark the DSO to not clear the TLS bss in tls initialization.  */
>> +  dlmargs.map->l_dont_set_tls_static = 1;
>>  }
>>  
>>  /* Notify the the audit modules that the object MAP has already been
> 
> I'm not sure if this actually works.  As far as I understand it,
> everything in an audit namespace needs this special treatment, and not
> just the audit module itself.

Yes, and since each audit modules is loaded with LM_ID_NEWLM it means
all its dependencies.  Adding some instrumentation, it works on the audit
modules does have some dependencies:

$ ldd /home/azanella/Projects/glibc/build/x86_64-linux-gnu/elf/tst-auditmod21a.so
        linux-vdso.so.1 (0x00007fff4a172000)
        /home/azanella/Projects/glibc/build/x86_64-linux-gnu/elf/tst-auditmod21b.so (0x00007fcd11fa5000)
        libc.so.6 => /lib/x86_64-linux-gnu/libc.so.6 (0x00007fcd11d61000)
        /lib64/ld-linux-x86-64.so.2 (0x00007fcd11fb1000)

$ LD_AUDIT=elf/tst-auditmod21a.so elf/ld.so --library-path . elf/tst-audit21
   2281374:     [../elf/dl-tls.c:603] map->l_name= l_dont_set_tls_static=0
   2281374:     [../elf/dl-tls.c:603] map->l_name=elf/tst-auditmod21a.so l_dont_set_tls_static=1
   2281374:     [../elf/dl-tls.c:603] map->l_name=/home/azanella/Projects/glibc/build/x86_64-linux-gnu/elf/tst-auditmod21b.so l_dont_set_tls_static=1
   2281374:     [../elf/dl-tls.c:603] map->l_name=./libc.so.6 l_dont_set_tls_static=1
   2281374:     [../elf/dl-tls.c:603] map->l_name=/home/azanella/Projects/glibc/build/x86_64-linux-gnu/elf/tst-auditmod21c.so l_dont_set_tls_static=0
   2281374:     [../elf/dl-tls.c:603] map->l_name=./libc.so.6 l_dont_set_tls_static=0
   2281374:     [../elf/dl-tls.c:603] map->l_name= l_dont_set_tls_static=0
   2281374:     [../elf/dl-tls.c:603] map->l_name=elf/tst-auditmod21a.so l_dont_set_tls_static=0
   2281374:     [../elf/dl-tls.c:603] map->l_name=/home/azanella/Projects/glibc/build/x86_64-linux-gnu/elf/tst-auditmod21b.so l_dont_set_tls_static=0
   2281374:     [../elf/dl-tls.c:603] map->l_name=./libc.so.6 l_dont_set_tls_static=0
   2281374:     [../elf/dl-tls.c:603] map->l_name=/home/azanella/Projects/glibc/build/x86_64-linux-gnu/elf/tst-auditmod21c.so l_dont_set_tls_static=0
   2281374:     [../elf/dl-tls.c:603] map->l_name=./libc.so.6 l_dont_set_tls_static=0

The l_dont_set_tls_static should be set for the audit module and its
dependencies.

> 
> I think we shuld add a parameter to _dl_allocate_tls_init and that
> indicates the initialization should only be applied to objects in the
> base namespace.  This way, initialization is also skipped for dlopen'ed
> modules in an audit namespace.

I am not sure if this would be best solution, I will check whether 
dlopen does work with this patch.