ELF: Only rewrite non-preemptible IFUNCs to IPLT functions if a non-IRELATIVE relocation is needed. #133531
Conversation
Created using spr 1.3.6-beta.1
|
@llvm/pr-subscribers-lld-elf @llvm/pr-subscribers-lld Author: Peter Collingbourne (pcc) ChangesThis enables the use of IFUNC to implement custom relocation types, TODO:
Full diff: https://github.com/llvm/llvm-project/pull/133531.diff 3 Files Affected:
diff --git a/lld/ELF/Relocations.cpp b/lld/ELF/Relocations.cpp
index 629702b45965b..46117a75ccea8 100644
--- a/lld/ELF/Relocations.cpp
+++ b/lld/ELF/Relocations.cpp
@@ -869,7 +869,8 @@ static void addRelativeReloc(Ctx &ctx, InputSectionBase &isec,
// relrDyn sections don't support odd offsets. Also, relrDyn sections
// don't store the addend values, so we must write it to the relocated
// address.
- if (part.relrDyn && isec.addralign >= 2 && offsetInSec % 2 == 0) {
+ if (part.relrDyn && isec.addralign >= 2 && offsetInSec % 2 == 0 &&
+ !sym.isGnuIFunc()) {
isec.addReloc({expr, type, offsetInSec, addend, &sym});
if (shard)
part.relrDyn->relocsVec[parallel::getThreadIndex()].push_back(
@@ -1107,8 +1108,6 @@ void RelocationScanner::processAux(RelExpr expr, RelType type, uint64_t offset,
}
} else if (needsPlt(expr)) {
sym.setFlags(NEEDS_PLT);
- } else if (LLVM_UNLIKELY(isIfunc)) {
- sym.setFlags(HAS_DIRECT_RELOC);
}
// If the relocation is known to be a link-time constant, we know no dynamic
@@ -1194,6 +1193,9 @@ void RelocationScanner::processAux(RelExpr expr, RelType type, uint64_t offset,
}
}
+ if (LLVM_UNLIKELY(isIfunc && !needsGot(expr) && !needsPlt(expr)))
+ sym.setFlags(HAS_DIRECT_RELOC);
+
// When producing an executable, we can perform copy relocations (for
// STT_OBJECT) and canonical PLT (for STT_FUNC) if sym is defined by a DSO.
// Copy relocations/canonical PLT entries are unsupported for
diff --git a/lld/ELF/Symbols.h b/lld/ELF/Symbols.h
index 64f2f6eaa8d09..d4280f367d23b 100644
--- a/lld/ELF/Symbols.h
+++ b/lld/ELF/Symbols.h
@@ -42,6 +42,8 @@ void printTraceSymbol(const Symbol &sym, StringRef name);
enum {
NEEDS_GOT = 1 << 0,
NEEDS_PLT = 1 << 1,
+ // True if this is an ifunc with a direct relocation that cannot be
+ // represented as a RELATIVE relocation.
HAS_DIRECT_RELOC = 1 << 2,
// True if this symbol needs a canonical PLT entry, or (during
// postScanRelocations) a copy relocation.
diff --git a/lld/ELF/SyntheticSections.cpp b/lld/ELF/SyntheticSections.cpp
index b03c4282ab1aa..afa0482bae3ba 100644
--- a/lld/ELF/SyntheticSections.cpp
+++ b/lld/ELF/SyntheticSections.cpp
@@ -1707,9 +1707,13 @@ void RelocationBaseSection::mergeRels() {
}
void RelocationBaseSection::partitionRels() {
+ const RelType relativeRel = ctx.target->relativeRel;
+ const RelType iRelativeRel = ctx.target->iRelativeRel;
+ for (auto &r : relocs)
+ if (r.type == relativeRel && r.sym->isGnuIFunc())
+ r.type = iRelativeRel;
if (!combreloc)
return;
- const RelType relativeRel = ctx.target->relativeRel;
numRelativeRelocs =
std::stable_partition(relocs.begin(), relocs.end(),
[=](auto &r) { return r.type == relativeRel; }) -
|
…eeded. This enables the use of IFUNC to implement custom relocation types, such as the PAuth relocation types that will be introduced in a subsequent pull request. TODO: - Add tests. - Fix broken tests. Pull Request: llvm#133531
…eeded. This enables the use of IFUNC to implement custom relocation types, such as the PAuth relocation types that will be introduced in a subsequent pull request. TODO: - Add tests. - Fix broken tests. Pull Request: llvm#133531
|
This patch has a few problems
|
Created using spr 1.3.6-beta.1
pcc
changed the title
All of this is done in the latest change (except that I decided to handle the demotion in a separate loop instead of |
Created using spr 1.3.6-beta.1
|
I’m worried about the added complexity of ifunc and the extra parameter in isStaticLinkTimeConstant, a key component of relocation scanning where performance is critical. (Our scanning process is already noticeably slower than mold’s, and these factors are contributing to the gap.) If the security hardening efforts rely on ifunc, I’d strongly suggest reconsidering this approach. What’s the next PR in line? If the goal of increasing ifunc adoption is to avoid updating rtld/libc, I don’t see that as a practical solution, and I’d strongly push back against linker changes. We should focus on improving rtld/libc instead. See: https://sourceware.org/bugzilla/show_bug.cgi?id=31959" |
Fair enough, I will check the performance after this patch.
It's not so much "rely" but that the alternatives have downsides that IMO end up making them worse than the ifunc approach.
Here is the list: Structure Protection
On the contrary, I see it as more practical than the alternatives. If avoiding upgrades were the only reason for this change, I agree with you that we should be looking at a static-pie based approach instead. We may even consider dropping #133532 as it may be possible to develop another solution that uses the static-pie approach. The main reason for supporting this use case for ifuncs is to decouple the pointer encoding scheme (which is a compiler implementation detail) from the dynamic loader. For pointer field protection we have these requirements:
The following alternatives were considered:
|
There was a problem hiding this comment.
How does this work in the non-PIE (PDE) case when we take the address of an ifunc and pass it to a function in a shared library, which then compares the argument with its own global address take of the ifunc?
For example:
shared lib
typedef void Fptr(void);
extern void ifn(void);
// take address of ifunc ifn defined in application
Fptr* ifp = &ifn;
// compare address of ifn we have calculated in ifp vs
// address calculated by application, passed in fp1.
int compare(Fptr* fp1) {
return fp1 == ifp;
}
App
typedef void Fptr(void);
extern int compare(Fptr* fp1);
int val = 0;
static void impl(void) { val = 42; }
static void *resolver(void) { return impl; }
__attribute__((ifunc("resolver"))) void *ifn();
extern Fptr* fp;
int main(void) {
return compare(fp);
}
// separate file so compiler is unaware ifn is an ifunc.
typedef void Fptr(void);
extern void ifn(void);
Fptr* fp = &ifn;
Right now in the application lld produces an iPLT entry for ifn, with fp pointing to the iPLT entry. The dynamic symbol table contains the address of the iPLT entry with type STT_FUNC . The shared library and the argument compare equal.
As I understand it, this patch will change fp to point directly to the result of the ifunc resolver. So unless we also change the value put into the dynamic symbol table we'll stop comparing equal.
I don't think there's a STT_FUNC symbol we can put in the dynamic symbol table that holds the result of the ifunc resolver. GNU ld, puts the address of the resolver function with a STT_GNU_IFUNC symbol type in the dynamic symbol table. If that causes the dynamic loader to call the resolver and replace the value with the result then that would work. I haven't had time to check what glibc does though.
I'll put some more general comments below. Didn't want to make this one too long.
| @@ -1761,6 +1761,9 @@ void RelocationBaseSection::computeRels() { | |||
| llvm::sort(nonRelative, irelative, [&](auto &a, auto &b) { | |||
| return std::tie(a.r_sym, a.r_offset) < std::tie(b.r_sym, b.r_offset); | |||
| }); | |||
| llvm::sort(irelative, relocs.end(), [&](auto &a, auto &b) { | |||
There was a problem hiding this comment.
Could be worth updating the comment on line 1753, which doesn't mention irelative after non-irelative. If the r_offset is not just for readability it will be worth updating that too.
There was a problem hiding this comment.
For the irelative relocations it's also needed for output determinism. Added a comment explaining that.
lld/ELF/Symbols.h
Outdated
| @@ -42,6 +42,8 @@ void printTraceSymbol(const Symbol &sym, StringRef name); | |||
| enum { | |||
| NEEDS_GOT = 1 << 0, | |||
| NEEDS_PLT = 1 << 1, | |||
| // True if this is an ifunc with a direct relocation that cannot be | |||
There was a problem hiding this comment.
Although not new, could be worth expanding on what a direct relocation is in the comment. Could be just direct (non GOT or PLT generating) relocation ...
lld/ELF/Relocations.cpp
Outdated
| // symbols, move IRELATIVE relocations to the right place: | ||
| // - Relocations for non-demoted ifuncs are added to .rela.dyn | ||
| // - Relocations for demoted ifuncs are turned into RELATIVE relocations | ||
| // or static relocations in PDEs |
There was a problem hiding this comment.
Could you expand the acronym? I think this means Position Dependent Executable (PDE). It isn't used anywhere else in the codebase, and while derivable made me stop and think of alternatives.
|
I have some small reservations about using ifunc resolvers like this. Mostly in that we are using a mechanism invented for a different purpose, and relying on some specific linker behaviour to make this case work. This is similar to comments made in the Discourse post https://discourse.llvm.org/t/rfc-structure-protection-a-family-of-uaf-mitigation-techniques/85555/9 but repeating them here as this is closest to the implementation. As I understand it, this has a more limited and more specific use case than ifuncs. Traditional ifuncs which can be address taken or called, possibly in multiple ways, so it makes sense to use a symbol type STT_GNU_IFUNC rather than special relocation directives. The initializers for structure field protection are compiler generated, can not be legally called or address taken from user code, and only have one relocation type R_ABS64 (or 32 on a 32-bit platform). With an addition of a single relocation, something like R_ADDRINIT64 which would target a STT_FUNC resolver symbol. We can isolate the structure field initialization use case from an actual ifunc. I guess it all comes down to whether structure field initialization needs, or benefits from being distinguished from an ifunc. Ifuncs seem to be quite easy to get wrong so being able to isolate this case has some attraction to me at least. It also handles the structure field that points to an ifunc relatively gracefully. As you pointed out in your response, this does mean adding 2 relocations to every psABI that supports structure field protection rather than just one. Although I'd expect the alternative of having relocations that alternatively write "directly call" ifunc resolver or take address of function might require new relocations too? |
If a dynamic loader sees a symbol with type |
|
The canonical PLT is for int main(void) {
return compare(&ifp);
}in code models where the address is computed inline (absolute or PC-relative) rather than as an indirect load (whether from a global or a GOT entry). I like to think of canonical PLTs as the function version of copy relocations. |
I see, so the idea would be that an ADDRINIT64 pointing to an STT_FUNC would generate an IRELATIVE pointing to the symbol address if the symbol is non-preemptible and result in a linker error otherwise.
I don't necessarily see usage errors as a convincing argument for ADDRINIT64. Because the relocations and resolvers are compiler generated (and the resolvers cannot be named by the user due to the use of local symbols) it is hard to commit a usage error if you are not a compiler developer who is expected to know what they are doing. I do think that there are two convincing arguments for ADDRINIT64:
Yes, we'd still the relocation to select between calling the resolver and taking the address unless we decide to not support other use cases for the inner ifuncs (in which case we'd just generate code to take the address and the IPLT would be used as the address of affected ifuncs). This may be fine as it's the status quo for ifuncs referred to by an ABS64. |
Created using spr 1.3.6-beta.1
|
I had been assuming that this patch didn't change lld behaviour from the comment in handleNonPreemptibleIfunc() I've had some time to build and run lld on my example with this change and I notice that with this change I'm seeing the dynamic symbol table use the ifunc resolver. Whereas my old lld uses So as you say this will work and the comparison will go through OK. With that in mind this might be part of the reason why 3 tests are failing (visible in the CI): Will be good to update the comment in handleNonPreemptibleIfunc() https://github.com/llvm/llvm-project/blob/main/lld/ELF/Relocations.cpp#L1759 |
|
As a FYI, I'll be out of office till after Easter, so I may not be able to respond next week. |
This is a small optimization if the IFUNC is only address-taken in
globals and never in code, as well as enabling the use of IFUNC to
implement custom relocation types, such as the PAuth relocation types
that will be introduced in a subsequent pull request.