-
Notifications
You must be signed in to change notification settings - Fork 11.9k
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
[RISCV] Add Zicfiss support to the shadow call stack implementation. #68075
Changes from all 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 |
---|---|---|
|
@@ -27,6 +27,12 @@ | |
// 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 | ||
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. 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" | ||
// DEFAULT-NOT: "-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", | ||
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 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()">; |
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 whenZicfiss
is 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-stack
behavior based on-mcpu
is 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.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)