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: Tulio Magno Quites Machado Filho <tuliom at linux dot ibm dot com>
- To: "Carlos O'Donell" <carlos at redhat 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>
- Cc:
- Date: Thu, 22 Nov 2018 16:21:30 -0200
- 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>
Carlos O'Donell <carlos@redhat.com> writes:
> On 10/23/18 4:52 PM, Tulio Magno Quites Machado Filho wrote:
> This is my suggested wording. It doesn't block commit, but some of what
> you write needs clarification.
>
> Suggested commit message:
>
> Fix _dl_profile_fixup data-dependency issue (Bug 23690)
>
> There is a data-dependency between the fields of struct l_reloc_result
> and the field used as the initialization guard. Users of the guard
> expect writes to the structure to be observable when they also observe
> the guard initialized. The solution for this problem is to use an acquire
> and release load and store to ensure previous writes to the structure are
> observable if the guard is initialized.
>
> The previous implementation used DL_FIXUP_VALUE_ADDR (l_reloc_result->addr)
> as the initialization guard, making it impossible for some architectures
> to load and store it atomically, i.e. hppa and ia64, due to its larger size.
>
> This commit adds an unsigned int to l_reloc_result to be used as the new
> initialization guard of the struct, making it possible to load and store
> it atomically in all architectures. The fix ensures that the values
> observed in l_reloc_result are consistent and do not lead to crashes.
> The algorithm is documented in the code in elf/dl-runtime.c
> (_dl_profile_fixup). Not all data races have been eliminated.
Fixed.
>> /* 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.
> 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?
>> +#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.
--
Tulio Magno