-
Notifications
You must be signed in to change notification settings - Fork 16.8k
[RISCV] Add Zicfiss support to the shadow call stack implementation. #68075
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.
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
Changes from 6 commits
faed2ea
fd1bb8c
7ee57c4
f99f73f
82411aa
8c3bbce
faa6768
eced3d8
70350f4
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -57,11 +57,16 @@ compiled application or the operating system. Integrating the runtime into | |
| the operating system should be preferred since otherwise all thread creation | ||
| and destruction would need to be intercepted by the application. | ||
|
|
||
| The instrumentation makes use of the platform register ``x18`` on AArch64 and | ||
| ``x3`` (``gp``) on RISC-V. For simplicity we will refer to this as the | ||
| ``SCSReg``. On some platforms, ``SCSReg`` is reserved, and on others, it is | ||
| designated as a scratch register. This generally means that any code that may | ||
| run on the same thread as code compiled with ShadowCallStack must either target | ||
| The instrumentation makes use of the platform register ``x18`` on AArch64, | ||
| ``x3`` (``gp``) on RISC-V with software shadow stack and ``ssp`` on RISC-V with | ||
| hardware shadow stack, which needs `Zicfiss`_ and ``-mno-forced-sw-shadow-stack`` | ||
| (default option). Note that with ``Zicfiss``_ the RISC-V backend will default to | ||
| the hardware based shadow call stack. Users can force the RISC-V backend to | ||
| generate the software shadow call stack with ``Zicfiss``_ by passing | ||
| ``-mforced-sw-shadow-stack``. | ||
| For simplicity we will refer to this as the ``SCSReg``. On some platforms, | ||
| ``SCSReg`` is reserved, and on others, it is designated as a scratch register. | ||
| This generally means that any code that may run on the same thread as code compiled with ShadowCallStack must either target | ||
|
||
| one of the platforms whose ABI reserves ``SCSReg`` (currently Android, Darwin, | ||
| Fuchsia and Windows) or be compiled with a flag to reserve that register (e.g., | ||
| ``-ffixed-x18``). If absolutely necessary, code compiled without reserving the | ||
|
|
@@ -70,6 +75,7 @@ saving the register value temporarily on the stack (`example in Android`_) but | |
| this should be done with care since it risks leaking the shadow call stack | ||
| address. | ||
|
|
||
| .. _`Zicfiss`: https://github.com/riscv/riscv-cfi/blob/main/cfi_backward.adoc | ||
| .. _`example in Android`: https://android-review.googlesource.com/c/platform/frameworks/base/+/803717 | ||
|
|
||
| Because it requires a dedicated register, the ShadowCallStack feature is | ||
|
|
@@ -151,9 +157,12 @@ Usage | |
|
|
||
| To enable ShadowCallStack, just pass the ``-fsanitize=shadow-call-stack`` flag | ||
| to both compile and link command lines. On aarch64, you also need to pass | ||
| ``-ffixed-x18`` unless your target already reserves ``x18``. On RISC-V, ``x3`` | ||
| (``gp``) is always reserved. It is, however, important to disable GP relaxation | ||
| in the linker. This can be done with the ``--no-relax-gp`` flag in GNU ld. | ||
| ``-ffixed-x18`` unless your target already reserves ``x18``. No additional flags | ||
| need to be passed on RISC-V because the software based shadow stack uses ``x3`` (``gp``), | ||
| which is always reserved, and the hardware based shadow call stack uses a dedicated register, ``ssp``. | ||
|
||
| However, it is important to disable GP relaxation in the linker when using the | ||
| software based shadow call stack on RISC-V. This can be done with the | ||
| ``--no-relax-gp`` flag in GNU ld, and is off by default in LLD. | ||
|
|
||
| Low-level API | ||
| ------------- | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -27,6 +27,11 @@ | |
| // DEFAULT-NOT: "-target-feature" "-save-restore" | ||
| // DEFAULT-NOT: "-target-feature" "+save-restore" | ||
|
|
||
| // RUN: %clang --target=riscv32-unknown-elf -### %s -mforced-sw-shadow-stack 2>&1 | FileCheck %s -check-prefix=FORCE-SW-SCS | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. After commenting about defaults, we should probably test the default, too.
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Done. |
||
| // RUN: %clang --target=riscv32-unknown-elf -### %s -mno-forced-sw-shadow-stack 2>&1 | FileCheck %s -check-prefix=NO-FORCE-SW-SCS | ||
| // FORCE-SW-SCS: "-target-feature" "+forced-sw-shadow-stack" | ||
| // NO-FORCE-SW-SCS: "-target-feature" "-forced-sw-shadow-stack" | ||
|
|
||
| // RUN: %clang --target=riscv32-unknown-elf -### %s -munaligned-access 2>&1 | FileCheck %s -check-prefix=FAST-UNALIGNED-ACCESS | ||
| // RUN: %clang --target=riscv32-unknown-elf -### %s -mno-unaligned-access 2>&1 | FileCheck %s -check-prefix=NO-FAST-UNALIGNED-ACCESS | ||
| // RUN: %clang --target=riscv32-unknown-elf -### %s -mno-strict-align 2>&1 | FileCheck %s -check-prefix=FAST-UNALIGNED-ACCESS | ||
|
|
||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -1044,3 +1044,8 @@ def FeatureTaggedGlobals : SubtargetFeature<"tagged-globals", | |
| "AllowTaggedGlobals", | ||
| "true", "Use an instruction sequence for taking the address of a global " | ||
| "that allows a memory tag in the upper address bits">; | ||
|
|
||
| def FeatureForcedSWShadowStack : SubtargetFeature< | ||
| "forced-sw-shadow-stack", "HasForcedSWShadowStack", "true", | ||
|
Contributor
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Should the default be
Contributor
Author
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. This |
||
| "Implement shadow stack with software.">; | ||
| def HasForcedSWShadowStack : Predicate<"Subtarget->hasForcedSWShadowStack()">; | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
|
|
@@ -51,9 +51,15 @@ static void emitSCSPrologue(MachineFunction &MF, MachineBasicBlock &MBB, | |
| CSI, [&](CalleeSavedInfo &CSR) { return CSR.getReg() == RAReg; })) | ||
| return; | ||
|
|
||
| const RISCVInstrInfo *TII = STI.getInstrInfo(); | ||
| if (!STI.hasForcedSWShadowStack() && | ||
| STI.hasFeature(RISCV::FeatureStdExtZicfiss)) { | ||
|
||
| BuildMI(MBB, MI, DL, TII->get(RISCV::SSPUSH)).addReg(RAReg); | ||
| return; | ||
| } | ||
|
|
||
| Register SCSPReg = RISCVABI::getSCSPReg(); | ||
|
|
||
| const RISCVInstrInfo *TII = STI.getInstrInfo(); | ||
| bool IsRV64 = STI.hasFeature(RISCV::Feature64Bit); | ||
| int64_t SlotSize = STI.getXLen() / 8; | ||
| // Store return address to shadow call stack | ||
|
|
@@ -106,9 +112,15 @@ static void emitSCSEpilogue(MachineFunction &MF, MachineBasicBlock &MBB, | |
| CSI, [&](CalleeSavedInfo &CSR) { return CSR.getReg() == RAReg; })) | ||
| return; | ||
|
|
||
| const RISCVInstrInfo *TII = STI.getInstrInfo(); | ||
| if (!STI.hasForcedSWShadowStack() && | ||
| STI.hasFeature(RISCV::FeatureStdExtZicfiss)) { | ||
|
||
| BuildMI(MBB, MI, DL, TII->get(RISCV::SSPOPCHK)).addReg(RAReg); | ||
| return; | ||
| } | ||
|
|
||
| Register SCSPReg = RISCVABI::getSCSPReg(); | ||
|
|
||
| const RISCVInstrInfo *TII = STI.getInstrInfo(); | ||
| bool IsRV64 = STI.hasFeature(RISCV::Feature64Bit); | ||
| int64_t SlotSize = STI.getXLen() / 8; | ||
| // Load return address from shadow call stack | ||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think that the default we want is
use SW shadow stack, right? I think that using the SW based SCS should be something users opt into whenZicfissis available, and shouldn't be something they have to opt out of...There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Should Android default to shadow stack?
My concern is that if a user passes an -mcpu that supports Zicfiss, but the OS doesn't, the compiler will emit shadow stack instructions that silently fall back to NOPs due to the missing OS support. This leaves your binary in a state where it gets no protection.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, changing
-fsanitize=shadow-call-stackbehavior based on-mcpuis problematic, especially when it can result in the program silently falling back to unprotected state. This might be a problem for other platforms too, not only Android.Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@enh-google @appujee @samitolvanen @ilovepi @kito-cheng What should the default behavior be. Changing based on -mcpu seems surprising to me.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I guess I'm in the minority here in thinking that the compiler should pick based on the HW capabilities. I think I'd be surprised if I was compiling w/ SCS on HW that supported it and got the software SCS... but at the end of the day, as long as we document the usage clearly its probably fine. I acknowledge the concern about falling back to a less secure method, but it still feels like the wrong tradeoff to me. That said, if there's a consensus that the other way makes more sense(which there seems to be), then I'm 100% fine with that.
Related: what are our thoughts about a frontend warning/diagnostic when the mcpu supports the feature, but hasn't been selected?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
isn't the problem there that it's unlikely to be a decision that's in the developer's hands? (because the real question is "does the OS support it?", and i think that's the counter-suggestion? "decide based on the whole triple", in effect, since taking Android as an example it's likely to be something like "if android && riscv64 && api level >= where you can assume hardware scs works".)
(apologies if this isn't what's really being talked about here --- i came in very late to a long conversation :-) )
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, I think your comment is on point.
w.r.t. the diagnostic, my thoughts were that it at least makes sense to point out the need for the developer to double check the flags. Maybe i'm being overly cautious, though.
I think using the whole triple is a good approach for platforms like Android and Fuchsia, where we can enable/disable certain features based on the target OS/SDK version. I don' think we can generalize that to other systems, but if some systems can automagically do the right thing its better than nothing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's really (non-Android) Linux that's the odd one out due to not having a version number in the triple?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(Though you still have the related problem that some older LLVM may not know that OS version X supports it, and still default it to off)