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

[Florian Weimer]

* Adhemerval Zanella:

> 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

Not sure if this is what I meant, the “continue” inverts the condition.

The initialization should happen for pthread_create or l_ns == LM_ID_BASE.

>> ?  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.

That's what I tried to express with the LM_ID_BASE check.

Thanks,
Florian