Re: [PATCH v7 12/16] elf: Fix initial-exec TLS access on audit modules (BZ #28096)

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

On 24/12/2021 14:17, Florian Weimer wrote:
> * Adhemerval Zanella:
> 
>> For audit modules or dependencies 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.
>>
>> This patch fixes by making_dl_new_object sets a new link_map flag
>> 'l_dont_set_tls_static' for namespaces different than the default
>> one.  The flag is later reset on _dl_allocate_tls_init.
> 
> Typo: making[ ]dl_new-object

Ack.

> 
>> diff --git a/elf/dl-object.c b/elf/dl-object.c
>> index 1875599eb2..f3d637ac27 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 (nsid != LM_ID_BASE)
>> +    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/dl-tls.c b/elf/dl-tls.c
>> index 273f60f233..89200ccc48 100644
>> --- a/elf/dl-tls.c
>> +++ b/elf/dl-tls.c
>> @@ -593,10 +593,18 @@ _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.  */
>> -	  memset (__mempcpy (dest, map->l_tls_initimage,
>> -			     map->l_tls_initimage_size), '\0',
>> -		  map->l_tls_blocksize - map->l_tls_initimage_size);
>> +	  /* 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_dont_set_tls_static))
>> +	    memset (__mempcpy (dest, map->l_tls_initimage,
>> +			       map->l_tls_initimage_size), '\0',
>> +		    map->l_tls_blocksize - map->l_tls_initimage_size);
>> +	  else
>> +	    map->l_dont_set_tls_static = 0;
>>  	}
> 
> Wouldn't it be simpler to skip the memcpy/memset for non-base namespaces
> if a flag is pased to _dl_allocate_tls_init?  I still don't think it's
> the status as an audit module what matters here, it applies to
> everything that is loaded through the dlopen implementation (as opposed
> to _dl_map_object_deps for the main executable).

It does make sense, and I think the extra argument flag is not really
required (it requires only to check if the map namespace is no the
base one).  I will change and resend it.