Use offsets instead of addresses in ELF_SECTION_IN_SEGMENT

Message ID 20180522105215.3339-1-alan.hayward@arm.com
State New
Headers show
Series
  • Use offsets instead of addresses in ELF_SECTION_IN_SEGMENT
Related show

Commit Message

Alan Hayward May 22, 2018, 10:52 a.m.
Use sh_offset and p_offset instead of sh_addr and p_vaddr when calculating if a section fits in a segment. Both methods are valid when using the GNU linker.

This change makes ELF_SECTION_IN_SEGMENT consistent with BFD _bfd_elf_make_section_from_shdr(), which has used offsets to calculate LMA since 2008.

bfd/elf.c _bfd_elf_make_section_from_shdr():
	      if ((flags & SEC_LOAD) == 0)
		newsect->lma = (phdr->p_paddr
				+ hdr->sh_addr - phdr->p_vaddr);
	      else
		/* We used to use the same adjustment for SEC_LOAD
		   sections, but that doesn't work if the segment
		   is packed with code from multiple VMAs.
		   Instead we calculate the section LMA based on
		   the segment LMA.  It is assumed that the
		   segment will contain sections with contiguous
		   LMAs, even if the VMAs are not.  */
		newsect->lma = (phdr->p_paddr
				+ hdr->sh_offset - phdr->p_offset);

For more details on that change see https://sourceware.org/ml/binutils/2008-05/msg00008.html

The upshot of this change is that, along with gdb patch "[PATCH] Use ELF_SECTION_IN_SEGMENT to map segments", it will allow debugging of baremetal Arm binaries produced by the Arm Compiler. Binaries produced by Armlinker in Arm Compiler uses sh_addr, p_vaddr and p_paddr differently to the GNU linker, but both Armlink and the GNU linker are fully compatible with the ELF spec.

This change has been tested using make check x86 on a binutils+gdb build. It has also been manually tested by using gdb to debug a Arm Mbed board.

Alan.

2018-05-22  Alan Hayward  <alan.hayward@arm.com>

	* include/elf/internal.h (ELF_SECTION_IN_SEGMENT): Use offsets
	instead of addresses.
---
 include/elf/internal.h | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

-- 
2.15.1 (Apple Git-101)

Comments

Alan Modra May 22, 2018, 2:04 p.m. | #1
On Tue, May 22, 2018 at 11:52:15AM +0100, Alan Hayward wrote:
> Use sh_offset and p_offset instead of sh_addr and p_vaddr when calculating if a section fits in a segment. Both methods are valid when using the GNU linker.

> 

> This change makes ELF_SECTION_IN_SEGMENT consistent with BFD _bfd_elf_make_section_from_shdr(), which has used offsets to calculate LMA since 2008.


LMA is not VMA.  The code you referenced doesn't really support the
change you propose, nor did you say exactly why the change is needed.

> diff --git a/include/elf/internal.h b/include/elf/internal.h

> index 05f9fab89c..5d93e0070e 100644

> --- a/include/elf/internal.h

> +++ b/include/elf/internal.h

> @@ -347,9 +347,9 @@ struct elf_segment_map

>         || ((sec_hdr)->sh_flags & SHF_ALLOC) == 0			\

>         || ((sec_hdr)->sh_addr >= (segment)->p_vaddr			\

>  	   && (!(strict)						\

> -	       || ((sec_hdr)->sh_addr - (segment)->p_vaddr		\

> +	       || ((sec_hdr)->sh_offset - (segment)->p_offset		\

>  		   <= (segment)->p_memsz - 1))				\

> -	   && (((sec_hdr)->sh_addr - (segment)->p_vaddr			\

> +	   && (((sec_hdr)->sh_offset - (segment)->p_offset		\

>  		+ ELF_SECTION_SIZE(sec_hdr, segment))			\

>  	       <= (segment)->p_memsz)))					\

>     /* No zero size sections at start or end of PT_DYNAMIC.  */		\


We test sh_offset already, just above the code you are patching.  So I
think this VMA test should continue to test sh_addr.

Now there are reasons you might want to disable the VMA checks
entirely, for example when overlays are present.  To do that, use
ELF_SECTION_IN_SEGMENT_1 with check_vma false.  See the example in
elf.c assign_files_positions_for_load_sections.

-- 
Alan Modra
Australia Development Lab, IBM
Alan Hayward May 23, 2018, 9:53 a.m. | #2
(swapping original email order around)
Thanks for the review!

> On 22 May 2018, at 15:04, Alan Modra <amodra@gmail.com> wrote:

> 

>> diff --git a/include/elf/internal.h b/include/elf/internal.h

>> index 05f9fab89c..5d93e0070e 100644

>> --- a/include/elf/internal.h

>> +++ b/include/elf/internal.h

>> @@ -347,9 +347,9 @@ struct elf_segment_map

>>        || ((sec_hdr)->sh_flags & SHF_ALLOC) == 0			\

>>        || ((sec_hdr)->sh_addr >= (segment)->p_vaddr			\

>> 	   && (!(strict)						\

>> -	       || ((sec_hdr)->sh_addr - (segment)->p_vaddr		\

>> +	       || ((sec_hdr)->sh_offset - (segment)->p_offset		\

>> 		   <= (segment)->p_memsz - 1))				\

>> -	   && (((sec_hdr)->sh_addr - (segment)->p_vaddr			\

>> +	   && (((sec_hdr)->sh_offset - (segment)->p_offset		\

>> 		+ ELF_SECTION_SIZE(sec_hdr, segment))			\

>> 	       <= (segment)->p_memsz)))					\

>>    /* No zero size sections at start or end of PT_DYNAMIC.  */		\

> 

> We test sh_offset already, just above the code you are patching.  So I

> think this VMA test should continue to test sh_addr.

> 

> Now there are reasons you might want to disable the VMA checks

> entirely, for example when overlays are present.  To do that, use

> ELF_SECTION_IN_SEGMENT_1 with check_vma false.  See the example in

> elf.c assign_files_positions_for_load_sections.

> 


Hmm, yes I somehow hadn’t spotted that.

My issue with ELF_SECTION_IN_SEGMENT was it’s use in
_bfd_elf_make_section_from_shdr(). If I remove my patch and replace it
with the following then everything “works” for me:

diff --git a/bfd/elf.c b/bfd/elf.c
index 6c66bbca02..1cf3131b2a 100644
--- a/bfd/elf.c
+++ b/bfd/elf.c
@@ -1140,7 +1140,7 @@ _bfd_elf_make_section_from_shdr (bfd *abfd,
          if (((phdr->p_type == PT_LOAD
                && (hdr->sh_flags & SHF_TLS) == 0)
               || phdr->p_type == PT_TLS)
-             && ELF_SECTION_IN_SEGMENT (hdr, phdr))
+             && ELF_SECTION_IN_SEGMENT_1 (hdr, phdr, 0, 0))
            {
              if ((flags & SEC_LOAD) == 0)
                newsect->lma = (phdr->p_paddr

Whilst this logically gives the same result as my patch, it feels
less like the correct solution.


> On Tue, May 22, 2018 at 11:52:15AM +0100, Alan Hayward wrote:

>> Use sh_offset and p_offset instead of sh_addr and p_vaddr when calculating if a section fits in a segment. Both methods are valid when using the GNU linker.

>> 

>> This change makes ELF_SECTION_IN_SEGMENT consistent with BFD _bfd_elf_make_section_from_shdr(), which has used offsets to calculate LMA since 2008.

> 

> LMA is not VMA.  The code you referenced doesn't really support the

> change you propose, nor did you say exactly why the change is needed.



I was hoping to not need to go into too much detail (my original version
of the patch email did have a description of the Armlinker, which I removed
due to it being a large off putting info dump).

The Armlinker in Arm Compiler uses a feature of scatter-loading, in the
sense that the linker has generated an image whose first task on startup is
to copy pieces of itself from their load addresses to their execution
addresses. Specifically, the entire image is packed into a single contiguous
blob running from addresses 0x0 – 0x49d4 (you imagine this being the target
embedded device's ROM / PROM / Flash / whatever), and the first thing that
happens is that the function __scatterload copies part of that range to
0x20000000 (the device's actual RAM, where the RW data will live during the
program's run time) and also zeroes out another range in the 0x20000000 area
(the bss / ZI data section).

(Other scatter loading features include data compression: in some situations
the version of a section's data packed into the initial ROM image will not even
match its runtime contents byte for byte, because instead it will be compressed,
and the scatter loading code will call a decompressor instead of a memcpy-type
function.)
 	
In images of this type, armlink's policy – since more or less forever – has
been to use section addresses (i.e. sh_addr in the section header table entries)
to indicate where a given section is expected to end up after the initial scatter
loading pass has run; and the segment addresses (p_vaddr and p_paddr – armlink
draws no distinction between the two, and I think in fact it always sets them
identically) indicate where and how the image expects to have been installed in
memory at the point where it starts running, i.e. before scatter loading. So in
this kind of case where the image starts off packed into a single chunk of ROM
and scatters itself out into RAM at the beginning of its run, the segment
addresses will describe the starting chunk of ROM, and the section addresses
show where everything ends up at the point where the interesting part of the
runtime begins.

The idea is that this gives a debugger everything it needs to know to both load
the image into memory in the first place (you use the segment addresses for that),
and to map memory addresses back to code / data / symbols during the main
runtime of the program (you use the section addresses, on the assumption that by
the time you're concerned with an address in any given section, that section has
already gone through scatter loading).
	 	
For example, here is the readelf output of a small binary I compiled. Note the
address for ER_DATA with PROGBITS. In the existing code this will use sh_addr,
incorrectly loading it to 0xead4. What we want instead is to use (p_vaddr +
sh_offset - p_offset), which is (0x8000 + 0x2b08 - 0x34), which is 0xaad4.


Section Headers:
  [Nr] Name              Type            Addr     Off    Size   ES Flg Lk Inf Al
  [ 0]                   NULL            00000000 000000 000000 00      0   0  0
  [ 1] ER_CODE           PROGBITS        00008000 000034 002ad4 00  AX  0   0  4
  [ 2] ER_DATA           PROGBITS        0000ead4 002b08 000020 00  WA  0   0  4
  [ 3] ER_DATA           NOBITS          0000eaf4 002b28 00015c 00  WA  0   0  4
  [ 4] ARM_LIB_STACKHEAP NOBITS          00040000 002b28 001000 00  WA  0   0  1
  [ 5] .debug_abbrev     PROGBITS        00000000 002b28 000065 00      0   0  1
  [ 6] .debug_frame      PROGBITS        00000000 002b8d 000d9c 00      0   0  1
  [ 7] .debug_info       PROGBITS        00000000 003929 00006e 00      0   0  1
  [ 8] .debug_line       PROGBITS        00000000 003997 00003c 00      0   0  1
  [ 9] .debug_pubnames   PROGBITS        00000000 0039d3 000029 00      0   0  1
  [10] .debug_pubtypes   PROGBITS        00000000 0039fc 000023 00      0   0  1
  [11] .debug_str        PROGBITS        00000000 003a1f 00008b 00      0   0  1
  [12] .symtab           SYMTAB          00000000 003aac 002d60 10     13 498  4
  [13] .strtab           STRTAB          00000000 00680c 001f54 00      0   0  1
  [14] .note             NOTE            00000000 008760 000020 00      0   0  4
  [15] .comment          PROGBITS        00000000 008780 0003e0 00      0   0  1
  [16] .shstrtab         STRTAB          00000000 008b60 0000ac 00      0   0  1

Program Headers:
  Type           Offset   VirtAddr   PhysAddr   FileSiz MemSiz  Flg Align
  LOAD           0x000034 0x00008000 0x00008000 0x02af4 0x03c50 RWE 0x4


All of this is still within the elf spec, but clearly different from the GNU
linker - the idea was to get a patch that allowed us the safely work for both
without any horrible “if gnu ... else if armlinker ...” code.


Alan.
Alan Modra May 23, 2018, 1:39 p.m. | #3
On Wed, May 23, 2018 at 09:53:06AM +0000, Alan Hayward wrote:
> (swapping original email order around)

> Thanks for the review!

> 

> > On 22 May 2018, at 15:04, Alan Modra <amodra@gmail.com> wrote:

> > 

> >> diff --git a/include/elf/internal.h b/include/elf/internal.h

> >> index 05f9fab89c..5d93e0070e 100644

> >> --- a/include/elf/internal.h

> >> +++ b/include/elf/internal.h

> >> @@ -347,9 +347,9 @@ struct elf_segment_map

> >>        || ((sec_hdr)->sh_flags & SHF_ALLOC) == 0			\

> >>        || ((sec_hdr)->sh_addr >= (segment)->p_vaddr			\

> >> 	   && (!(strict)						\

> >> -	       || ((sec_hdr)->sh_addr - (segment)->p_vaddr		\

> >> +	       || ((sec_hdr)->sh_offset - (segment)->p_offset		\

> >> 		   <= (segment)->p_memsz - 1))				\

> >> -	   && (((sec_hdr)->sh_addr - (segment)->p_vaddr			\

> >> +	   && (((sec_hdr)->sh_offset - (segment)->p_offset		\

> >> 		+ ELF_SECTION_SIZE(sec_hdr, segment))			\

> >> 	       <= (segment)->p_memsz)))					\

> >>    /* No zero size sections at start or end of PT_DYNAMIC.  */		\

> > 

> > We test sh_offset already, just above the code you are patching.  So I

> > think this VMA test should continue to test sh_addr.

> > 

> > Now there are reasons you might want to disable the VMA checks

> > entirely, for example when overlays are present.  To do that, use

> > ELF_SECTION_IN_SEGMENT_1 with check_vma false.  See the example in

> > elf.c assign_files_positions_for_load_sections.

> > 

> 

> Hmm, yes I somehow hadn’t spotted that.

> 

> My issue with ELF_SECTION_IN_SEGMENT was it’s use in

> _bfd_elf_make_section_from_shdr(). If I remove my patch and replace it

> with the following then everything “works” for me:

> 

> diff --git a/bfd/elf.c b/bfd/elf.c

> index 6c66bbca02..1cf3131b2a 100644

> --- a/bfd/elf.c

> +++ b/bfd/elf.c

> @@ -1140,7 +1140,7 @@ _bfd_elf_make_section_from_shdr (bfd *abfd,

>           if (((phdr->p_type == PT_LOAD

>                 && (hdr->sh_flags & SHF_TLS) == 0)

>                || phdr->p_type == PT_TLS)

> -             && ELF_SECTION_IN_SEGMENT (hdr, phdr))

> +             && ELF_SECTION_IN_SEGMENT_1 (hdr, phdr, 0, 0))

>             {

>               if ((flags & SEC_LOAD) == 0)

>                 newsect->lma = (phdr->p_paddr

> 

> Whilst this logically gives the same result as my patch, it feels

> less like the correct solution.

> 

> 

> > On Tue, May 22, 2018 at 11:52:15AM +0100, Alan Hayward wrote:

> >> Use sh_offset and p_offset instead of sh_addr and p_vaddr when calculating if a section fits in a segment. Both methods are valid when using the GNU linker.

> >> 

> >> This change makes ELF_SECTION_IN_SEGMENT consistent with BFD _bfd_elf_make_section_from_shdr(), which has used offsets to calculate LMA since 2008.

> > 

> > LMA is not VMA.  The code you referenced doesn't really support the

> > change you propose, nor did you say exactly why the change is needed.

> 

> 

> I was hoping to not need to go into too much detail (my original version

> of the patch email did have a description of the Armlinker, which I removed

> due to it being a large off putting info dump).


Thanks for the explanation.

> The Armlinker in Arm Compiler uses a feature of scatter-loading, in the

> sense that the linker has generated an image whose first task on startup is

> to copy pieces of itself from their load addresses to their execution

> addresses. Specifically, the entire image is packed into a single contiguous

> blob running from addresses 0x0 – 0x49d4 (you imagine this being the target

> embedded device's ROM / PROM / Flash / whatever), and the first thing that

> happens is that the function __scatterload copies part of that range to

> 0x20000000 (the device's actual RAM, where the RW data will live during the

> program's run time) and also zeroes out another range in the 0x20000000 area

> (the bss / ZI data section).

> 

> (Other scatter loading features include data compression: in some situations

> the version of a section's data packed into the initial ROM image will not even

> match its runtime contents byte for byte, because instead it will be compressed,

> and the scatter loading code will call a decompressor instead of a memcpy-type

> function.)

>  	

> In images of this type, armlink's policy – since more or less forever – has

> been to use section addresses (i.e. sh_addr in the section header table entries)

> to indicate where a given section is expected to end up after the initial scatter

> loading pass has run; and the segment addresses (p_vaddr and p_paddr – armlink

> draws no distinction between the two, and I think in fact it always sets them

> identically) indicate where and how the image expects to have been installed in

> memory at the point where it starts running, i.e. before scatter loading. So in

> this kind of case where the image starts off packed into a single chunk of ROM

> and scatters itself out into RAM at the beginning of its run, the segment

> addresses will describe the starting chunk of ROM, and the section addresses

> show where everything ends up at the point where the interesting part of the

> runtime begins.

> 

> The idea is that this gives a debugger everything it needs to know to both load

> the image into memory in the first place (you use the segment addresses for that),

> and to map memory addresses back to code / data / symbols during the main

> runtime of the program (you use the section addresses, on the assumption that by

> the time you're concerned with an address in any given section, that section has

> already gone through scatter loading).

> 	 	

> For example, here is the readelf output of a small binary I compiled. Note the

> address for ER_DATA with PROGBITS. In the existing code this will use sh_addr,

> incorrectly loading it to 0xead4. What we want instead is to use (p_vaddr +

> sh_offset - p_offset), which is (0x8000 + 0x2b08 - 0x34), which is 0xaad4.

> 

> 

> Section Headers:

>   [Nr] Name              Type            Addr     Off    Size   ES Flg Lk Inf Al

>   [ 0]                   NULL            00000000 000000 000000 00      0   0  0

>   [ 1] ER_CODE           PROGBITS        00008000 000034 002ad4 00  AX  0   0  4

>   [ 2] ER_DATA           PROGBITS        0000ead4 002b08 000020 00  WA  0   0  4

>   [ 3] ER_DATA           NOBITS          0000eaf4 002b28 00015c 00  WA  0   0  4

>   [ 4] ARM_LIB_STACKHEAP NOBITS          00040000 002b28 001000 00  WA  0   0  1

>   [ 5] .debug_abbrev     PROGBITS        00000000 002b28 000065 00      0   0  1

>   [ 6] .debug_frame      PROGBITS        00000000 002b8d 000d9c 00      0   0  1

>   [ 7] .debug_info       PROGBITS        00000000 003929 00006e 00      0   0  1

>   [ 8] .debug_line       PROGBITS        00000000 003997 00003c 00      0   0  1

>   [ 9] .debug_pubnames   PROGBITS        00000000 0039d3 000029 00      0   0  1

>   [10] .debug_pubtypes   PROGBITS        00000000 0039fc 000023 00      0   0  1

>   [11] .debug_str        PROGBITS        00000000 003a1f 00008b 00      0   0  1

>   [12] .symtab           SYMTAB          00000000 003aac 002d60 10     13 498  4

>   [13] .strtab           STRTAB          00000000 00680c 001f54 00      0   0  1

>   [14] .note             NOTE            00000000 008760 000020 00      0   0  4

>   [15] .comment          PROGBITS        00000000 008780 0003e0 00      0   0  1

>   [16] .shstrtab         STRTAB          00000000 008b60 0000ac 00      0   0  1

> 

> Program Headers:

>   Type           Offset   VirtAddr   PhysAddr   FileSiz MemSiz  Flg Align

>   LOAD           0x000034 0x00008000 0x00008000 0x02af4 0x03c50 RWE 0x4

> 

> 

> All of this is still within the elf spec, but clearly different from the GNU

> linker - the idea was to get a patch that allowed us the safely work for both

> without any horrible “if gnu ... else if armlinker ...” code.


I think you may need to detect these armlinker segments.  The problem
with disabling the sh_addr comparisons is that sh_offset for
SHT_NOBITS sections doesn't really have a physical meaning.  You don't
load SHT_NOBITS from file after all.  Your armlinker binaries might
have the convention that sh_offset is given values as if a segment's
bss part actually existed on disk, but not all ELF binaries will
follow that convention.  What's more, the convention breaks down in
the presence of multiple tightly packed PT_LOAD segments where any but
the last has p_memsz > p_filesz.  That situation would lead to
SHT_NOBITS sections having sh_offset values corresponding to the
PT_LOAD segment past the one to which they actually belong.

If the armlinker only ever creates one PT_LOAD segment, it might be
possible to accommodate it by disabling check_vma whenever there is
just one PT_LOAD segment.

-- 
Alan Modra
Australia Development Lab, IBM
Alan Hayward May 24, 2018, 1:18 p.m. | #4
> On 23 May 2018, at 14:39, Alan Modra <amodra@gmail.com> wrote:

> 


<snip>

>> 

>> For example, here is the readelf output of a small binary I compiled. Note the

>> address for ER_DATA with PROGBITS. In the existing code this will use sh_addr,

>> incorrectly loading it to 0xead4. What we want instead is to use (p_vaddr +

>> sh_offset - p_offset), which is (0x8000 + 0x2b08 - 0x34), which is 0xaad4.

>> 

>> 

>> Section Headers:

>>  [Nr] Name              Type            Addr     Off    Size   ES Flg Lk Inf Al

>>  [ 0]                   NULL            00000000 000000 000000 00      0   0  0

>>  [ 1] ER_CODE           PROGBITS        00008000 000034 002ad4 00  AX  0   0  4

>>  [ 2] ER_DATA           PROGBITS        0000ead4 002b08 000020 00  WA  0   0  4

>>  [ 3] ER_DATA           NOBITS          0000eaf4 002b28 00015c 00  WA  0   0  4

>>  [ 4] ARM_LIB_STACKHEAP NOBITS          00040000 002b28 001000 00  WA  0   0  1

>>  [ 5] .debug_abbrev     PROGBITS        00000000 002b28 000065 00      0   0  1

>>  [ 6] .debug_frame      PROGBITS        00000000 002b8d 000d9c 00      0   0  1

>>  [ 7] .debug_info       PROGBITS        00000000 003929 00006e 00      0   0  1

>>  [ 8] .debug_line       PROGBITS        00000000 003997 00003c 00      0   0  1

>>  [ 9] .debug_pubnames   PROGBITS        00000000 0039d3 000029 00      0   0  1

>>  [10] .debug_pubtypes   PROGBITS        00000000 0039fc 000023 00      0   0  1

>>  [11] .debug_str        PROGBITS        00000000 003a1f 00008b 00      0   0  1

>>  [12] .symtab           SYMTAB          00000000 003aac 002d60 10     13 498  4

>>  [13] .strtab           STRTAB          00000000 00680c 001f54 00      0   0  1

>>  [14] .note             NOTE            00000000 008760 000020 00      0   0  4

>>  [15] .comment          PROGBITS        00000000 008780 0003e0 00      0   0  1

>>  [16] .shstrtab         STRTAB          00000000 008b60 0000ac 00      0   0  1

>> 

>> Program Headers:

>>  Type           Offset   VirtAddr   PhysAddr   FileSiz MemSiz  Flg Align

>>  LOAD           0x000034 0x00008000 0x00008000 0x02af4 0x03c50 RWE 0x4

>> 

>> 

>> All of this is still within the elf spec, but clearly different from the GNU

>> linker - the idea was to get a patch that allowed us the safely work for both

>> without any horrible “if gnu ... else if armlinker ...” code.

> 

> I think you may need to detect these armlinker segments.  The problem

> with disabling the sh_addr comparisons is that sh_offset for

> SHT_NOBITS sections doesn't really have a physical meaning.  You don't

> load SHT_NOBITS from file after all.  Your armlinker binaries might

> have the convention that sh_offset is given values as if a segment's

> bss part actually existed on disk, but not all ELF binaries will

> follow that convention.  What's more, the convention breaks down in

> the presence of multiple tightly packed PT_LOAD segments where any but

> the last has p_memsz > p_filesz.  That situation would lead to

> SHT_NOBITS sections having sh_offset values corresponding to the

> PT_LOAD segment past the one to which they actually belong.

> 

> If the armlinker only ever creates one PT_LOAD segment, it might be

> possible to accommodate it by disabling check_vma whenever there is

> just one PT_LOAD segment.


Unfortunately, there is one PT_LOAD section per load region, and the
number of load regions is arbitrary and defined by the user.

I’ve been looking at ways to reliably detect if the armlinker was used.
Sadly, there is nothing in the elf file to reliably detect armlinker arm
binaries vs gnu linker arm binaries.

Am I right in thinking that both the zero offsets for SHT_NOBITS and the
tightly packed binaries are things that could appear in an Arm linux binary?
(I suspect you could write a linker script to do it). If so, then it wouldn’t
be safe to do something like "if (Arm and Linux) check_vma=false”.


Alan.
Alan Modra May 25, 2018, 3:17 a.m. | #5
On Thu, May 24, 2018 at 01:18:08PM +0000, Alan Hayward wrote:
> > On 23 May 2018, at 14:39, Alan Modra <amodra@gmail.com> wrote:

> > I think you may need to detect these armlinker segments.  The problem

> > with disabling the sh_addr comparisons is that sh_offset for

> > SHT_NOBITS sections doesn't really have a physical meaning.  You don't

> > load SHT_NOBITS from file after all.  Your armlinker binaries might

> > have the convention that sh_offset is given values as if a segment's

> > bss part actually existed on disk, but not all ELF binaries will

> > follow that convention.  What's more, the convention breaks down in

> > the presence of multiple tightly packed PT_LOAD segments where any but

> > the last has p_memsz > p_filesz.  That situation would lead to

> > SHT_NOBITS sections having sh_offset values corresponding to the

> > PT_LOAD segment past the one to which they actually belong.

> > 

> > If the armlinker only ever creates one PT_LOAD segment, it might be

> > possible to accommodate it by disabling check_vma whenever there is

> > just one PT_LOAD segment.

> 

> Unfortunately, there is one PT_LOAD section per load region, and the

> number of load regions is arbitrary and defined by the user.

> 

> I’ve been looking at ways to reliably detect if the armlinker was used.

> Sadly, there is nothing in the elf file to reliably detect armlinker arm

> binaries vs gnu linker arm binaries.

> 

> Am I right in thinking that both the zero offsets for SHT_NOBITS and the

> tightly packed binaries are things that could appear in an Arm linux binary?

> (I suspect you could write a linker script to do it). If so, then it wouldn’t

> be safe to do something like "if (Arm and Linux) check_vma=false”.


Current GNU ld follows the ELF gABI which says of sh_offset:

    This member's value gives the byte offset from the beginning of
    the file to the first byte in the section. One section type,
    SHT_NOBITS described below, occupies no space in the file, and its
    sh_offset member locates the conceptual placement in the file.

GNU ld interprets "conceptual placement" as if the PT_LOAD segment
existed in the file for the entire p_memsz not just its p_filesz.  I'm
farily certain we had times in the past where that wasn't the case,
and of course I can't say much about other linkers, which is why I
made the comment about "not all ELF binaries will follow that
convention".  (You could argue that GNU binutils shouldn't care about
binaries that don't follow this convention, and I think I mostly
agree..)

BTW, this interpretation of conceptual placement can lead to sh_offset
being past the end of file, and overlapping with sh_offset for
following sections if there are multiple PT_LOAD segments.  And yes,
you could generate such binaries for any ELF target using linker
scripts.

-- 
Alan Modra
Australia Development Lab, IBM
Alan Hayward June 18, 2018, 2:29 p.m. | #6
> On 25 May 2018, at 04:17, Alan Modra <amodra@gmail.com> wrote:

> 

> On Thu, May 24, 2018 at 01:18:08PM +0000, Alan Hayward wrote:

>>> On 23 May 2018, at 14:39, Alan Modra <amodra@gmail.com> wrote:

>>> I think you may need to detect these armlinker segments.  The problem

>>> with disabling the sh_addr comparisons is that sh_offset for

>>> SHT_NOBITS sections doesn't really have a physical meaning.  You don't

>>> load SHT_NOBITS from file after all.  Your armlinker binaries might

>>> have the convention that sh_offset is given values as if a segment's

>>> bss part actually existed on disk, but not all ELF binaries will

>>> follow that convention.  What's more, the convention breaks down in

>>> the presence of multiple tightly packed PT_LOAD segments where any but

>>> the last has p_memsz > p_filesz.  That situation would lead to

>>> SHT_NOBITS sections having sh_offset values corresponding to the

>>> PT_LOAD segment past the one to which they actually belong.

>>> 

>>> If the armlinker only ever creates one PT_LOAD segment, it might be

>>> possible to accommodate it by disabling check_vma whenever there is

>>> just one PT_LOAD segment.

>> 

>> Unfortunately, there is one PT_LOAD section per load region, and the

>> number of load regions is arbitrary and defined by the user.

>> 

>> I’ve been looking at ways to reliably detect if the armlinker was used.

>> Sadly, there is nothing in the elf file to reliably detect armlinker arm

>> binaries vs gnu linker arm binaries.

>> 

>> Am I right in thinking that both the zero offsets for SHT_NOBITS and the

>> tightly packed binaries are things that could appear in an Arm linux binary?

>> (I suspect you could write a linker script to do it). If so, then it wouldn’t

>> be safe to do something like "if (Arm and Linux) check_vma=false”.

> 

> Current GNU ld follows the ELF gABI which says of sh_offset:

> 

>    This member's value gives the byte offset from the beginning of

>    the file to the first byte in the section. One section type,

>    SHT_NOBITS described below, occupies no space in the file, and its

>    sh_offset member locates the conceptual placement in the file.

> 

> GNU ld interprets "conceptual placement" as if the PT_LOAD segment

> existed in the file for the entire p_memsz not just its p_filesz.  I'm

> farily certain we had times in the past where that wasn't the case,

> and of course I can't say much about other linkers, which is why I

> made the comment about "not all ELF binaries will follow that

> convention".  (You could argue that GNU binutils shouldn't care about

> binaries that don't follow this convention, and I think I mostly

> agree..)

> 

> BTW, this interpretation of conceptual placement can lead to sh_offset

> being past the end of file, and overlapping with sh_offset for

> following sections if there are multiple PT_LOAD segments.  And yes,

> you could generate such binaries for any ELF target using linker

> scripts.

> 


(looking at this issue again with fresh eyes)

As I read it above, the problem comes down to changing the way SHT_NOBITS
works

However, working through it, the existing code is completely fine for
the SHT_NOBITS with armlinker.

Firstly, on SHT_NOBITS sections, lma and vma always get set to sh_addr.
Which is fine.
(
This is regardless of the result of ELF_SECTION_IN_SEGMENT
lma and vma get set to sh_addr early in _bfd_elf_make_section_from_shdr.
Then either
ELF_SECTION_IN_SEGMENT will find a segment (potentially an incorrect one)
and set lma to phdr->p_paddr + hdr->sh_addr - phdr->p_vaddr. But because
armlink produces segments only with p_paddr = p_vaddr, it ends up with
sh_addr - the same as it was before.
Or
ELF_SECTION_IN_SEGMENT doesn’t find a segment, and lma remains unchanged.
)

Secondly SHT_NOBITS never get written to target memory by gdb ( because
_bfd_elf_make_section_from_shdr() doesn’t set SEC_LOAD for SHT_NOBITS 
sections ). This matches what images produced by armlink expect - those
sections are initialized at startup by the C library code.


That just leaves us with non-SHT_NOBITS sections.

In the existing code all non-SHT_NOBITS sections are set to SEC_LOAD:
      if (hdr->sh_type != SHT_NOBITS)
	flags |= SEC_LOAD;

Then after finding a section with ELF_SECTION_IN_SEGMENT, all SEC_LOAD
sections are set using offsets in the code quoted in the original email:

	      if ((flags & SEC_LOAD) == 0)
		newsect->lma = (phdr->p_paddr
				+ hdr->sh_addr - phdr->p_vaddr);
	      else
		/* We used to use the same adjustment for SEC_LOAD
		   sections, but that doesn't work if the segment
		   is packed with code from multiple VMAs.
		   Instead we calculate the section LMA based on
		   the segment LMA.  It is assumed that the
		   segment will contain sections with contiguous
		   LMAs, even if the VMAs are not.  */
		newsect->lma = (phdr->p_paddr
				+ hdr->sh_offset - phdr->p_offset); 

Therefore if lma for non-SHT_NOBITS is set based on offsets then surely
it is safe to only use offsets for non-SHT_NOBITS sections in 
ELF_SECTION_IN_SEGMENT ?

That would then change the patch to the one below. (tested using make
check x86 on a binutils+gdb build. It has also been manually tested by
using gdb to debug a Arm Mbed board.)

Are you ok with this version?

Thanks,
Alan.


2018-06-15  Alan Hayward  <alan.hayward@arm.com>

	* include/elf/internal.h (ELF_SECTION_IN_SEGMENT): Don’t check
	addresses for non SHT_NOBITS.


diff --git a/include/elf/internal.h b/include/elf/internal.h
index 05f9fab89cbe2aee94006d18a689cb01e101776f..b012820f6cf9c7e6b5879748b0b05685594987bb 100644
--- a/include/elf/internal.h
+++ b/include/elf/internal.h
@@ -342,8 +342,10 @@ struct elf_segment_map
 	   && (((sec_hdr)->sh_offset - (segment)->p_offset		\
 		+ ELF_SECTION_SIZE(sec_hdr, segment))			\
 	       <= (segment)->p_filesz)))				\
-   /* SHF_ALLOC sections must have VMAs within the segment.  */		\
+   /* SHT_NOBITS sections with SHF_ALLOC must have VMAs within the	\
+      segment.  */							\
    && (!(check_vma)							\
+       || (sec_hdr)->sh_type != SHT_NOBITS				\
        || ((sec_hdr)->sh_flags & SHF_ALLOC) == 0			\
        || ((sec_hdr)->sh_addr >= (segment)->p_vaddr			\
 	   && (!(strict)						\
Alan Modra June 20, 2018, 10:59 p.m. | #7
On Mon, Jun 18, 2018 at 02:29:05PM +0000, Alan Hayward wrote:
> 	* include/elf/internal.h (ELF_SECTION_IN_SEGMENT): Don’t check

> 	addresses for non SHT_NOBITS.

> 

> 

> diff --git a/include/elf/internal.h b/include/elf/internal.h

> index 05f9fab89cbe2aee94006d18a689cb01e101776f..b012820f6cf9c7e6b5879748b0b05685594987bb 100644

> --- a/include/elf/internal.h

> +++ b/include/elf/internal.h

> @@ -342,8 +342,10 @@ struct elf_segment_map

>  	   && (((sec_hdr)->sh_offset - (segment)->p_offset		\

>  		+ ELF_SECTION_SIZE(sec_hdr, segment))			\

>  	       <= (segment)->p_filesz)))				\

> -   /* SHF_ALLOC sections must have VMAs within the segment.  */		\

> +   /* SHT_NOBITS sections with SHF_ALLOC must have VMAs within the	\

> +      segment.  */							\

>     && (!(check_vma)							\

> +       || (sec_hdr)->sh_type != SHT_NOBITS				\

>         || ((sec_hdr)->sh_flags & SHF_ALLOC) == 0			\

>         || ((sec_hdr)->sh_addr >= (segment)->p_vaddr			\

>  	   && (!(strict)						\


This looks reasonable to me.  OK to apply.

-- 
Alan Modra
Australia Development Lab, IBM

Patch

diff --git a/include/elf/internal.h b/include/elf/internal.h
index 05f9fab89c..5d93e0070e 100644
--- a/include/elf/internal.h
+++ b/include/elf/internal.h
@@ -347,9 +347,9 @@  struct elf_segment_map
        || ((sec_hdr)->sh_flags & SHF_ALLOC) == 0			\
        || ((sec_hdr)->sh_addr >= (segment)->p_vaddr			\
 	   && (!(strict)						\
-	       || ((sec_hdr)->sh_addr - (segment)->p_vaddr		\
+	       || ((sec_hdr)->sh_offset - (segment)->p_offset		\
 		   <= (segment)->p_memsz - 1))				\
-	   && (((sec_hdr)->sh_addr - (segment)->p_vaddr			\
+	   && (((sec_hdr)->sh_offset - (segment)->p_offset		\
 		+ ELF_SECTION_SIZE(sec_hdr, segment))			\
 	       <= (segment)->p_memsz)))					\
    /* No zero size sections at start or end of PT_DYNAMIC.  */		\