[ELF] - Restrict section offsets that exceeds file size.

[grimar]

Summary

This is part of PR36515.

With some linkerscripts it is possible to get file offset overlaps
and overflows. Currently LLD checks overlaps in checkNoOverlappingSections().
And also we allow broken output with --no-inhibit-exec.
Problem is that sometimes final offset of sections is completely broken
and we calculate output file size wrong and might crash.

After investigation, I think best what we can do is to error out
when section offsets exceeding file size. That still allows moving location counter backwards
and parameters overlaping tested by checkNoOverlappingSections(),
but at the same time ensures that linker will never crash because of that.

[grimar]

Will update with one more check soon.

[grimar]

• Rebased, improved check.

[jhenderson]

ELF/Writer.cpp

15–16

"with linker script" -> "with linker scripts"
"want to fully restrict" -> "want to prevent"
"would crash linker" -> "would crash the linker"

18

I don't have a strong feeling here either way, but I wonder if we should worry about empty sections at all? Maybe they shouldn't cause the file to be resized? Also, why do we care whether the start of a NOBITS section is in the file?

Also, assuming the intended behaviour is that the section can start at the end of the file, if it is empty or NOBITS, I think the Overflow check is slightly wrong. If a NOBITS or empty section starts at the very end of the file (i.e. Sec->Offset == FileSize), it will be treated as overflowing as things stand.

24

", check" -> ". Check"

test/ELF/linkerscript/sections-va-overflow.s

6 ↗

(On Diff #136074)

Could you split this line into two, please, one per section.

11 ↗

(On Diff #136074)

result of broken -> result of a broken
overlow -> overflow

12 ↗

(On Diff #136074)

Seems -> It seems
we report error -> we report an error

[grimar]

ELF/Writer.cpp

18

Your right. I think we should just ignore all SHT_NOBITS sections in this loop as we newer write them and they can
not be a reason of crash.

[grimar]

• Addressed review comments.

ELF/Writer.cpp

24

I do not think we use dots + uppercases in error messages. I used ; instead,
it seems a bit hard to find few long error for reference, but at least it is consistent with
"can't create dynamic relocation R_386_GOT32X against symbol: foo in readonly segment; recompile object files with -fPIC"

[jhenderson]

Could you extend the test to show that we don't emit an error for NOBITS sections in this situation, please. Otherwise, looks reasonable.

ELF/Writer.cpp

24

Okay, that's far enough.

[grimar]

• Check in thestcase that nobits section does not trigger an error.

[jhenderson]

test/ELF/linkerscript/sections-va-overflow.s

7 ↗

(On Diff #136276)

I think you should explicitly place .bss somewhere that would cause a problem, similar to .text, as otherwise the test is going to rely on the implicit nature of orphan section handling.

[grimar]

• Addressed review comment.
• Rewrote test (in according to the recent LLD changes).

[jhenderson]

Also, assuming the intended behaviour is that the section can start at the end of the file, if it is empty or NOBITS, I think the Overflow check is slightly wrong. If a NOBITS or empty section starts at the very end of the file (i.e. Sec->Offset == FileSize), it will be treated as overflowing as things stand.

You've only addressed this comment for NOBITS sections and not empty sections. As things stand, an empty section at the very end of the file will cause an error. Similarly, any regular section that has a size such that it terminates at exactly the end of the file will have problems.

test/ELF/linkerscript/sections-va-overflow.test

3

Nit: Don't use .so filename without -shared, or somebody is likely to get misled!

[grimar]

In D43819#1023718, @jhenderson wrote:

Also, assuming the intended behaviour is that the section can start at the end of the file, if it is empty or NOBITS, I think the Overflow check is slightly wrong. If a NOBITS or empty section starts at the very end of the file (i.e. Sec->Offset == FileSize), it will be treated as overflowing as things stand.

You've only addressed this comment for NOBITS sections and not empty sections. As things stand, an empty section at the very end of the file will cause an error. Similarly, any regular section that has a size such that it terminates at exactly the end of the file will have problems.

I am not sure I understand the use case, sorry. We normally can not have an empty section at the end of the file, because we place section header table there.
If somehow section offset is damaged during calculation because of overflows, then ((Sec->Offset >= FileSize) || (Sec->Offset + Sec->Size >= FileSize) check will catch it and report an error to prevent
mapped buffer overflow and crash during writing. That is what this code supposed to do. What am I missing?

[grimar]

As things stand, an empty section at the very end of the file will cause an error.

I think I was able to imagine that part. So if we have an empty section and overflow of its offset, so that it is placed at the very end of file. That would not cause crash
during writing because it is empty. It is too specific to support, I do not think it worth to add code here, we will report an error saying that script has overflows and that will be true anyways.

[jhenderson]

In D43819#1025064, @grimar wrote:

I am not sure I understand the use case, sorry. We normally can not have an empty section at the end of the file, because we place section header table there.
If somehow section offset is damaged during calculation because of overflows, then ((Sec->Offset >= FileSize) || (Sec->Offset + Sec->Size >= FileSize) check will catch it and report an error to prevent
mapped buffer overflow and crash during writing. That is what this code supposed to do. What am I missing?

It's a correctness thing again. Ultimately, you are correct, we currently can't generate sections at the end of the file, because the section header table is written there. However, this may not always be the case, and then we'd have a bug. Two potential future use cases might be a) writing the section header table earlier in the file (although admittedly I don't know why we would, but it is valid), or b) not writing the section header table at all (since linked ELFs don't require section headers in order to be run).
Sec->Offset == FileSize (or Sec->Offset + Sec->Size == FileSize - note that this latter case actually applies to non-empty sections too).

We don't need to add any additional code to get this. Simply change the '>=' to '>' in the comparisons.

[grimar]

In D43819#1025104, @jhenderson wrote:

In D43819#1025064, @grimar wrote:

I am not sure I understand the use case, sorry. We normally can not have an empty section at the end of the file, because we place section header table there.
If somehow section offset is damaged during calculation because of overflows, then ((Sec->Offset >= FileSize) || (Sec->Offset + Sec->Size >= FileSize) check will catch it and report an error to prevent
mapped buffer overflow and crash during writing. That is what this code supposed to do. What am I missing?

It's a correctness thing again. Ultimately, you are correct, we currently can't generate sections at the end of the file, because the section header table is written there. However, this may not always be the case, and then we'd have a bug. Two potential future use cases might be a) writing the section header table earlier in the file (although admittedly I don't know why we would, but it is valid), or b) not writing the section header table at all (since linked ELFs don't require section headers in order to be run).
Sec->Offset == FileSize (or Sec->Offset + Sec->Size == FileSize - note that this latter case actually applies to non-empty sections too).

We don't need to add any additional code to get this. Simply change the '>=' to '>' in the comparisons.

I see now. Thank you for explanation. I'll change the comparsions as suggested, but I think
probably will be unable to add test for that. Can not imagine reasonable test since we have section headers currently.

[grimar]

• Addressed review comments.

[jhenderson]

LGTM.

[grimar]

Thanks, James!

Rui, Rafael, what do you think?

[ruiu]

ELF/Writer.cpp

14–16

But why does this happen? If we detects all backward movements of '.', it shouldn't happen, no?

16

[start -> end] (e.g. [0xFFFFFFFF20000000 -> 0xFFFFFFFE40000000]) is a weird notion to represent a range. I think [start:end] is more common.

test/ELF/linkerscript/sections-va-overflow.test

8

This file contains trailing white spaces.

10

Please insert a blank line between PHDRS and SECTIONS because that's what you would usually do to normal linker scripts.

[jhenderson]

ELF/Writer.cpp

16

FWIW - this function is just a move of an existing function.

In places like llvm-dwarfdump dumping of debug ranges etc, the common practice is "[start, end)". Maybe that's what we should follow as well.

[grimar]

ELF/Writer.cpp

16

Yes. Since it is a move, I would not change the format of the output in this patch.

[grimar]

*Addressed comments about test case.

ELF/Writer.cpp

14–16

No, we don't. We detect the backward movements only inside the section declarations.
See: https://github.com/llvm-mirror/lld/blob/master/ELF/LinkerScript.cpp#L125
If we would detect all movements, we would be unable to link some software,
for example - Linux kernel (see D34977).

[grimar]

ELF/Writer.cpp

16

LLD currently use [X, Y] for ranges in the messages:
relocation R_386_16 out of range: 65536 is not in [0, 65535]

Should we be consistent here?

[jhenderson]

ELF/Writer.cpp

16

Being consistent with what LLD does works for me.

[ruiu]

ELF/Writer.cpp

14–16

Please explain that. At least I couldn't get that from the current comment.

[• rafael]

LGTM

It feels like we should be able to error earlier, but it is better to
fix the crash first and investigate alternatives second.

Cheers,
Rafael

George Rimar via Phabricator via llvm-commits
<[e-mail]> writes:

grimar updated this revision to Diff 136725.
grimar added a comment.

• Addressed review comments.

https://reviews.llvm.org/D43819

Files:

ELF/Writer.cpp
test/ELF/linkerscript/Inputs/sections-va-overflow.s
test/ELF/linkerscript/sections-va-overflow.test

Index: test/ELF/linkerscript/sections-va-overflow.test

• test/ELF/linkerscript/sections-va-overflow.test

+++ test/ELF/linkerscript/sections-va-overflow.test
@@ -0,0 +1,21 @@
+# REQUIRES: x86
+# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %p/Inputs/sections-va-overflow.s -o %t.o
+# RUN: not ld.lld -o /dev/null --script %s %t.o 2>&1 | FileCheck %s -check-prefix=ERR
+
+PHDRS {
+ ph_headers PT_PHDR PHDRS;
+ ph_text PT_LOAD FILEHDR PHDRS FLAGS (0x1 | 0x4);
+}
+SECTIONS {
+ . = 0xffffffff20000000;
+ .text : { *(.text*) } : ph_text
+ .test 0x1000 : { BYTE(0) }
+ .bss : { *(.bss*) }
+}
+
+ Section .test has VA 0x1000 and placed in the same segment as section .text + with VA 0xffffffff20000000. That might be technically correct, but most probably
+ is a result of a broken script file and causes file offset calculation overflow. + It seems we do not have to support it, so we don't and we report an error in this case.
+# ERR: error: unable to place section .text at file offset [0xFFFFFFFF20000000 -> 0xFFFFFFFE40000000]; check your linker script for overflows
+# ERR-NOT: unable to place section .bss

Index: test/ELF/linkerscript/Inputs/sections-va-overflow.s

• test/ELF/linkerscript/Inputs/sections-va-overflow.s

+++ test/ELF/linkerscript/Inputs/sections-va-overflow.s
@@ -0,0 +1,6 @@
+.global _start
+_start:
+ retq
+
+.bss
+.space 0x2000

Index: ELF/Writer.cpp

• ELF/Writer.cpp

+++ ELF/Writer.cpp
@@ -1826,6 +1826,12 @@

}

}

+static std::string rangeToString(uint64_t Addr, uint64_t Len) {
+ if (Len == 0)
+ return "<empty range at 0x" + utohexstr(Addr) + ">";
+ return "[0x" + utohexstr(Addr) + " -> 0x" + utohexstr(Addr + Len - 1) + "]";
+}
+
Adjusts the file alignment for a given output section and returns
its new file offset. The file offset must be the same with its
// virtual address (modulo the page size) so that the loader can load
@@ -1896,6 +1902,18 @@

SectionHeaderOff = alignTo(Off, Config->Wordsize);
FileSize = SectionHeaderOff + (OutputSections.size() + 1) * sizeof(Elf_Shdr);

+
+ It is possible to get file offset overlaps or overflows with linker
+ scripts. We perform checks required in checkNoOverlappingSections() and
+ // want to prevent file size overflows here because it would crash the linker.
+ for (OutputSection *Sec : OutputSections) {
+ if (Sec->Type == SHT_NOBITS)
+ continue;
+ if ((Sec->Offset > FileSize) || (Sec->Offset + Sec->Size > FileSize))
+ error("unable to place section " + Sec->Name + " at file offset " +
+ rangeToString(Sec->Offset, Sec->Offset + Sec->Size) +
+ "; check your linker script for overflows");
+ }
}

// Finalize the program headers. We call this function after we assign
@@ -1934,12 +1952,6 @@

}

}

-static std::string rangeToString(uint64_t Addr, uint64_t Len) {

• if (Len == 0)
• return "<empty range at 0x" + utohexstr(Addr) + ">";
• return "[0x" + utohexstr(Addr) + " -> 0x" + utohexstr(Addr + Len - 1) + "]";

-}

• Check whether sections overlap for a specific address range (file offsets, load and virtual adresses). //

Index: test/ELF/linkerscript/sections-va-overflow.test

• test/ELF/linkerscript/sections-va-overflow.test

+++ test/ELF/linkerscript/sections-va-overflow.test
@@ -0,0 +1,21 @@
+# REQUIRES: x86
+# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %p/Inputs/sections-va-overflow.s -o %t.o
+# RUN: not ld.lld -o /dev/null --script %s %t.o 2>&1 | FileCheck %s -check-prefix=ERR
+
+PHDRS {
+ ph_headers PT_PHDR PHDRS;
+ ph_text PT_LOAD FILEHDR PHDRS FLAGS (0x1 | 0x4);
+}
+SECTIONS {
+ . = 0xffffffff20000000;
+ .text : { *(.text*) } : ph_text
+ .test 0x1000 : { BYTE(0) }
+ .bss : { *(.bss*) }
+}
+
+ Section .test has VA 0x1000 and placed in the same segment as section .text + with VA 0xffffffff20000000. That might be technically correct, but most probably
+ is a result of a broken script file and causes file offset calculation overflow. + It seems we do not have to support it, so we don't and we report an error in this case.
+# ERR: error: unable to place section .text at file offset [0xFFFFFFFF20000000 -> 0xFFFFFFFE40000000]; check your linker script for overflows
+# ERR-NOT: unable to place section .bss

Index: test/ELF/linkerscript/Inputs/sections-va-overflow.s

• test/ELF/linkerscript/Inputs/sections-va-overflow.s

+++ test/ELF/linkerscript/Inputs/sections-va-overflow.s
@@ -0,0 +1,6 @@
+.global _start
+_start:
+ retq
+
+.bss
+.space 0x2000

Index: ELF/Writer.cpp

• ELF/Writer.cpp

+++ ELF/Writer.cpp
@@ -1826,6 +1826,12 @@

}

}

+static std::string rangeToString(uint64_t Addr, uint64_t Len) {
+ if (Len == 0)
+ return "<empty range at 0x" + utohexstr(Addr) + ">";
+ return "[0x" + utohexstr(Addr) + " -> 0x" + utohexstr(Addr + Len - 1) + "]";
+}
+
Adjusts the file alignment for a given output section and returns
its new file offset. The file offset must be the same with its
// virtual address (modulo the page size) so that the loader can load
@@ -1896,6 +1902,18 @@

SectionHeaderOff = alignTo(Off, Config->Wordsize);
FileSize = SectionHeaderOff + (OutputSections.size() + 1) * sizeof(Elf_Shdr);

+
+ It is possible to get file offset overlaps or overflows with linker
+ scripts. We perform checks required in checkNoOverlappingSections() and
+ // want to prevent file size overflows here because it would crash the linker.
+ for (OutputSection *Sec : OutputSections) {
+ if (Sec->Type == SHT_NOBITS)
+ continue;
+ if ((Sec->Offset > FileSize) || (Sec->Offset + Sec->Size > FileSize))
+ error("unable to place section " + Sec->Name + " at file offset " +
+ rangeToString(Sec->Offset, Sec->Offset + Sec->Size) +
+ "; check your linker script for overflows");
+ }
}

// Finalize the program headers. We call this function after we assign
@@ -1934,12 +1952,6 @@

}

}

-static std::string rangeToString(uint64_t Addr, uint64_t Len) {

• if (Len == 0)
• return "<empty range at 0x" + utohexstr(Addr) + ">";
• return "[0x" + utohexstr(Addr) + " -> 0x" + utohexstr(Addr + Len - 1) + "]";

-}

• Check whether sections overlap for a specific address range (file offsets, load and virtual adresses). //

---

llvm-commits mailing list
[e-mail]
http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits

[grimar]

• Rebased, rewrote comment as requested.

[grimar]

• Adjusted the comment.

[jhenderson]

ELF/Writer.cpp

15

use of linker script -> use of linker scripts

17

move location counter -> move the location counter

18–19

Linux kernel which controls segments -> Linux kernel, which control segment
moves counter backwards so -> moves the counter backwards, so

[grimar]

• Applied grammar fixes. Thanks, James!

[jhenderson]

ELF/Writer.cpp

18–19

My bad - missed one more!

moves the counter -> move the counter

[grimar]

• Addressed review comment.

[jhenderson]

Okay, LGTM again, although @ruiu should probably check the comment clarifies the situation satisfactorily from his point of view.

[• rafael]

LGTM.

I think the comment can probably be simplified, but that can be a
followup.

Cheers,
Rafael

George Rimar via Phabricator <[e-mail]> writes:

grimar updated this revision to Diff 137976.
grimar added a comment.

• Addressed review comment.

https://reviews.llvm.org/D43819

Files:

ELF/Writer.cpp
test/ELF/linkerscript/Inputs/sections-va-overflow.s
test/ELF/linkerscript/sections-va-overflow.test

Index: test/ELF/linkerscript/sections-va-overflow.test

• test/ELF/linkerscript/sections-va-overflow.test

+++ test/ELF/linkerscript/sections-va-overflow.test
@@ -0,0 +1,22 @@
+# REQUIRES: x86
+# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %p/Inputs/sections-va-overflow.s -o %t.o
+# RUN: not ld.lld -o /dev/null --script %s %t.o 2>&1 | FileCheck %s -check-prefix=ERR
+
+PHDRS {
+ ph_headers PT_PHDR PHDRS;
+ ph_text PT_LOAD FILEHDR PHDRS FLAGS (0x1 | 0x4);
+}
+
+SECTIONS {
+ . = 0xffffffff20000000;
+ .text : { *(.text*) } : ph_text
+ .test 0x1000 : { BYTE(0) }
+ .bss : { *(.bss*) }
+}
+
+ Section .test has VA 0x1000 and placed in the same segment as section .text + with VA 0xffffffff20000000. That might be technically correct, but most probably
+ is a result of a broken script file and causes file offset calculation overflow. + It seems we do not have to support it, so we don't and we report an error in this case.
+# ERR: error: unable to place section .text at file offset [0xFFFFFFFF20000000, 0xFFFFFFFE40000000]; check your linker script for overflows
+# ERR-NOT: unable to place section .bss

Index: test/ELF/linkerscript/Inputs/sections-va-overflow.s

• test/ELF/linkerscript/Inputs/sections-va-overflow.s

+++ test/ELF/linkerscript/Inputs/sections-va-overflow.s
@@ -0,0 +1,6 @@
+.global _start
+_start:
+ retq
+
+.bss
+.space 0x2000

Index: ELF/Writer.cpp

• ELF/Writer.cpp

+++ ELF/Writer.cpp
@@ -1873,6 +1873,12 @@

FileSize = alignTo(Off, Config->Wordsize);

}

+static std::string rangeToString(uint64_t Addr, uint64_t Len) {
+ if (Len == 0)
+ return "<empty range at 0x" + utohexstr(Addr) + ">";
+ return "[0x" + utohexstr(Addr) + ", 0x" + utohexstr(Addr + Len - 1) + "]";
+}
+
// Assign file offsets to output sections.
template <class ELFT> void Writer<ELFT>::assignFileOffsets() {

uint64_t Off = 0;

@@ -1897,6 +1903,25 @@

SectionHeaderOff = alignTo(Off, Config->Wordsize);
FileSize = SectionHeaderOff + (OutputSections.size() + 1) * sizeof(Elf_Shdr);

+
+ Our logic assumes that sections have rising VA within the same segment.
+ With use of linker scripts it is possible to violate this rule and get file
+ offset overlaps or overflows. That should never happen with a valid script
+ which does not move the location counter backwards and usually scripts do
+ not do that. Unfortunately, there are apps in the wild, for example, Linux
+ kernel, which control segment distribution explicitly and move the counter
+ backwards, so we have to allow doing that to support linking them. We
+ perform non-critical checks for overlaps in checkNoOverlappingSections(),
+ but here we want to prevent file size overflows because it would crash the
+ linker.
+ for (OutputSection *Sec : OutputSections) {
+ if (Sec->Type == SHT_NOBITS)
+ continue;
+ if ((Sec->Offset > FileSize) || (Sec->Offset + Sec->Size > FileSize))
+ error("unable to place section " + Sec->Name + " at file offset " +
+ rangeToString(Sec->Offset, Sec->Offset + Sec->Size) +
+ "; check your linker script for overflows");
+ }
}

// Finalize the program headers. We call this function after we assign
@@ -1935,12 +1960,6 @@

}

}

-static std::string rangeToString(uint64_t Addr, uint64_t Len) {

• if (Len == 0)
• return "<empty range at 0x" + utohexstr(Addr) + ">";
• return "[0x" + utohexstr(Addr) + ", 0x" + utohexstr(Addr + Len - 1) + "]";

-}

• Check whether sections overlap for a specific address range (file offsets, load and virtual adresses). //

Index: test/ELF/linkerscript/sections-va-overflow.test

• test/ELF/linkerscript/sections-va-overflow.test

+++ test/ELF/linkerscript/sections-va-overflow.test
@@ -0,0 +1,22 @@
+# REQUIRES: x86
+# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %p/Inputs/sections-va-overflow.s -o %t.o
+# RUN: not ld.lld -o /dev/null --script %s %t.o 2>&1 | FileCheck %s -check-prefix=ERR
+
+PHDRS {
+ ph_headers PT_PHDR PHDRS;
+ ph_text PT_LOAD FILEHDR PHDRS FLAGS (0x1 | 0x4);
+}
+
+SECTIONS {
+ . = 0xffffffff20000000;
+ .text : { *(.text*) } : ph_text
+ .test 0x1000 : { BYTE(0) }
+ .bss : { *(.bss*) }
+}
+
+ Section .test has VA 0x1000 and placed in the same segment as section .text + with VA 0xffffffff20000000. That might be technically correct, but most probably
+ is a result of a broken script file and causes file offset calculation overflow. + It seems we do not have to support it, so we don't and we report an error in this case.
+# ERR: error: unable to place section .text at file offset [0xFFFFFFFF20000000, 0xFFFFFFFE40000000]; check your linker script for overflows
+# ERR-NOT: unable to place section .bss

Index: test/ELF/linkerscript/Inputs/sections-va-overflow.s

• test/ELF/linkerscript/Inputs/sections-va-overflow.s

+++ test/ELF/linkerscript/Inputs/sections-va-overflow.s
@@ -0,0 +1,6 @@
+.global _start
+_start:
+ retq
+
+.bss
+.space 0x2000

Index: ELF/Writer.cpp

• ELF/Writer.cpp

+++ ELF/Writer.cpp
@@ -1873,6 +1873,12 @@

FileSize = alignTo(Off, Config->Wordsize);

}

+static std::string rangeToString(uint64_t Addr, uint64_t Len) {
+ if (Len == 0)
+ return "<empty range at 0x" + utohexstr(Addr) + ">";
+ return "[0x" + utohexstr(Addr) + ", 0x" + utohexstr(Addr + Len - 1) + "]";
+}
+
// Assign file offsets to output sections.
template <class ELFT> void Writer<ELFT>::assignFileOffsets() {

uint64_t Off = 0;

@@ -1897,6 +1903,25 @@

SectionHeaderOff = alignTo(Off, Config->Wordsize);
FileSize = SectionHeaderOff + (OutputSections.size() + 1) * sizeof(Elf_Shdr);

+
+ Our logic assumes that sections have rising VA within the same segment.
+ With use of linker scripts it is possible to violate this rule and get file
+ offset overlaps or overflows. That should never happen with a valid script
+ which does not move the location counter backwards and usually scripts do
+ not do that. Unfortunately, there are apps in the wild, for example, Linux
+ kernel, which control segment distribution explicitly and move the counter
+ backwards, so we have to allow doing that to support linking them. We
+ perform non-critical checks for overlaps in checkNoOverlappingSections(),
+ but here we want to prevent file size overflows because it would crash the
+ linker.
+ for (OutputSection *Sec : OutputSections) {
+ if (Sec->Type == SHT_NOBITS)
+ continue;
+ if ((Sec->Offset > FileSize) || (Sec->Offset + Sec->Size > FileSize))
+ error("unable to place section " + Sec->Name + " at file offset " +
+ rangeToString(Sec->Offset, Sec->Offset + Sec->Size) +
+ "; check your linker script for overflows");
+ }
}

// Finalize the program headers. We call this function after we assign
@@ -1935,12 +1960,6 @@

}

}

-static std::string rangeToString(uint64_t Addr, uint64_t Len) {

• if (Len == 0)
• return "<empty range at 0x" + utohexstr(Addr) + ">";
• return "[0x" + utohexstr(Addr) + ", 0x" + utohexstr(Addr + Len - 1) + "]";

-}

• Check whether sections overlap for a specific address range (file offsets, load and virtual adresses). //

[ruiu]

LGTM

ELF/Writer.cpp

 //===- Writer.cpp ---------------------------------------------------------===//
 //
 //                             The LLVM Linker
 //
 // This file is distributed under the University of Illinois Open Source
 // License. See LICENSE.TXT for details.
 //
 //===----------------------------------------------------------------------===//
 
 #include "Writer.h"
 #include "AArch64ErrataFix.h"
 #include "Config.h"
 #include "Filesystem.h"
 #include "LinkerScript.h"
 #include "MapFile.h"
 #include "OutputSections.h"
 #include "Relocations.h"
 #include "SymbolTable.h"
 #include "Symbols.h"
 #include "SyntheticSections.h"
 #include "Target.h"
 #include "lld/Common/Memory.h"
 #include "lld/Common/Strings.h"
 #include "lld/Common/Threads.h"
 #include "llvm/ADT/StringMap.h"
 #include "llvm/ADT/StringSwitch.h"
 #include <climits>
 
 using namespace llvm;
 using namespace llvm::ELF;
 using namespace llvm::object;
 using namespace llvm::support;
for (const PhdrEntry *P : Phdrs) {
     if (I == End || (I + 1) == End)
       continue;
     OutputSection *Cmd = (*(I + 1));
     if (needsPtLoad(Cmd))
       PageAlign(Cmd);
   }
 }
 
+static std::string rangeToString(uint64_t Addr, uint64_t Len) {
+  if (Len == 0)
+    return "<empty range at 0x" + utohexstr(Addr) + ">";
+  return "[0x" + utohexstr(Addr) + " -> 0x" + utohexstr(Addr + Len - 1) + "]";
+}
+
 // Adjusts the file alignment for a given output section and returns
 // its new file offset. The file offset must be the same with its
 // virtual address (modulo the page size) so that the loader can load
 // executables without any address adjustment.
 static uint64_t getFileAlignment(uint64_t Off, OutputSection *Cmd) {
   OutputSection *First = Cmd->PtLoad ? Cmd->PtLoad->FirstSec : nullptr;
   // The first section in a PT_LOAD has to have congruent offset and address
   // module the page size.
for (OutputSection *Sec : OutputSections) {
     // segment is the last loadable segment, align the offset of the
     // following section to avoid loading non-segments parts of the file.
     if (LastRX && LastRX->LastSec == Sec)
       Off = alignTo(Off, Target->PageSize);
   }
 
   SectionHeaderOff = alignTo(Off, Config->Wordsize);
   FileSize = SectionHeaderOff + (OutputSections.size() + 1) * sizeof(Elf_Shdr);
+
+  // It is possible to get file offset overlaps or overflows with linker
+  // scripts. We perform checks required in checkNoOverlappingSections() and
+  // want to prevent file size overflows here because it would crash the linker.
+  for (OutputSection *Sec : OutputSections) {
+    if (Sec->Type == SHT_NOBITS)
+      continue;
+    if ((Sec->Offset >= FileSize) || (Sec->Offset + Sec->Size >= FileSize))
+      error("unable to place section " + Sec->Name + " at file offset " +
+            rangeToString(Sec->Offset, Sec->Offset + Sec->Size) +
+            "; check your linker script for overflows");
+  }
 }
 
 // Finalize the program headers. We call this function after we assign
 // file offsets and VAs to all sections.
 template <class ELFT> void Writer<ELFT>::setPhdrs() {
   for (PhdrEntry *P : Phdrs) {
     OutputSection *First = P->FirstSec;
     OutputSection *Last = P->LastSec;
for (PhdrEntry *P : Phdrs) {
     if (P->p_type == PT_TLS) {
       Out::TlsPhdr = P;
       if (P->p_memsz)
         P->p_memsz = alignTo(P->p_memsz, P->p_align);
     }
   }
 }
 
-static std::string rangeToString(uint64_t Addr, uint64_t Len) {
-  if (Len == 0)
-    return "<empty range at 0x" + utohexstr(Addr) + ">";
-  return "[0x" + utohexstr(Addr) + " -> 0x" + utohexstr(Addr + Len - 1) + "]";
-}
-
 // Check whether sections overlap for a specific address range (file offsets,
 // load and virtual adresses).
 //
 // This is a helper function called by Writer::checkNoOverlappingSections().
 template <typename Getter, typename Predicate>
 static void checkForSectionOverlap(ArrayRef<OutputSection *> AllSections,
                                    StringRef Kind, Getter GetStart,
                                    Predicate ShouldSkip) {

test/ELF/linkerscript/Inputs/sections-va-overflow.s

+.global _start
+_start:
+  retq
+
+.bss
+.space 0x2000

test/ELF/linkerscript/sections-va-overflow.test

+# REQUIRES: x86
+# RUN: llvm-mc -filetype=obj -triple=x86_64-unknown-linux %p/Inputs/sections-va-overflow.s -o %t.o
+# RUN: not ld.lld -o %t.so --script %s %t.o 2>&1 | FileCheck %s -check-prefix=ERR
+
+PHDRS {
+  ph_headers  PT_PHDR PHDRS;
+  ph_text PT_LOAD FILEHDR PHDRS  FLAGS (0x1 | 0x4); 
+}
+SECTIONS { 
+ . = 0xffffffff20000000;
+ .text : { *(.text*) } : ph_text
+ .test 0x1000 : { BYTE(0) }
+ .bss : { *(.bss*) }
+}
+
+## Section .test has VA 0x1000 and placed in the same segment as section .text
+## with VA 0xffffffff20000000. That might be technically correct, but most probably
+## is a result of a broken script file and causes file offset calculation overflow.
+## It seems we do not have to support it, so we don't and we report an error in this case.
+# ERR: error: unable to place section .text at file offset [0xFFFFFFFF20000000 -> 0xFFFFFFFE40000000]; check your linker script for overflows
+# ERR-NOT: unable to place section .bss