Re: [PATCH] PR ld/16322: ld fails to generate GNU_RELRO segment

["H.J. Lu" <hjl dot tools at gmail dot com>]

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.