[PATCH RFC] bfd/ELF: offset-adjust also the LOAD segment containing the PHDRs

[Jan Beulich]

On 23.12.2024 02:08, Alan Modra wrote:
> On Fri, Dec 20, 2024 at 02:20:49PM +0100, Jan Beulich wrote:
>> PR binutils/32324
>>
>> Such segments have their file offsets set early. Include the subsequent
>> offset adjustment also there, to maintain proper alignment (as mandated
>> by the input binary).
>> ---
>> RFC: I can't really explain the m->p_align_valid part of the
>>      conditional, other than it (or something with the same overall
>>      effect) being needed to make the ld-elf/size-2 check pass. Without
>>      that extra check the program headers there move by 0x18 bytes. The
>>      resulting binary still looks legitimate, just a little odd in
>>      having an 24-byte gap _before_ the PHDRs rather than after.
> 
> I think it's fine to adjust the testcase.  And putting the gap before
> PHDRS makes for a smaller LOAD segment.
> 
>>      Overall the change still feels like a gross hack, yet considering
>>      how convoluted the entire function is, that's surely not the first
>>      one there. I'm in trouble here conceptually anyway: When objcopy-
>>      ing a fully linked binary, it feels wrong to see VAs be calculated
>>      from scratch. VAs may not change in this process, after all (and
>>      that they did in the reported case appears to support my view).
> 
> Yes, vma changes are suspect.  However, rewriting file offsets may
> be required if we allow objcopy/strip to repack the file.
> 
>> RFC: Having a testcase for this situation would be nice, but ld won't
>>      permit creating an ELF binary with the problematic properties: It
>>      demands that if the PHDRs are part of any LOAD segment, they also
>>      be part of any earlier ones.
>>
>> --- a/bfd/elf.c
>> +++ b/bfd/elf.c
>> @@ -6057,6 +6057,9 @@ assign_file_positions_for_load_sections
>>  		  == ((off + off_adjust) & -maxpagesize)))
>>  	    off_adjust += maxpagesize;
>>  	  off += off_adjust;
>> +	  if (m == phdr_load_seg && !m->includes_filehdr && m->p_align_valid)
>> +	    p->p_offset += off_adjust;
>> +
>>  	  if (no_contents)
>>  	    {
>>  	      /* We shouldn't need to align the segment on disk since
> 
> I prefer the following fix, which is equivalent to yours without the
> p_align_valid test.  The improvement is keeping all the p_offset
> assignments together.

Ah yes, that looks neater indeed. Thanks for looking into this.

Jan

> commit 49c701d4c667c9bd60c6a707db9fd5f44d2d239e
> Author: Alan Modra <amodra@gmail.com>
> Date:   Mon Dec 23 10:26:02 2024 +1030
> 
>     PR 32324, Stripping BOLT'ed binaries leads to unwanted behaviour
>     
>     This patch corrects layout for a PT_LOAD header that doesn't include
>     the ELF file header but does contain PHDRs and sections requiring
>     alignment.  The required alignment (which was missing) is placed
>     before the PHDRs.
> 
> diff --git a/bfd/elf.c b/bfd/elf.c
> index 78394319bf0..9f5ac6384f3 100644
> --- a/bfd/elf.c
> +++ b/bfd/elf.c
> @@ -5978,11 +5978,7 @@ assign_file_positions_for_load_sections (bfd *abfd,
>  	p->p_align = 1 << bed->s->log_file_align;
>  
>        if (m == phdr_load_seg)
> -	{
> -	  if (!m->includes_filehdr)
> -	    p->p_offset = off;
> -	  off += actual * bed->s->sizeof_phdr;
> -	}
> +	off += actual * bed->s->sizeof_phdr;
>  
>        no_contents = false;
>        off_adjust = 0;
> @@ -6131,6 +6127,7 @@ assign_file_positions_for_load_sections (bfd *abfd,
>  	    {
>  	      if (p->p_type == PT_LOAD)
>  		{
> +		  p->p_offset = off - actual * bed->s->sizeof_phdr;
>  		  elf_elfheader (abfd)->e_phoff = p->p_offset;
>  		  if (m->count > 0)
>  		    {
> @@ -6141,6 +6138,9 @@ assign_file_positions_for_load_sections (bfd *abfd,
>  		}
>  	      else if (phdr_load_seg != NULL)
>  		{
> +		  /* Also set PT_PHDR to match phdr_load_seg.  We've
> +		     sorted segments so that phdr_load_seg will
> +		     already be set by the code immediately above.  */
>  		  Elf_Internal_Phdr *phdr = phdrs + phdr_load_seg->idx;
>  		  bfd_vma phdr_off = 0;  /* Octets.  */
>  		  if (phdr_load_seg->includes_filehdr)
> diff --git a/ld/testsuite/ld-elf/size-2.d b/ld/testsuite/ld-elf/size-2.d
> index 9f1a9cf48fa..e3e28cf7a93 100644
> --- a/ld/testsuite/ld-elf/size-2.d
> +++ b/ld/testsuite/ld-elf/size-2.d
> @@ -13,8 +13,8 @@
>  .* \.tbss +NOBITS +0+130 [0-9a-f]+ 0+30 00 WAT .*
>  .* \.map +PROGBITS +0+130 [0-9a-f]+ 0+c 00 +A .*
>  #...
> - +PHDR +(0x0+40 0x0+40 0x0+40 0x0+a8 0x0+a8|0x0+34 0x0+34 0x0+34 0x0+60 0x0+60|0x0+34 0x0+a0 0x0+a0 0x0+60 0x0+60) R .*
> - +LOAD +(0x0+40 0x0+40 0x0+40 0x0+fc 0x0+fc|0x0+34 0x0+34 0x0+34 0x0+1(08|10) 0x0+1(08|10)|0x0+34 0x0+a0 0x0+a0 0x0+9c 0x0+9c) R E .*
> + +PHDR +(0x0+58 0x0+58 0x0+58 0x0+a8 0x0+a8|0x0+(a0|34) 0x0+a0 0x0+a0 0x0+60 0x0+60) R .*
> + +LOAD +(0x0+58 0x0+58 0x0+58 0x0+e4 0x0+e4|0x0+(a0|34) 0x0+a0 0x0+a0 0x0+(9c|a0) 0x0+(9c|a0)) R E .*
>   +TLS +0x0+(110|a4) 0x0+110 0x0+110 0x0+20 0x0+50 R .*
>  #...
>  .* \.text \.tdata \.map 
>