This is the mail archive of the
libc-alpha@sourceware.org
mailing list for the glibc project.
Re: [PATCHv5] Protect _dl_profile_fixup data-dependency order [BZ #23690]
- From: Carlos O'Donell <carlos at redhat dot com>
- To: Tulio Magno Quites Machado Filho <tuliom at linux dot ibm dot com>, Florian Weimer <fw at deneb dot enyo dot de>, libc-alpha at sourceware dot org, John David Anglin <dave dot anglin at bell dot net>, Adhemerval Zanella <adhemerval dot zanella at linaro dot org>, Joseph Myers <joseph at codesourcery dot com>, Florian Weimer <fweimer at redhat dot com>
- Date: Thu, 29 Nov 2018 09:28:32 -0500
- Subject: Re: [PATCHv5] Protect _dl_profile_fixup data-dependency order [BZ #23690]
- References: <15c11c67-09df-e7c0-3d4a-e5f774ce17fe@redhat.com> <20181023205204.641-1-tuliom@linux.ibm.com> <99867270-63cd-a7ad-4039-02f2f36e56cf@redhat.com> <87va4px9at.fsf@linux.ibm.com>
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.