Re: [PATCH v10 2/4] elf: Fix initial-exec TLS access on audit modules (BZ #28096)

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

On 18/01/2022 08:33, Florian Weimer wrote:
> * Adhemerval Zanella via Libc-alpha:
> 
>> diff --git a/elf/dl-tls.c b/elf/dl-tls.c
>> index 8ba70c9a9d..8ed91ff599 100644
>> --- a/elf/dl-tls.c
>> +++ b/elf/dl-tls.c
>> @@ -520,7 +520,7 @@ _dl_resize_dtv (dtv_t *dtv, size_t max_modid)
>>  
>>  
>>  void *
>> -_dl_allocate_tls_init (void *result)
>> +_dl_allocate_tls_init (void *result, bool init_tls)
>>  {
>>    if (result == NULL)
>>      /* The memory allocation failed.  */
>> @@ -593,7 +593,14 @@ _dl_allocate_tls_init (void *result)
>>  	     some platforms use in static programs requires it.  */
>>  	  dtv[map->l_tls_modid].pointer.val = dest;
>>  
>> -	  /* Copy the initialization image and clear the BSS part.  */
>> +	  /* Copy the initialization image and clear the BSS part.  For
>> +	     audit modules or depedencies with initial-exec TLS, we can not
>> +	     set the initial TLS image on default loader initialization
>> +	     because it would already be set by the audit setup.  However,
>> +	     subsequent thread creation would need to follow the default
>> +	     behaviour.   */
>> +	  if (__glibc_unlikely (map->l_auditing && !init_tls))
>> +	    continue;
>>  	  memset (__mempcpy (dest, map->l_tls_initimage,
>>  			     map->l_tls_initimage_size), '\0',
>>  		  map->l_tls_blocksize - map->l_tls_initimage_size);
> 
> I don't understand why the map->l_auditing condition is correct.
> Shouldn't it be
> 
>   map->l_ns == LM_ID_BASE && !init_tls
> 
> ?  Everything else has been loaded via dlopen or dlmopen, so TLS
> initialization for the main thread has already happened.

This is not suffice (I tried before in fact) because we need to skip
solely for the audit modules and dependencies at the startup.  For
instance with tst-audit21 we have with some _dl_debug_printf to 
show what is happening

   1533769:     [rtld.c:966] load_audit_module
   1533769:     [rtld.c:973] load_audit_module
   1533769:     [rtld.c:2465] dl_main
   1533769:     [../elf/dl-tls.c:603] map->l_name= map->l_ns=0 map->l_auditing=0 init_tls=0 [INIT]
   1533769:     [../elf/dl-tls.c:603] map->l_name=[...]/elf/tst-auditmod21a.so map->l_ns=1 map->l_auditing=1 init_tls=0
   1533769:     [../elf/dl-tls.c:603] map->l_name=[...]/libc.so.6 map->l_ns=1 map->l_auditing=1 init_tls=0
   1533769:     [../elf/dl-tls.c:603] map->l_name=[...]/libc.so.6 map->l_ns=0 map->l_auditing=0 init_tls=0 [INIT]
   1533769:     [../elf/dl-tls.c:621]
   1533769:     [rtld.c:2467] dl_main
[allocatestack.c:435] allocate_stack
   1533769:     [../elf/dl-tls.c:633] _dl_allocate_tls
   1533769:     [../elf/dl-tls.c:603] map->l_name= map->l_ns=0 map->l_auditing=0 init_tls=1 [INIT]
   1533769:     [../elf/dl-tls.c:603] [...]/elf/tst-auditmod21a.so map->l_ns=1 map->l_auditing=1 init_tls=1 [INIT]
   1533769:     [../elf/dl-tls.c:603] [...]/libc.so.6 map->l_ns=1 map->l_auditing=1 init_tls=1 [INIT]
   1533769:     [../elf/dl-tls.c:603] [...]/libc.so.6 map->l_ns=0 map->l_auditing=0 init_tls=1 [INIT]
   1533769:     [../elf/dl-tls.c:621]
   1533769:     [../elf/dl-tls.c:642] _dl_allocate_tls
[allocatestack.c:446] allocate_stack


So we need to avoid initialization only for the libc.so loading by audit libraries, 
the default one should still be initialized (marked as [INIT]).  Later when new
threads are created we need to initialized everything.