Re: [PATCH 07/15] elf: Refactor _dl_update_slotinfo to avoid use after free

[Szabolcs Nagy <szabolcs.nagy@arm.com>]

The 04/06/2021 16:40, Adhemerval Zanella wrote:
> On 15/02/2021 09:00, Szabolcs Nagy via Libc-alpha wrote:
> > map is not valid to access here because it can be freed by a
> > concurrent dlclose, so don't check the modid.
> 
> Won't it be protected by the recursive GL(dl_load_lock) in such case?
> I think the concurrency issue is between dlopen and _dl_deallocate_tls
> called by pthread stack handling (nptl/allocatestack.c).  Am I missing
> something here?


_dl_update_slotinfo is called both with and without
the dlopen lock held: during dynamic tls access
the lock is not held (see the __tls_get_addr path)

we cannot add a lock there because that would cause
new deadlocks, dealing with this is the tricky part
of the patchset.

> > 
> > The map == 0 and map != 0 code paths can be shared (avoiding
> > the dtv resize in case of map == 0 is just an optimization:
> > larger dtv than necessary would be fine too).
> > ---
> >  elf/dl-tls.c | 21 +++++----------------
> >  1 file changed, 5 insertions(+), 16 deletions(-)
> > 
> > diff --git a/elf/dl-tls.c b/elf/dl-tls.c
> > index 24d00c14ef..f8b32b3ecb 100644
> > --- a/elf/dl-tls.c
> > +++ b/elf/dl-tls.c
> > @@ -743,6 +743,8 @@ _dl_update_slotinfo (unsigned long int req_modid)
> >  	{
> >  	  for (size_t cnt = total == 0 ? 1 : 0; cnt < listp->len; ++cnt)
> >  	    {
> > +	      size_t modid = total + cnt;
> > +
> >  	      size_t gen = listp->slotinfo[cnt].gen;
> >  
> >  	      if (gen > new_gen)
> > @@ -758,25 +760,12 @@ _dl_update_slotinfo (unsigned long int req_modid)
> >  
> >  	      /* If there is no map this means the entry is empty.  */
> >  	      struct link_map *map = listp->slotinfo[cnt].map;
> > -	      if (map == NULL)
> > -		{
> > -		  if (dtv[-1].counter >= total + cnt)
> > -		    {
> > -		      /* If this modid was used at some point the memory
> > -			 might still be allocated.  */
> > -		      free (dtv[total + cnt].pointer.to_free);
> > -		      dtv[total + cnt].pointer.val = TLS_DTV_UNALLOCATED;
> > -		      dtv[total + cnt].pointer.to_free = NULL;
> > -		    }
> > -
> > -		  continue;
> > -		}
> > -
> >  	      /* Check whether the current dtv array is large enough.  */
> > -	      size_t modid = map->l_tls_modid;
> > -	      assert (total + cnt == modid);
> >  	      if (dtv[-1].counter < modid)
> >  		{
> > +		  if (map == NULL)
> > +		    continue;
> > +
> >  		  /* Resize the dtv.  */
> >  		  dtv = _dl_resize_dtv (dtv);
> >  
> >