[PATCH] PR ld/16322: ld fails to generate GNU_RELRO segment
H.J. Lu
hjl.tools@gmail.com
Thu Jan 9 16:28:00 GMT 2014
On Thu, Jan 9, 2014 at 6:50 AM, Alan Modra <amodra@gmail.com> wrote:
> On Thu, Jan 09, 2014 at 04:08:17AM -0800, H.J. Lu wrote:
>> On Wed, Jan 8, 2014 at 8:13 PM, Alan Modra <amodra@gmail.com> wrote:
>> > On Wed, Jan 08, 2014 at 06:01:49AM -0800, H.J. Lu wrote:
>> >> >> + /* If PT_LOAD segment doesn't fit PT_GNU_RELRO segment,
>> >> >> + adjust its p_filesz and p_memsz. */
>> >
>> > This is broken. What if there is another LOAD segment following
>> > the one you're adjusting?
>>
>> It will only happen with a costumer linker script using
>> DATA_SEGMENT_ALIGN, DATA_SEGMENT_RELRO_END,
>> DATA_SEGMENT_END.
>
> Right. Such a script is possible.
>
>> Do you have a testcase?
>
> No.
>
>> We can
>> either ignore relro or issue an error.
>
> Please fix it one way or the other.
>
This patch issues an error if there is another PT_LOAD
segment:
diff --git a/bfd/elf.c b/bfd/elf.c
index 870e281..452cae0 100644
--- a/bfd/elf.c
+++ b/bfd/elf.c
@@ -4816,6 +4816,22 @@ assign_file_positions_for_load_sections (bfd *abfd,
if (p->p_vaddr + p->p_filesz < relro_end)
{
bfd_vma adjust = relro_end - (p->p_vaddr + p->p_filesz);
+ /* This won't work if there is another PT_LOAD segment
+ after this PT_LOAD segment. */
+ struct elf_segment_map *mn;
+ for (mn = m->next; mn != NULL; mn = mn->next)
+ if (mn->p_type == PT_LOAD)
+ break;
+ if (mn != NULL)
+ {
+ _bfd_error_handler
+ (_("%B: 2 PT_LOAD segments overlap due to PT_GNU_RELRO segment"),
+ abfd);
+ print_segment_map (m);
+ print_segment_map (mn);
+ bfd_set_error (bfd_error_bad_value);
+ return FALSE;
+ }
p->p_filesz += adjust;
off += adjust;
}
>> >> >> if (expld.dataseg.base - (1 << max_alignment_power) < old_base)
>> >> >> expld.dataseg.base += expld.dataseg.pagesize;
>> >> >> - expld.dataseg.base -= (1 << max_alignment_power);
>> >> >> + /* Properly align base to max_alignment_power. */
>> >> >> + expld.dataseg.base &= ~((1 << max_alignment_power) - 1);
>> >> >> lang_reset_memory_regions ();
>> >> >> one_lang_size_sections_pass (relax, check_regions);
>> >
>> > This also doesn't look correct to me. Please explain why you think
>> > this is a good idea.
>>
>> There are
>>
>> if (expld.dataseg.relro_end > relro_end)
>> {
>> /* The alignment of sections between DATA_SEGMENT_ALIGN
>> and DATA_SEGMENT_RELRO_END caused huge padding to be
>> inserted at DATA_SEGMENT_RELRO_END. Try to start a bit lower so
>> that the section alignments will fit in. */
>> asection *sec;
>> unsigned int max_alignment_power = 0;
>>
>> /* Find maximum alignment power of sections between
>> DATA_SEGMENT_ALIGN and DATA_SEGMENT_RELRO_END. */
>> for (sec = link_info.output_bfd->sections; sec; sec = sec->next)
>> if (sec->vma >= expld.dataseg.base
>> && sec->vma < expld.dataseg.relro_end
>> && sec->alignment_power > max_alignment_power)
>> max_alignment_power = sec->alignment_power;
>>
>> if (((bfd_vma) 1 << max_alignment_power) < expld.dataseg.pagesize)
>> {
>> if (expld.dataseg.base - (1 << max_alignment_power) < old_base)
>> expld.dataseg.base += expld.dataseg.pagesize;
>> /* Properly align base to max_alignment_power. */
>> expld.dataseg.base &= ~((1 << max_alignment_power) - 1);
>> lang_reset_memory_regions ();
>> one_lang_size_sections_pass (relax, check_regions);
>> }
>> }
>> link_info.relro_start = expld.dataseg.base;
>> link_info.relro_end = expld.dataseg.relro_end;
>>
>> max_alignment_power is the maximum alignment of sections
>> between DATA_SEGMENT_ALIGN and DATA_SEGMENT_RELRO_END.
>> If expld.dataseg.base (link_info.relro_start) isn't aligned to
>> max_alignment_power, some sections won't fit into RELRO
>> segment. If you have a testcase to show it doesn't some
>> cases correctly, I will fix it.
>
> I understand well enough what the code is trying to do, but this code
> is not obvious and it seems to me that you have not spent the time to
> properly understand it before committing unreviewed patches. That's
> why I wanted you to explain your patch.
>
> What caught my eye first is that this adjustment
>
> if (expld.dataseg.base - (1 << max_alignment_power) < old_base)
> expld.dataseg.base += expld.dataseg.pagesize;
>
> no longer makes sense after you changed the following lines to
>
> /* Properly align base to max_alignment_power. */
> expld.dataseg.base &= ~((1 << max_alignment_power) - 1);
>
> That makes your patch wrong, or at least incomplete. I also think
> your comment is worse than useless.
>
> What you need to understand is that the whole point of the relro
> adjustment to dataseg.base is to make the *end* of the relro area
> page aligned. That's what the following is trying to do
> expld.dataseg.base += (-expld.dataseg.relro_end
> & (expld.dataseg.pagesize - 1));
>
> "Properly aligning base" is not what we're doing at all!
>
> We need at least the following fixes..
>
>
> diff --git a/ld/ldlang.c b/ld/ldlang.c
> index 7851615..79b3c4b 100644
> --- a/ld/ldlang.c
> +++ b/ld/ldlang.c
> @@ -5362,18 +5362,14 @@ lang_size_sections (bfd_boolean *relax, bfd_boolean check_regions)
> && link_info.relro && expld.dataseg.relro_end)
> {
> /* If DATA_SEGMENT_ALIGN DATA_SEGMENT_RELRO_END pair was seen, try
> - to put expld.dataseg.relro on a (common) page boundary. */
> - bfd_vma min_base, old_base, relro_end, maxpage;
> + to put expld.dataseg.relro_end on a (common) page boundary. */
> + bfd_vma min_base, relro_end, maxpage;
>
> expld.dataseg.phase = exp_dataseg_relro_adjust;
> maxpage = expld.dataseg.maxpagesize;
> /* MIN_BASE is the absolute minimum address we are allowed to start the
> read-write segment (byte before will be mapped read-only). */
> min_base = (expld.dataseg.min_base + maxpage - 1) & ~(maxpage - 1);
> - /* OLD_BASE is the address for a feasible minimum address which will
> - still not cause a data overlap inside MAXPAGE causing file offset skip
> - by MAXPAGE. */
> - old_base = expld.dataseg.base;
> expld.dataseg.base += (-expld.dataseg.relro_end
> & (expld.dataseg.pagesize - 1));
> /* Compute the expected PT_GNU_RELRO segment end. */
> @@ -5405,9 +5401,9 @@ lang_size_sections (bfd_boolean *relax, bfd_boolean check_regions)
>
> if (((bfd_vma) 1 << max_alignment_power) < expld.dataseg.pagesize)
> {
> - if (expld.dataseg.base - (1 << max_alignment_power) < old_base)
> - expld.dataseg.base += expld.dataseg.pagesize;
> - /* Properly align base to max_alignment_power. */
> + /* Aligning the adjusted base guarantees the padding between
> + sections won't change. FIXME: Doesn't this mean relro end
> + might no longer be page aligned? */
> expld.dataseg.base &= ~((1 << max_alignment_power) - 1);
> lang_reset_memory_regions ();
> one_lang_size_sections_pass (relax, check_regions);
>
This works for me.
Thanks.
--
H.J.
More information about the Binutils
mailing list