Re: [PATCH] Use offsets instead of addresses in ELF_SECTION_IN_SEGMENT

[Alan Hayward <Alan dot Hayward at arm dot com>]

> 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)						\