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 10:02, Florian Weimer wrote:
> * 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.

The problem is 'map->l_ns == LM_ID_BASE && !init_tls' will not initilize
the required libc.so for main application during audit module loading.
With tst-audit21:

   1593586:     [rtld.c:965] load_audit_module
   1593586:     [rtld.c:972] load_audit_module
   1593586:     [rtld.c:2464] dl_main
   1593586:     [../elf/dl-tls.c:605] map->l_name= map->l_ns=0 map->l_auditing=0 init_tls=0
   1593586:     [../elf/dl-tls.c:610] map->l_name=[...]/elf/tst-auditmod21a.so map->l_ns=1 map->l_auditing=1 init_tls=0 [INIT]
   1593586:     [../elf/dl-tls.c:610] map->l_name=[...]/libc.so.6 map->l_ns=1 map->l_auditing=1 init_tls=0 [INIT]
   1593586:     [../elf/dl-tls.c:605] map->l_name=[...]/libc.so.6 map->l_ns=0 map->l_auditing=0 init_tls=0
   1593586:     [../elf/dl-tls.c:627]
   1593586:     [rtld.c:2466] dl_main
Segmentation fault (core dumped)