[PATCHv5] Protect _dl_profile_fixup data-dependency order [BZ #23690]

[Carlos O'Donell]

On 11/22/18 1:21 PM, Tulio Magno Quites Machado Filho wrote:
>>>    /* By default we do not call the pltexit function.  */
>>>    long int framesize = -1;
>>>  
>>> +
>>>  #ifdef SHARED
>>>    /* Auditing checkpoint: report the PLT entering and allow the
>>>       auditors to change the value.  */
>>> -  if (DL_FIXUP_VALUE_CODE_ADDR (value) != 0 && GLRO(dl_naudit) > 0
>>> +  if (init != 0 && GLRO(dl_naudit) > 0
>>
>> This bit worries me, and took most of my review time to think up and
>> review surrounding code.
>>
>> Isn't 'init != 0' always going to be true?
>>
>> Up above if it's 0 then we do the initialization, and set it to 1.
>>
>> Otherwise it's non-zero and we load value.
>>
>> In both cases it's non-zero by the time we reach here.
> 
> Indeed.
> 
>> The previous check had some interesting side-effects in that if value
>> was relocated to a NULL value, we would skip running the auditor.
> 
> I haven't seen this happening yet.

OK.

>> The test here is probably not about the initialization guard, but
>> rather if value is non-NULL then run the auditor.
>>
>> I think this needs restoring to 'DL_FIXUP_VALUE_CODE_ADDR (value) != 0'
>>
>> What do you think?
> 
> AFAICS, the same analysis applies to 'DL_FIXUP_VALUE_CODE_ADDR (value)'.
> It will never be 0 at this point.

Agreed.

> I think we can safely add 'assert (DL_FIXUP_VALUE_CODE_ADDR (value) != 0)' and
> remove both tests for init and DL_FIXUP_VALUE_CODE_ADDR (value) from here.
> 
> What do you think?

That's fine with me.

>>> +#ifdef callnum
>>> +# define FUNC(x) CONCAT (retNum, x) (); sync_all (x)
>>> +#endif
>>> +
>>
>> Please add:
>>
>> /* A value of 7000 functions is chosen as an arbitrarily large
>>    number of functions that will allow us enough attempts to
>>    verify lazy resolution operation.  */
> 
> Fixed.
> 

Please post a v6 and I'll do one final check over it and then we'll commit.

I'm eager to see this fixed for our downstream users that reported this bug :-)

-- 
Cheers,
Carlos.