Page MenuHomePhabricator

[objcopy][ELF] Handle sections not contained in PT_LOAD segments
AbandonedPublic

Authored by LemonBoy on Apr 28 2020, 1:50 PM.

Details

Summary

In D71035 the LMA calculation was changed to more closely match the behaviour of GNU
tools. The code assumed that every parent section would be a PT_LOAD one, but it's possible
to create legal ELF binaries where this rule doesn't hold anymore.

Handle this case by using the same equation used by GNU objdump, the fix allows objdump
to create correctly-sized raw binaries now.

Diff Detail

Event Timeline

LemonBoy created this revision.Apr 28 2020, 1:50 PM
Herald added a reviewer: alexshap. · View Herald Transcript
Herald added a project: Restricted Project. · View Herald Transcript

I don't understand the issue here personally. Can you elaborate? The formula also doesn't make sense to me since Sec.Addr isn't really well defined at that point in the code. e.g. this *is* the layout algorithm so you shouldn't be using Sec.Addr here IMO

MaskRay added a comment.EditedApr 28 2020, 4:07 PM

The code assumed that every parent section would be a PT_LOAD one, but it's possible to create legal ELF binaries where this rule doesn't hold anymore.

It is possible, but I hope we can find legitimate and reasonable use cases. p_memsz(PT_GNU_RELRO) makes sense (-z stack-size=), so this seems to suggest that we should patch template <class ELFT> void ELFBuilder<ELFT>::setParentSegment(Segment &Child) to take into account this case (don't set ParentSegment(PT_LOAD) to PT_GNU_STACK)

I don't understand the issue here personally

As explained by @MaskRay you can create a segment with zero starting address and arbitrarily large memsize by using the -z stack-size= switch, the libc interprets that value as the minimum size for new threads. The segment acts as a catch-all parent segment for many sections, such as the .bss in the test case, and causes problems in the LMA calculation code.

Sec.Addr isn't really well defined at that point in the code

There's an unfortunate typo in the patch, the equation is meant to simply rebase the sh_addr by subtracting the phdr vaddr and adding its paddr.

this seems to suggest that we should patch template <class ELFT> void ELFBuilder<ELFT>::setParentSegment(Segment &Child) to take into account this case

I decided against that because I couldn't find any definition of what makes a segment eligible to be a parent. If we use the same logic that readelf uses then the PT_STACK_SIZE is indeed a parent for .bss

Section to Segment mapping:
 Segment Sections...
  00     .text 
  01     .bss
LemonBoy updated this revision to Diff 261005.Apr 29 2020, 1:16 PM

Fix a typo in the address calculation formula and fix the hexdump in the test case.

Please fix the review title and description to say "llvm-objcopy" not "llvm-objdump".

It seems confusing that there should be different logic between sections within PT_LOAD and those within other segment types. What is actually the difference between them here?

I decided against that because I couldn't find any definition of what makes a segment eligible to be a parent.

The definition is internal to llvm-objcopy, and isn't really well written down in comments anywhere, IIRC, just with the code. There are no rules on "eligibility" for a segment to be a parent, i.e. it doesn't have to be a specific type. Simply all that happens is that if a segment at least partially overlaps another segment, the "left-most" one is picked. If one starts at the same offset, the larger is considered the parent, with the first in the program header the final tie-breaker. Finally, there is a "most parental" segment, i.e. there are no "grandparents" in the code - if segment A has a parent, segment B, and would be the parent of segment C, segment C's parent will be segment B instead.

llvm/test/tools/llvm-objcopy/ELF/binary-paddr.test
133–134

Could you outline in a comment what the "before" behaviour of this test case was. It will help with reviewing.

138

Why --ignore-case?

llvm/tools/llvm-objcopy/ELF/Object.cpp
2274

Sec.Addr here is just the initial address contained in the section header, right? That deserves a comment at least, because it is definitely not obvious that that is the case in this context, where we appear to be setting the value.

By the way, I don't have any particular insight into how the behaviour should work here, as I don't use binary output myself.

What is actually the difference between them here?

The difference doesn't really matter here as the segment PAddr and VAddr are zero, the test case passes by simply avoiding the Sec.Addr assigment for sections with non-PT_LOAD segments, that's why it's been suggested to patch setParentSegment and keep the old logic.
The added logic takes into account one more edge case, as explained by this comment in BFD:

/* 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.  */

If you prefer a less-invasive patch I can simply go for the setParentSegment route.

MaskRay added a comment.EditedApr 30 2020, 9:43 AM

What is actually the difference between them here?

The difference doesn't really matter here as the segment PAddr and VAddr are zero, the test case passes by simply avoiding the Sec.Addr assigment for sections with non-PT_LOAD segments, that's why it's been suggested to patch setParentSegment and keep the old logic.
The added logic takes into account one more edge case, as explained by this comment in BFD:

/* 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.  */

If you prefer a less-invasive patch I can simply go for the setParentSegment route.

I did some shallow analysis of bfd/elf.c's rules related to bfd_section::lma/file offset/etc. They are overly complex and I am pretty sure we can do better than them. Their distinction of PT_LOAD ver non-PT_LOAD is artificial and can really be simplified if they don't do that. I second to James that we should not make PT_LOAD different.

Unfortunately, the PT_GNU_RELRO use case (p_vaddr=p_paddr=0, large p_memsz) means we still have to specialize something. It seems that we can simply treat PT_GNU_RELRO as an (ugly) exception.

LemonBoy updated this revision to Diff 261311.Apr 30 2020, 11:59 AM

Simplify the patch by making sure PT_GNU_STACK segments cannot be considered parents of any section.

New code looks good to me.

llvm/test/tools/llvm-objcopy/ELF/binary-paddr.test
138
149

Reduce indentation so that there is exactly one space after the longest key (Machine:)

174

The comment is stale.

LemonBoy updated this revision to Diff 261363.Apr 30 2020, 2:51 PM

Minor touchups for the test case.

jhenderson accepted this revision.May 1 2020, 2:17 AM

Latest version LGTM, with one comment nit.

llvm/test/tools/llvm-objcopy/ELF/binary-paddr.test
138

Thanks. Evidently I forgot that.

Bizarre that this tool somehow is so inconsistent in behaviour compared to other simialr tools (see also problems I've had before with argument ordering).

llvm/tools/llvm-objcopy/ELF/Object.cpp
1068

"segments, they" -> either "segments. They" or "segments: they"

This revision is now accepted and ready to land.May 1 2020, 2:17 AM

with one comment nit

It's probably better if you (or somebody else) with commit access amends the comment just before committing so that the PR doesn't have to be approved again.

LemonBoy retitled this revision from [objdump][ELF] Handle sections not contained in PT_LOAD segments to [objcopy][ELF] Handle sections not contained in PT_LOAD segments.May 1 2020, 11:34 AM
MaskRay accepted this revision.May 1 2020, 2:29 PM

I will address the comments and push.

MaskRay requested changes to this revision.May 1 2020, 3:15 PM

The test does not make sense: .bss is not covered by a PT_LOAD.

Section Headers:
  [Nr] Name              Type            Address          Off    Size   ES Flg Lk Inf Al
  [ 0]                   NULL            0000000000000000 000000 000000 00      0   0  0
  [ 1] .text             PROGBITS        0000000000008000 0000b0 000008 00  AX  0   0  8
  [ 2] .bss              NOBITS          0000000000008008 0000b8 000004 00  WA  0   0  1

Program Headers:
  Type           Offset   VirtAddr           PhysAddr           FileSiz  MemSiz   Flg Align
  LOAD           0x0000b0 0x0000000000008000 0x0000000000008000 0x000008 0x000008 R E 0x8
  GNU_STACK      0x000000 0x0000000000000000 0x0000000000000000 0x000000 0x1000000 RW  0

If we extend the existing PT_LOAD to cover .bss, then we can apply D74755 instead. We don't need the special case for PT_GNU_STACK.

The description is also wrong now. I may say something like:

A PT_GNU_STACK segment can have very large p_memsz (specified by ld -z stack-size=)
 and zero p_vaddr/p_paddr.
 
 `sectionWithinSegment` prefers assigning the parent segment of a section
 to the PT_GNU_STACK over a PT_LOAD. In the test, this causes .bss to have a large LMA and make the output much larger.
This revision now requires changes to proceed.May 1 2020, 3:15 PM
MaskRay added inline comments.May 1 2020, 3:17 PM
llvm/test/tools/llvm-objcopy/ELF/binary-paddr.test
138

I would use only 0-9 and omit --ignore-case

# RUN: yaml2obj --docnum=6 %s -o %t6
# RUN: llvm-objcopy -O binary %t6 %t6.out
# RUN: od -A x -t x1 %t6.out | FileCheck %s --check-prefix=GNUSTACK

# GNUSTACK:      000000 90 90
# GNUSTACK-NEXT: 000002

test does not make sense: .bss is not covered by a PT_LOAD.

That's the whole point of this patch, the binary that ptomptrd me to investigate why the binary image was so big has a single PT_LOAD segment covering only .text.

The description is also wrong now. I may say something like:

Sure thing, I actually forgot to update it.

test does not make sense: .bss is not covered by a PT_LOAD.

That's the whole point of this patch, the binary that ptomptrd me to investigate why the binary image was so big has a single PT_LOAD segment covering only .text.

The description is also wrong now. I may say something like:

Sure thing, I actually forgot to update it.

Why does your executable have a .bss not covered by a PT_LOAD? I think it is invalid but I am open to justifications.

An alternative is to apply D74755. Last time I checked D74755 would fix your problem as well.

test does not make sense: .bss is not covered by a PT_LOAD.

That's the whole point of this patch, the binary that ptomptrd me to investigate why the binary image was so big has a single PT_LOAD segment covering only .text.

The description is also wrong now. I may say something like:

Sure thing, I actually forgot to update it.

Why does your executable have a .bss not covered by a PT_LOAD? I think it is invalid but I am open to justifications.

An alternative is to apply D74755. Last time I checked D74755 would fix your problem as well.

There's a bit more context here, the ELF is indeed wrong but GNU objcopy manages to extract the correct raw binary. Feel free to apply whatever patch fixes the problem and/or mark this use case as invalid, I don't have much time for this stuff right now.

LemonBoy abandoned this revision.May 31 2020, 12:54 PM