Skip to content

Commit

Permalink
Cherry-pick f442fbe222f3. rdar://126892345
Browse files Browse the repository at this point in the history
    Make it harder to get a PAC signing gadget in JIT code.
    https://bugs.webkit.org/show_bug.cgi?id=272750
    rdar://125596635

    Reviewed by Yusuke Suzuki.

    Right now if an attacker can control where code is allocated they can overlap code to create a PAC bypass.
    This patch makes that harder (in the WebContent process) by only allowing pacibsp and pacizb. This means
    that during arity fixup we now tag the return PC with pacizb. This is ok because we don't use the zero
    diversifier for anything. For reifying inlined call frames during OSR exit things are a bit more complicated.
    First we have be careful to only move signed return addresses into lr then untag them there. Also, we have
    to shuffle SP to point to where it would in reified frame. This means that there is technically live data
    below our SP, which on many OSes causes problems. Talking to our kernel folks however this isn't a problem
    as long as we don't have any signal handlers or run lldb expressions in this window. We don't use signal
    handlers in the WebContent process and this patch tries to limit/document the window of JIT code where lldb
    would trash the stack.

    * Source/JavaScriptCore/assembler/MacroAssemblerARM64E.h:
    (JSC::MacroAssemblerARM64E::tagPtr):
    * Source/JavaScriptCore/dfg/DFGOSRExitCompilerCommon.cpp:
    (JSC::DFG::reifyInlinedCallFrames):
    (JSC::AssemblyHelpers::transferReturnPC):
    * Source/JavaScriptCore/jit/ThunkGenerators.cpp:
    (JSC::arityFixupGenerator):
    * Source/JavaScriptCore/llint/LLIntThunks.cpp:
    (JSC::LLInt::tagGateThunk):
    (JSC::LLInt::untagGateThunk):
    * Source/JavaScriptCore/runtime/OptionsList.h:
    * Source/WTF/wtf/PtrTag.h:
    * Source/WebKit/WebProcess/WebProcess.cpp:
    (WebKit::WebProcess::initializeProcess):
    * Tools/Scripts/run-jsc-stress-tests:

    Canonical link: https://commits.webkit.org/272448.948@safari-7618-branch

Canonical link: https://commits.webkit.org/277149.25@safari-7619.1.9-branch
  • Loading branch information
kmiller68 authored and Dan Robson committed Apr 23, 2024
1 parent 3262080 commit ffd05ad
Show file tree
Hide file tree
Showing 9 changed files with 118 additions and 25 deletions.
12 changes: 12 additions & 0 deletions Source/JavaScriptCore/assembler/MacroAssemblerARM64E.h
Original file line number Diff line number Diff line change
Expand Up @@ -65,6 +65,12 @@ class MacroAssemblerARM64E : public MacroAssemblerARM64 {

ALWAYS_INLINE void tagPtr(PtrTag tag, RegisterID target)
{
if (!tag) {
m_assembler.pacizb(target);
return;
}

RELEASE_ASSERT(Options::allowNonSPTagging());
auto tagGPR = getCachedDataTempRegisterIDAndInvalidate();
move(TrustedImm64(tag), tagGPR);
m_assembler.pacib(target, tagGPR);
Expand All @@ -76,11 +82,17 @@ class MacroAssemblerARM64E : public MacroAssemblerARM64 {
m_assembler.pacibsp();
return;
}
RELEASE_ASSERT(Options::allowNonSPTagging());
m_assembler.pacib(target, tag);
}

ALWAYS_INLINE void untagPtr(PtrTag tag, RegisterID target)
{
if (!tag) {
m_assembler.autizb(target);
return;
}

auto tagGPR = getCachedDataTempRegisterIDAndInvalidate();
move(TrustedImm64(tag), tagGPR);
m_assembler.autib(target, tagGPR);
Expand Down
50 changes: 40 additions & 10 deletions Source/JavaScriptCore/dfg/DFGOSRExitCompilerCommon.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -252,6 +252,16 @@ void reifyInlinedCallFrames(CCallHelpers& jit, const OSRExitBase& exit)
ASSERT(JITCode::isBaselineCode(jit.baselineCodeBlock()->jitType()));
jit.storePtr(AssemblyHelpers::TrustedImmPtr(jit.baselineCodeBlock()), AssemblyHelpers::addressFor(CallFrameSlot::codeBlock));

GPRReg returnPCReg = GPRInfo::regT5;
#if CPU(ARM64E)
GPRReg signingTagReg = GPRInfo::regT2;
if (!Options::allowNonSPTagging()) {
returnPCReg = ARM64Registers::lr;
signingTagReg = MacroAssembler::stackPointerRegister;
// We could save/restore lr here but we don't need to because the LLInt/Baseline will load it from the stack before returning anyway.
}
#endif

const CodeOrigin* codeOrigin;
for (codeOrigin = &exit.m_codeOrigin; codeOrigin && codeOrigin->inlineCallFrame(); codeOrigin = codeOrigin->inlineCallFrame()->getCallerSkippingTailCalls()) {
InlineCallFrame* inlineCallFrame = codeOrigin->inlineCallFrame();
Expand All @@ -264,15 +274,25 @@ void reifyInlinedCallFrames(CCallHelpers& jit, const OSRExitBase& exit)

if (!trueCaller) {
ASSERT(inlineCallFrame->isTail());
jit.loadPtr(AssemblyHelpers::Address(GPRInfo::callFrameRegister, CallFrame::returnPCOffset()), GPRInfo::regT3);
jit.loadPtr(AssemblyHelpers::Address(GPRInfo::callFrameRegister, CallFrame::returnPCOffset()), returnPCReg);
#if CPU(ARM64E)
if (!Options::allowNonSPTagging()) {
JIT_COMMENT(jit, "lldb dynamic execution / posix signals could trash your stack"); // We don't have to worry about signals because they shouldn't fire in WebContent process in this window.
jit.move(MacroAssembler::stackPointerRegister, GPRInfo::regT4);
}

jit.addPtr(AssemblyHelpers::TrustedImm32(sizeof(CallerFrameAndPC)), GPRInfo::callFrameRegister, GPRInfo::regT2);
jit.untagPtr(GPRInfo::regT2, GPRInfo::regT3);
jit.addPtr(AssemblyHelpers::TrustedImm32(inlineCallFrame->returnPCOffset() + sizeof(void*)), GPRInfo::callFrameRegister, GPRInfo::regT2);
jit.validateUntaggedPtr(GPRInfo::regT3, GPRInfo::regT4);
jit.tagPtr(GPRInfo::regT2, GPRInfo::regT3);
jit.untagPtr(GPRInfo::regT2, returnPCReg);
jit.validateUntaggedPtr(returnPCReg, GPRInfo::regT2);
jit.addPtr(AssemblyHelpers::TrustedImm32(inlineCallFrame->returnPCOffset() + sizeof(CPURegister)), GPRInfo::callFrameRegister, signingTagReg);
jit.tagPtr(signingTagReg, returnPCReg);

if (!Options::allowNonSPTagging()) {
JIT_COMMENT(jit, "lldb dynamic execution / posix signals are ok again");
jit.move(GPRInfo::regT4, MacroAssembler::stackPointerRegister);
}
#endif
jit.storePtr(GPRInfo::regT3, AssemblyHelpers::addressForByteOffset(inlineCallFrame->returnPCOffset()));
jit.storePtr(returnPCReg, AssemblyHelpers::addressForByteOffset(inlineCallFrame->returnPCOffset()));
jit.loadPtr(AssemblyHelpers::Address(GPRInfo::callFrameRegister, CallFrame::callerFrameOffset()), GPRInfo::regT3);
callerFrameGPR = GPRInfo::regT3;
} else {
Expand All @@ -289,10 +309,20 @@ void reifyInlinedCallFrames(CCallHelpers& jit, const OSRExitBase& exit)
}

#if CPU(ARM64E)
jit.addPtr(AssemblyHelpers::TrustedImm32(inlineCallFrame->returnPCOffset() + sizeof(void*)), GPRInfo::callFrameRegister, GPRInfo::regT2);
jit.move(AssemblyHelpers::TrustedImmPtr(jumpTarget.untaggedPtr()), GPRInfo::regT4);
jit.tagPtr(GPRInfo::regT2, GPRInfo::regT4);
jit.storePtr(GPRInfo::regT4, AssemblyHelpers::addressForByteOffset(inlineCallFrame->returnPCOffset()));
if (!Options::allowNonSPTagging()) {
JIT_COMMENT(jit, "lldb dynamic execution / posix signals could trash your stack"); // We don't have to worry about signals because they shouldn't fire in WebContent process in this window.
jit.move(MacroAssembler::stackPointerRegister, GPRInfo::regT4);
}

jit.addPtr(AssemblyHelpers::TrustedImm32(inlineCallFrame->returnPCOffset() + sizeof(CPURegister)), GPRInfo::callFrameRegister, signingTagReg);
jit.move(AssemblyHelpers::TrustedImmPtr(jumpTarget.untaggedPtr()), returnPCReg);
jit.tagPtr(signingTagReg, returnPCReg);
jit.storePtr(returnPCReg, AssemblyHelpers::addressForByteOffset(inlineCallFrame->returnPCOffset()));

if (!Options::allowNonSPTagging()) {
JIT_COMMENT(jit, "lldb dynamic execution / posix signals are ok again");
jit.move(GPRInfo::regT4, MacroAssembler::stackPointerRegister);
}
#else
jit.storePtr(AssemblyHelpers::TrustedImmPtr(jumpTarget.untaggedPtr()), AssemblyHelpers::addressForByteOffset(inlineCallFrame->returnPCOffset()));
#endif
Expand Down
35 changes: 26 additions & 9 deletions Source/JavaScriptCore/jit/ThunkGenerators.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -626,9 +626,9 @@ MacroAssemblerCodeRef<JITThunkPtrTag> arityFixupGenerator(VM& vm)
jit.addPtr(JSInterfaceJIT::TrustedImm32(sizeof(CallerFrameAndPC)), GPRInfo::callFrameRegister, extraTemp);
jit.untagPtr(extraTemp, GPRInfo::regT3);
jit.validateUntaggedPtr(GPRInfo::regT3, extraTemp);
PtrTag tempReturnPCTag = static_cast<PtrTag>(random());
jit.move(JSInterfaceJIT::TrustedImmPtr(tempReturnPCTag), extraTemp);
jit.tagPtr(extraTemp, GPRInfo::regT3);
// We don't want to pass a tag register to pacib because it could get hijacked to make a PAC bypass gadget so we use pacizb instead.
static_assert(NoPtrTag == static_cast<PtrTag>(0));
jit.tagPtr(NoPtrTag, GPRInfo::regT3);
jit.storePtr(GPRInfo::regT3, JSInterfaceJIT::Address(GPRInfo::callFrameRegister, CallFrame::returnPCOffset()));
#endif
jit.move(JSInterfaceJIT::callFrameRegister, JSInterfaceJIT::regT3);
Expand Down Expand Up @@ -697,18 +697,35 @@ MacroAssemblerCodeRef<JITThunkPtrTag> arityFixupGenerator(VM& vm)

#if CPU(ARM64E)
jit.loadPtr(JSInterfaceJIT::Address(GPRInfo::callFrameRegister, CallFrame::returnPCOffset()), GPRInfo::regT3);
jit.move(JSInterfaceJIT::TrustedImmPtr(tempReturnPCTag), extraTemp);
jit.untagPtr(extraTemp, GPRInfo::regT3);
jit.validateUntaggedPtr(GPRInfo::regT3, extraTemp);
jit.addPtr(JSInterfaceJIT::TrustedImm32(sizeof(CallerFrameAndPC)), GPRInfo::callFrameRegister, extraTemp);
jit.tagPtr(extraTemp, GPRInfo::regT3);
jit.storePtr(GPRInfo::regT3, JSInterfaceJIT::Address(GPRInfo::callFrameRegister, CallFrame::returnPCOffset()));
if (Options::allowNonSPTagging()) {
static_assert(NoPtrTag == static_cast<PtrTag>(0));
jit.untagPtr(NoPtrTag, GPRInfo::regT3);
jit.validateUntaggedPtr(GPRInfo::regT3, extraTemp);
jit.addPtr(JSInterfaceJIT::TrustedImm32(sizeof(CallerFrameAndPC)), GPRInfo::callFrameRegister, extraTemp);
jit.tagPtr(extraTemp, GPRInfo::regT3);
jit.storePtr(GPRInfo::regT3, JSInterfaceJIT::Address(GPRInfo::callFrameRegister, CallFrame::returnPCOffset()));
} else {
jit.move(ARM64Registers::lr, JSInterfaceJIT::argumentGPR0);
jit.move(ARM64Registers::sp, JSInterfaceJIT::argumentGPR1);
JIT_COMMENT(jit, "lldb dynamic execution / posix signals could trash your stack"); // We don't have to worry about signals because they shouldn't fire in WebContent process in this window.
jit.addPtr(JSInterfaceJIT::TrustedImm32(sizeof(CallerFrameAndPC)), GPRInfo::callFrameRegister, ARM64Registers::sp);
jit.move(GPRInfo::regT3, ARM64Registers::lr);
static_assert(NoPtrTag == static_cast<PtrTag>(0));
jit.untagPtr(NoPtrTag, ARM64Registers::lr);
jit.validateUntaggedPtr(ARM64Registers::lr, extraTemp);
jit.tagReturnAddress();
jit.storePtr(ARM64Registers::lr, JSInterfaceJIT::Address(GPRInfo::callFrameRegister, CallFrame::returnPCOffset()));
JIT_COMMENT(jit, "lldb dynamic execution / posix signals are ok again");
jit.move(JSInterfaceJIT::argumentGPR1, ARM64Registers::sp);
jit.move(JSInterfaceJIT::argumentGPR0, ARM64Registers::lr);
}
#endif

#if CPU(X86_64)
jit.push(JSInterfaceJIT::regT4);
#endif
jit.ret();

#else // USE(JSVALUE64) section above, USE(JSVALUE32_64) section below.
jit.move(JSInterfaceJIT::callFrameRegister, JSInterfaceJIT::regT3);
jit.load32(JSInterfaceJIT::addressFor(CallFrameSlot::argumentCountIncludingThis), JSInterfaceJIT::argumentGPR2);
Expand Down
23 changes: 18 additions & 5 deletions Source/JavaScriptCore/llint/LLIntThunks.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -668,9 +668,22 @@ MacroAssemblerCodeRef<NativeToJITGatePtrTag> tagGateThunk(void* pointer)
{
CCallHelpers jit;

jit.addPtr(CCallHelpers::TrustedImm32(16), GPRInfo::callFrameRegister, GPRInfo::regT3);
jit.tagPtr(GPRInfo::regT3, ARM64Registers::lr);
jit.storePtr(ARM64Registers::lr, CCallHelpers::Address(GPRInfo::callFrameRegister, 8));
GPRReg signingTagReg = GPRInfo::regT3;
if (!Options::allowNonSPTagging()) {
JIT_COMMENT(jit, "lldb dynamic execution / posix signals could trash your stack"); // We don't have to worry about signals because they shouldn't fire in WebContent process in this window.
jit.move(MacroAssembler::stackPointerRegister, GPRInfo::regT3);
signingTagReg = MacroAssembler::stackPointerRegister;
}

jit.addPtr(CCallHelpers::TrustedImm32(sizeof(CallerFrameAndPC)), GPRInfo::callFrameRegister, signingTagReg);
jit.tagPtr(signingTagReg, ARM64Registers::lr);
jit.storePtr(ARM64Registers::lr, CCallHelpers::Address(GPRInfo::callFrameRegister, sizeof(CPURegister)));

if (!Options::allowNonSPTagging()) {
JIT_COMMENT(jit, "lldb dynamic execution / posix signals are ok again");
jit.move(GPRInfo::regT3, MacroAssembler::stackPointerRegister);
}

jit.move(CCallHelpers::TrustedImmPtr(pointer), GPRInfo::regT3);
jit.farJump(GPRInfo::regT3, OperationPtrTag);

Expand All @@ -682,8 +695,8 @@ MacroAssemblerCodeRef<NativeToJITGatePtrTag> untagGateThunk(void* pointer)
{
CCallHelpers jit;

jit.loadPtr(CCallHelpers::Address(GPRInfo::callFrameRegister, 8), ARM64Registers::lr);
jit.addPtr(CCallHelpers::TrustedImm32(16), GPRInfo::callFrameRegister, GPRInfo::regT3);
jit.loadPtr(CCallHelpers::Address(GPRInfo::callFrameRegister, sizeof(CPURegister)), ARM64Registers::lr);
jit.addPtr(CCallHelpers::TrustedImm32(sizeof(CallerFrameAndPC)), GPRInfo::callFrameRegister, GPRInfo::regT3);
jit.untagPtr(GPRInfo::regT3, ARM64Registers::lr);
jit.validateUntaggedPtr(ARM64Registers::lr, GPRInfo::regT3);
jit.move(CCallHelpers::TrustedImmPtr(pointer), GPRInfo::regT3);
Expand Down
10 changes: 10 additions & 0 deletions Source/JavaScriptCore/runtime/Options.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -898,6 +898,11 @@ void Options::notifyOptionsChanged()
}
#endif

// We can't use our pacibsp system while using posix signals because the signal handler could trash our stack during reifyInlinedCallFrames.
// If we have JITCage we don't need to restrict ourselves to pacibsp.
if (!Options::useMachForExceptions() || Options::useJITCage())
Options::allowNonSPTagging() = true;

if (!Options::useWasmFaultSignalHandler())
Options::useWebAssemblyFastMemory() = false;

Expand Down Expand Up @@ -1268,6 +1273,11 @@ void Options::assertOptionsAreCoherent()
coherent = false;
dataLogLn("Bytecode profiler is not concurrent JIT safe.");
}
if (!allowNonSPTagging() && !useMachForExceptions()) {
coherent = false;
dataLog("INCOHERENT OPTIONS: can't restrict pointer tagging to pacibsp and use posix signals");
}

if (!coherent)
CRASH();
}
Expand Down
1 change: 1 addition & 0 deletions Source/JavaScriptCore/runtime/OptionsList.h
Original file line number Diff line number Diff line change
Expand Up @@ -476,6 +476,7 @@ bool hasCapacityToUseLargeGigacage();
v(Bool, usePollingTraps, false, Normal, "use polling (instead of signalling) VM traps") \
\
v(Bool, useMachForExceptions, true, Normal, "Use mach exceptions rather than signals to handle faults and pass thread messages. (This does nothing on platforms without mach)") \
v(Bool, allowNonSPTagging, true, Normal, "allow use of the pacib instruction instead of just pacibsp (This can break lldb/posix signals as it puts live data below SP)") \
\
v(Bool, useICStats, false, Normal, nullptr) \
\
Expand Down
2 changes: 1 addition & 1 deletion Source/WTF/wtf/PtrTag.h
Original file line number Diff line number Diff line change
Expand Up @@ -50,7 +50,7 @@ enum class PACKeyType {
FOR_EACH_ADDITIONAL_WTF_PTRTAG(v) \

enum PtrTag : uintptr_t {
NoPtrTag,
NoPtrTag = 0, // Note: We use the 0 tag for temporarily holding the return PC during JSC's arity fixup.
CFunctionPtrTag,
};

Expand Down
6 changes: 6 additions & 0 deletions Source/WebKit/WebProcess/WebProcess.cpp
Original file line number Diff line number Diff line change
Expand Up @@ -362,6 +362,12 @@ void WebProcess::initializeProcess(const AuxiliaryProcessInitializationParameter
{
WTF::setProcessPrivileges({ });

{
JSC::Options::AllowUnfinalizedAccessScope scope;
JSC::Options::allowNonSPTagging() = false;
JSC::Options::notifyOptionsChanged();
}

MessagePortChannelProvider::setSharedProvider(WebMessagePortChannelProvider::singleton());

platformInitializeProcess(parameters);
Expand Down
4 changes: 4 additions & 0 deletions Tools/Scripts/run-jsc-stress-tests
Original file line number Diff line number Diff line change
Expand Up @@ -1130,6 +1130,7 @@ BASE_MODES = [
"misc-ftl-no-cjit",
[
"--useDataICInFTL=true",
"--allowNonSPTagging=false",
] +
FTL_OPTIONS +
NO_CJIT_OPTIONS
Expand Down Expand Up @@ -1157,6 +1158,7 @@ BASE_MODES = [
"--airForceIRCAllocator=true",
"--useDataICInFTL=true",
"--forceUnlinkedDFG=true",
"--allowNonSPTagging=false",
] +
FTL_OPTIONS +
NO_CJIT_OPTIONS
Expand Down Expand Up @@ -1218,6 +1220,7 @@ BASE_MODES = [
"--useRandomizingExecutableIslandAllocation=true",
"--forcePolyProto=true",
"--useDataICInFTL=true",
"--allowNonSPTagging=false",
] +
FTL_OPTIONS +
EAGER_OPTIONS +
Expand Down Expand Up @@ -1281,6 +1284,7 @@ BASE_MODES = [
"ftl-no-cjit-no-access-inlining",
[
"--useAccessInlining=false",
"--allowNonSPTagging=false",
] +
FTL_OPTIONS +
NO_CJIT_OPTIONS
Expand Down

0 comments on commit ffd05ad

Please sign in to comment.