Reject invalid absolute relocations in PIE/shared links#923
Reject invalid absolute relocations in PIE/shared links#923quic-areg wants to merge 1 commit intoqualcomm:mainfrom
Conversation
quic-areg
force-pushed
the
dynrelocs
branch
3 times, most recently
from
64c5622 to
164156c
Compare
quic-seaswara
left a comment
quic-seaswara
left a comment
There was a problem hiding this comment.
Please add tests for -z text with -shared links.
It might be helpful to add tests that will fail with -z text and not with -z notext
| // non-preemptible, or we are linking statically. | ||
| if (pSym.isAbsolute() && | ||
| (pSym.binding() == ResolveInfo::Local || config().isCodeStatic())) | ||
| (pSym.binding() == ResolveInfo::Local || !isSymbolPreemptible(pSym) || |
There was a problem hiding this comment.
This fix needs to be part of a different PR.
Please raise a issue and fix accordingly.
There was a problem hiding this comment.
I think it should be part of this PR since it is part of making PIC relocation legality correct. Without this change, the existing defsym case (which isn't actually checked by the following readelf line) would incorrectly fail in ABS64RelocationPIE.test with recompile with -fPIC.
There was a problem hiding this comment.
I agree it is an issue. We may need to cherry pick the change to another release branch if needed.
For this reason, this change needs to be in a separate PR.
There was a problem hiding this comment.
I can push it to the release branch when it's needed
There was a problem hiding this comment.
Bug fixes / features need to be separated. Please push that as a separate PR.
There was a problem hiding this comment.
Also I am not sure if absolute symbols ever need a dynamic relocation, should it return false for all cases ?
There was a problem hiding this comment.
This is all the same bug fix in my view
| // Truncated absolute relocations cannot be safely represented as dynamic | ||
| // relocations for 64-bit outputs. | ||
| if (isAbs && config().targets().is64Bits() && getSize(reloc.type()) < 64) | ||
| return reportNonPICRelocation(reloc); |
There was a problem hiding this comment.
I could not follow this. This is not a dynamic relocation if the size of the reloc < 64.
Are we creating a dynamic relocation for this ?
There was a problem hiding this comment.
The point is that we cannot emit truncated absolute relocations for pie/shared links for 64-bit outputs. We currently do, and that's what this code aims to address. See pie-abs32-64.s for a test case: lld and bfd will reject the truncated relocation while today's eld will accept it.
| return reportNonPICRelocation(reloc); | ||
| } | ||
|
|
||
| bool Relocator::checkDynamicRelocAllowed(const Relocation &reloc, |
There was a problem hiding this comment.
Instead of introducing a new function, can we change the function helper_DynRel_init to error out if we are ending up creating a dynamic relocation from read only sections ?
There was a problem hiding this comment.
I think it is easier and cleaner to keep legality decisions in a separate function.
lib/Target/Relocator.cpp
Outdated
| const bool isReadOnly = !(section.getFlags() & llvm::ELF::SHF_WRITE); | ||
|
|
||
| // -z text (default) rejects dynamic relocations in read-only sections. | ||
| if (section.isAlloc() && isReadOnly && |
There was a problem hiding this comment.
This should probably be checking for output section instead of input section.
cat > 1.s << 'S'
.globl foo
.section .foo, "a", @progbits
foo:
.word bar
ret
.data
.hidden bar
bar:
.word 0
S
cat > s.t << \!
SECTIONS {
.t : {
*(.foo*)
*(.data*)
}
}
!
clang -c 1.s -target riscv32 -fPIC
$ riscv-ld -shared 1.o -o lib1.so -z text -T s.t && llvm-readelf -r lib1.so
Relocation section '.rela.dyn' at offset 0x1050 contains 1 entries:
Offset Info Type Sym. Value Symbol's Name + Addend
0000005c 00000003 R_RISCV_RELATIVE 62
the bfd linker does not error here.
There was a problem hiding this comment.
Thanks for catching this. Updated.
ELD currently permits some absolute relocations that should be rejected when producing PIE/shared objects. ELD would also allow truncated absolute relocations in 64-bit outputs. Now it correctly reports an error. Symbols fully resolved at link time (eg defsym) should not require a dynamic relocation. With -z text, reject absolute relocations in PIE/shared links. With -z notext, split behavior by target bitness: - 32-bit targets may emit TEXTREL. - 64-bit targets error on truncated absolute dynamic relocations. Closes qualcomm#832. Signed-off-by: quic-areg <aregmi@qti.qualcomm.com>
|
3 participants
Reject invalid absolute relocations in PIE/shared links
ELD currently permits some absolute relocations that should be rejected when producing PIE/shared objects.
ELD would also allow truncated absolute relocations in 64-bit outputs. Now it correctly reports an error.
Symbols fully resolved at link time (eg defsym) should not require a dynamic relocation.
With -z text, reject absolute relocations in PIE/shared links.
With -z notext, split behavior by target bitness:
Closes #832.