Skip to content

Reject invalid absolute relocations in PIE/shared links #923

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
quic-areg wants to merge 1 commit into
base: main
Choose a base branch
from
Open

Reject invalid absolute relocations in PIE/shared links #923

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions include/eld/Target/GNULDBackend.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -380,6 +380,8 @@ class GNULDBackend {
/// Target can override this function if needed.
virtual uint64_t maxBranchOffset() { return (uint64_t)-1; }

bool isTargetReadOnly(const ELFSection &input) const;

/// checkAndSetHasTextRel - check pSection flag to set HasTextRel
void checkAndSetHasTextRel(const ELFSection &pSection);

Expand Down
15 changes: 15 additions & 0 deletions include/eld/Target/Relocator.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,6 +133,11 @@ class Relocator {

uint32_t getRelocType(std::string Name);

bool checkPICRelocSupported(const Relocation &reloc);

bool checkDynamicRelocAllowed(const Relocation &reloc,
const ELFSection &section, bool isAbs) const;

/// Get Symbol Name
std::string getSymbolName(const ResolveInfo *R) const;

Expand All @@ -159,6 +164,16 @@ class Relocator {
llvm::DenseMap<uint64_t, bool> UndefHits;

protected:
bool reportNonPICRelocation(const Relocation &reloc) const;

virtual bool isPICRelocTypeSupported(const Relocation &reloc) const {
return true;
}

virtual bool isDynamicRelocSupported(const Relocation &reloc) const {
return true;
}

Module &m_Module;
std::mutex m_RelocMutex;
std::unordered_map<std::string, uint32_t> RelocNameMap;
Expand Down
11 changes: 5 additions & 6 deletions lib/Object/ObjectLinker.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -718,6 +718,10 @@ void ObjectLinker::assignOutputSections(std::vector<eld::InputFile *> &Inputs) {
ObjectBuilder Builder(ThisConfig, *ThisModule);
auto Start = std::chrono::steady_clock::now();
Builder.assignOutputSections(Inputs, MPostLtoPhase);
// Refresh matched inputs for each rule before relocation scan. We need this
// to infer output properties from script merged inputs before layout is
// finalized.
RuleContainer::updateMatchedSections(*ThisModule);
// FIXME: Perhaps transfer entry section ownership to GarbageCollection as
// Entry sections are only relevant with garbage collection.
// Currently, entry section are computed even if garbage-collection is not
Expand All @@ -726,13 +730,8 @@ void ObjectLinker::assignOutputSections(std::vector<eld::InputFile *> &Inputs) {
LayoutInfo *LayoutInfo = ThisModule->getLayoutInfo();
if (LayoutInfo && LayoutInfo->LayoutInfo::showInitialLayout()) {
TextLayoutPrinter *TextMapPrinter = ThisModule->getTextMapPrinter();
if (TextMapPrinter) {
// FIXME: ideally, we should not need 'updateMatchedSections' call here.
// However, we need it because currently we do not maintain the list of
// matched input sections for rule containers consistently.
RuleContainer::updateMatchedSections(*ThisModule);
if (TextMapPrinter)
TextMapPrinter->printLayout(*ThisModule);
}
}
// FIXME: SectionMatcher plugins should not consume time under
// 'LinkerScriptRuleMatch' timing category!
Expand Down
30 changes: 13 additions & 17 deletions lib/Target/AArch64/AArch64Relocator.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -160,20 +160,18 @@ bool AArch64Relocator::isRelocSupported(Relocation &pReloc) const {
return true;
}

bool AArch64Relocator::isInvalidReloc(Relocation &pReloc) const {
if (!config().isCodeIndep())
return false;
switch (pReloc.type()) {
bool AArch64Relocator::isPICRelocTypeSupported(const Relocation &reloc) const {
switch (reloc.type()) {
case llvm::ELF::R_AARCH64_ABS16:
return true;
return false;
case llvm::ELF::R_AARCH64_TLSLE_ADD_TPREL_LO12_NC:
case llvm::ELF::R_AARCH64_TLSLE_ADD_TPREL_LO12:
case llvm::ELF::R_AARCH64_TLSLE_ADD_TPREL_HI12:
return !config().options().isPIE();
return config().options().isPIE();
default:
break;
}
return false;
return true;
}

bool AArch64Relocator::relocNeedsDynRel(Relocation &pReloc) const {
Expand All @@ -187,11 +185,9 @@ bool AArch64Relocator::relocNeedsDynRel(Relocation &pReloc) const {
return false;
return true;
}
// Only ABS64 relocations can be dynamic in AArch64
bool isAbsReloc = pReloc.type() == llvm::ELF::R_AARCH64_ABS64;
return getTarget().symbolNeedsDynRel(
*rsym, (rsym->reserved() & ReservePLT),
isAbsReloc);
true);
}

Relocator::Result AArch64Relocator::applyRelocation(Relocation &pRelocation) {
Expand Down Expand Up @@ -245,6 +241,8 @@ void AArch64Relocator::scanLocalReloc(InputFile &pInput, Relocation &pReloc,
// Reserve an entry in .rel.dyn
if (config().isCodeIndep() || isAuthAbs) {
std::lock_guard<std::mutex> relocGuard(m_RelocMutex);
if (!checkDynamicRelocAllowed(pReloc, pSection, true))
return;
// set Rel bit
rsym->setReserved(rsym->reserved() | ReserveRel);
getTarget().checkAndSetHasTextRel(pSection);
Expand All @@ -266,6 +264,8 @@ void AArch64Relocator::scanLocalReloc(InputFile &pInput, Relocation &pReloc,
// Reserve an entry in .rel.dyn
if (config().isCodeIndep()) {
std::lock_guard<std::mutex> relocGuard(m_RelocMutex);
if (!checkDynamicRelocAllowed(pReloc, pSection, true))
return;
// set up the dyn rel directly
helper_DynRel_init(Obj, &pReloc, rsym, pReloc.targetRef()->frag(),
pReloc.targetRef()->offset(), pReloc.type(), m_Target);
Expand Down Expand Up @@ -391,6 +391,8 @@ void AArch64Relocator::scanGlobalReloc(InputFile &pInput, Relocation &pReloc,
}
CopyRelocs.insert(rsym);
} else {
if (!checkDynamicRelocAllowed(pReloc, pSection, true))
return;
// set Rel bit
rsym->setReserved(rsym->reserved() | ReserveRel);
getTarget().checkAndSetHasTextRel(pSection);
Expand Down Expand Up @@ -608,14 +610,8 @@ void AArch64Relocator::scanRelocation(Relocation &pReloc,
return;
}

if (isInvalidReloc(pReloc)) {
std::lock_guard<std::mutex> relocGuard(m_RelocMutex);
config().raise(Diag::non_pic_relocation)
<< getName(pReloc.type()) << pReloc.symInfo()->name()
<< pReloc.getSourcePath(config().options());
m_Target.getModule().setFailure(true);
if (!checkPICRelocSupported(pReloc))
return;
}

// rsym - The relocation target symbol
ResolveInfo *rsym = pReloc.symInfo();
Expand Down
2 changes: 1 addition & 1 deletion lib/Target/AArch64/AArch64Relocator.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -76,7 +76,7 @@ class AArch64Relocator : public Relocator {
const ELFSection &pSection) override;

private:
bool isInvalidReloc(Relocation &pReloc) const;
bool isPICRelocTypeSupported(const Relocation &reloc) const override;
bool isRelocSupported(Relocation &pReloc) const;
bool relocNeedsDynRel(Relocation &pReloc) const;

Expand Down
52 changes: 13 additions & 39 deletions lib/Target/ARM/ARMRelocator.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -254,10 +254,8 @@ ARMRelocator::ARMRelocator(ARMGNULDBackend &pParent, LinkerConfig &pConfig,

ARMRelocator::~ARMRelocator() {}

bool ARMRelocator::isInvalidReloc(Relocation &pReloc) const {
if (!config().isCodeIndep())
return false;
switch (pReloc.type()) {
bool ARMRelocator::isPICRelocTypeSupported(const Relocation &reloc) const {
switch (reloc.type()) {
case llvm::ELF::R_ARM_ABS16:
case llvm::ELF::R_ARM_ABS12:
case llvm::ELF::R_ARM_THM_ABS5:
Expand All @@ -269,13 +267,12 @@ bool ARMRelocator::isInvalidReloc(Relocation &pReloc) const {
case llvm::ELF::R_ARM_THM_MOVT_ABS:
case llvm::ELF::R_ARM_TLS_LE12:
case llvm::ELF::R_ARM_TLS_IE12GP:
return true;
return false;
case llvm::ELF::R_ARM_TLS_LE32:
return !config().options().isPIE();
return config().options().isPIE();
default:
break;
return true;
}
return false;
}

Relocator::Result ARMRelocator::applyRelocation(Relocation &pRelocation) {
Expand Down Expand Up @@ -312,14 +309,8 @@ Relocator::Size ARMRelocator::getSize(Relocation::Type pType) const {
return 32;
}

/// checkValidReloc - When we attempt to generate a dynamic relocation for
/// output file, check if the relocation is supported by dynamic linker.
bool ARMRelocator::checkValidReloc(Relocation &pReloc) const {
// If not PIC object, no relocation type is invalid
if (!config().isCodeIndep())
return true;

switch (pReloc.type()) {
bool ARMRelocator::isDynamicRelocSupported(const Relocation &reloc) const {
switch (reloc.type()) {
case llvm::ELF::R_ARM_RELATIVE:
case llvm::ELF::R_ARM_COPY:
case llvm::ELF::R_ARM_GLOB_DAT:
Expand All @@ -331,12 +322,9 @@ bool ARMRelocator::checkValidReloc(Relocation &pReloc) const {
case llvm::ELF::R_ARM_TLS_DTPOFF32:
case llvm::ELF::R_ARM_TLS_TPOFF32:
return true;
break;

default:
return false;
}
return false;
}

void ARMRelocator::scanLocalReloc(InputFile &pInput, Relocation::Type Type,
Expand All @@ -362,6 +350,8 @@ void ARMRelocator::scanLocalReloc(InputFile &pInput, Relocation::Type Type,
// Reserve an entry in .rel.dyn
if (config().isCodeIndep()) {
std::lock_guard<std::mutex> relocGuard(m_RelocMutex);
if (!checkDynamicRelocAllowed(pReloc, pSection, true))
return;
helper_DynRel_init(Obj, &pReloc, rsym, pReloc.targetRef()->frag(),
pReloc.targetRef()->offset(),
llvm::ELF::R_ARM_RELATIVE, m_Target);
Expand Down Expand Up @@ -554,13 +544,8 @@ void ARMRelocator::scanGlobalReloc(InputFile &pInput, Relocation::Type Type,
}
CopyRelocs.insert(rsym);
} else {
if (!checkValidReloc(pReloc)) {
config().raise(Diag::non_pic_relocation)
<< (int)pReloc.type() << pReloc.symInfo()->name()
<< pReloc.getSourcePath(config().options());
m_Target.getModule().setFailure(true);
if (!checkDynamicRelocAllowed(pReloc, pSection, true))
return;
}
helper_DynRel_init(Obj, &pReloc, rsym, pReloc.targetRef()->frag(),
pReloc.targetRef()->offset(),
isSymbolPreemptible ? llvm::ELF::R_ARM_ABS32
Expand Down Expand Up @@ -642,13 +627,8 @@ void ARMRelocator::scanGlobalReloc(InputFile &pInput, Relocation::Type Type,
if (getTarget().symbolNeedsCopyReloc(pReloc, *rsym)) {
CopyRelocs.insert(rsym);
} else {
if (!checkValidReloc(pReloc)) {
config().raise(Diag::non_pic_relocation)
<< (int)pReloc.type() << pReloc.symInfo()->name()
<< pReloc.getSourcePath(config().options());
m_Target.getModule().setFailure(true);
return;
}
if(!checkDynamicRelocAllowed(pReloc, pSection, false))
return ;
rsym->setReserved(rsym->reserved() | ReserveRel);
getTarget().checkAndSetHasTextRel(pSection);
}
Expand Down Expand Up @@ -807,14 +787,8 @@ void ARMRelocator::scanRelocation(Relocation &pReloc, eld::IRBuilder &pBuilder,
if (LinkerConfig::Object == config().codeGenType())
return;

if (isInvalidReloc(pReloc)) {
std::lock_guard<std::mutex> relocGuard(m_RelocMutex);
config().raise(Diag::non_pic_relocation)
<< getName(pReloc.type()) << pReloc.symInfo()->name()
<< pReloc.getSourcePath(config().options());
m_Target.getModule().setFailure(true);
if (!checkPICRelocSupported(pReloc))
return;
}

// rsym - The relocation target symbol
ResolveInfo *rsym = pReloc.symInfo();
Expand Down
4 changes: 2 additions & 2 deletions lib/Target/ARM/ARMRelocator.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -73,15 +73,15 @@ class ARMRelocator : public Relocator {
}

private:
bool isInvalidReloc(Relocation &pType) const;
bool isPICRelocTypeSupported(const Relocation &reloc) const override;
void scanLocalReloc(InputFile &pInput, Relocation::Type, Relocation &pReloc,
const ELFSection &pSection);

void scanGlobalReloc(InputFile &pInput, Relocation::Type, Relocation &pReloc,
eld::IRBuilder &pBuilder, ELFSection &pSection,
CopyRelocs &);

bool checkValidReloc(Relocation &pReloc) const;
bool isDynamicRelocSupported(const Relocation &reloc) const override;

uint32_t relocType() const override { return llvm::ELF::SHT_REL; }

Expand Down
31 changes: 22 additions & 9 deletions lib/Target/GNULDBackend.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -3916,11 +3916,11 @@ bool GNULDBackend::symbolNeedsDynRel(const ResolveInfo &pSym, bool pSymHasPLT,
LinkerConfig::Binary == config().codeGenType()))
return false;

// An absolute symbol can be resolved directly if it is either local
// or we are linking statically. Otherwise it can still be overridden
// at runtime.
// An absolute symbol can be resolved directly if it is either local,
// non-preemptible, or we are linking statically.
if (pSym.isAbsolute() &&
(pSym.binding() == ResolveInfo::Local || config().isCodeStatic()))
(pSym.binding() == ResolveInfo::Local || !isSymbolPreemptible(pSym) ||
Copy link
Contributor

@quic-seaswara quic-seaswara Mar 10, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This fix needs to be part of a different PR.

Please raise a issue and fix accordingly.

Copy link
Contributor Author

@quic-areg quic-areg Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

@quic-seaswara quic-seaswara Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor Author

@quic-areg quic-areg Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I can push it to the release branch when it's needed

Copy link
Contributor

@quic-seaswara quic-seaswara Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bug fixes / features need to be separated. Please push that as a separate PR.

Copy link
Contributor

@quic-seaswara quic-seaswara Mar 11, 2026

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Also I am not sure if absolute symbols ever need a dynamic relocation, should it return false for all cases ?

config().isCodeStatic()))
return false;
if (config().isCodeIndep() && isAbsReloc)
return true;
Expand Down Expand Up @@ -3981,17 +3981,30 @@ const LDSymbol *GNULDBackend::getEntrySymbol() const {
return entrySymbol;
}

bool GNULDBackend::isTargetReadOnly(const ELFSection &input) const {
if (!input.isAlloc() || input.isWritable())
return false;

if (input.getOutputELFSection()->isWritable())
return false;

for (const RuleContainer *rule : *input.getOutputSection()) {
if (rule->getSection()->isWritable())
return false;
for (const ELFSection *matched : rule->getMatchedInputSections())
if (matched->isWritable())
return false;
}
return true;
}

void GNULDBackend::checkAndSetHasTextRel(const ELFSection &pSection) {
if (m_bHasTextRel)
return;

// if the target section of the dynamic relocation is ALLOCATE but is not
// writable, than we should set DF_TEXTREL
const uint32_t flag = pSection.getFlags();
if (0 == (flag & llvm::ELF::SHF_WRITE) && (flag & llvm::ELF::SHF_ALLOC))
m_bHasTextRel = true;

return;
m_bHasTextRel = isTargetReadOnly(pSection) && pSection.isAlloc();
}

/// sortRelocation - sort the dynamic relocations to let dynamic linker
Expand Down
Loading
Loading