This is the mail archive of the
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: <email@example.com> <firstname.lastname@example.org> <email@example.com> <firstname.lastname@example.org>
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.
>> 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.
>> 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.
> 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)
>> 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. */
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 :-)