[RFC] bfd/ELF: offset-adjust also the LOAD segment containing the PHDRs
Checks
Commit Message
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.
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).
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.
Comments
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.
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
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
>
@@ -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