[RISCV] Add Zicfiss support to the shadow call stack implementation. #68075
Conversation
llvmbot
added
clang
|
This is based on #66043 . |
|
@llvm/pr-subscribers-clang-driver @llvm/pr-subscribers-llvm-support ChangesThere are two shadow stack implements with Zicfiss in spec now. Patch is 31.33 KiB, truncated to 20.00 KiB below, full version: https://github.com/llvm/llvm-project/pull/68075.diff 16 Files Affected:
diff --git a/clang/test/Preprocessor/riscv-target-features.c b/clang/test/Preprocessor/riscv-target-features.c
index 02b67dc7944ba88..163eba81543ee5b 100644
--- a/clang/test/Preprocessor/riscv-target-features.c
+++ b/clang/test/Preprocessor/riscv-target-features.c
@@ -113,6 +113,7 @@
// CHECK-NOT: __riscv_zfa {{.*$}}
// CHECK-NOT: __riscv_zfbfmin {{.*$}}
// CHECK-NOT: __riscv_zicfilp {{.*$}}
+// CHECK-NOT: __riscv_zicfiss {{.*$}}
// CHECK-NOT: __riscv_zicond {{.*$}}
// CHECK-NOT: __riscv_ztso {{.*$}}
// CHECK-NOT: __riscv_zvbb {{.*$}}
@@ -1220,3 +1221,11 @@
// RUN: -march=rv64i_zve32x_zvkt1p0 -x c -E -dM %s \
// RUN: -o - | FileCheck --check-prefix=CHECK-ZVKT-EXT %s
// CHECK-ZVKT-EXT: __riscv_zvkt 1000000{{$}}
+
+// RUN: %clang -target riscv32 -menable-experimental-extensions \
+// RUN: -march=rv32izicfiss0p3 -x c -E -dM %s \
+// RUN: -o - | FileCheck --check-prefix=CHECK-ZICFISS-EXT %s
+// RUN: %clang -target riscv64 -menable-experimental-extensions \
+// RUN: -march=rv64izicfiss0p3 -x c -E -dM %s \
+// RUN: -o - | FileCheck --check-prefix=CHECK-ZICFISS-EXT %s
+// CHECK-ZICFISS-EXT: __riscv_zicfiss 3000{{$}}
diff --git a/llvm/docs/RISCVUsage.rst b/llvm/docs/RISCVUsage.rst
index 8d12d58738c609a..31c55def0a7694a 100644
--- a/llvm/docs/RISCVUsage.rst
+++ b/llvm/docs/RISCVUsage.rst
@@ -205,6 +205,9 @@ The primary goal of experimental support is to assist in the process of ratifica
``experimental-zicfilp``
LLVM implements the `0.2 draft specification <https://github.com/riscv/riscv-cfi/releases/tag/v0.2.0>`__.
+``experimental-zicfiss``
+ LLVM implements the `0.3.1 draft specification <https://github.com/riscv/riscv-cfi/releases/tag/v0.3.1>`__.
+
``experimental-zicond``
LLVM implements the `1.0-rc1 draft specification <https://github.com/riscv/riscv-zicond/releases/tag/v1.0-rc1>`__.
diff --git a/llvm/lib/Support/RISCVISAInfo.cpp b/llvm/lib/Support/RISCVISAInfo.cpp
index a02c9842e85839a..c414b62c5f31092 100644
--- a/llvm/lib/Support/RISCVISAInfo.cpp
+++ b/llvm/lib/Support/RISCVISAInfo.cpp
@@ -170,6 +170,8 @@ static const RISCVSupportedExtension SupportedExperimentalExtensions[] = {
{"zfbfmin", RISCVExtensionVersion{0, 8}},
{"zicfilp", RISCVExtensionVersion{0, 2}},
+ {"zicfiss", RISCVExtensionVersion{0, 3}},
+
{"zicond", RISCVExtensionVersion{1, 0}},
{"ztso", RISCVExtensionVersion{0, 1}},
diff --git a/llvm/lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp b/llvm/lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp
index d561d90d3088c1a..9f088b6f3bf6492 100644
--- a/llvm/lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp
+++ b/llvm/lib/Target/RISCV/Disassembler/RISCVDisassembler.cpp
@@ -74,6 +74,17 @@ static DecodeStatus DecodeGPRRegisterClass(MCInst &Inst, uint32_t RegNo,
return MCDisassembler::Success;
}
+static DecodeStatus DecodeGPRX1X5RegisterClass(MCInst &Inst, uint32_t RegNo,
+ uint64_t Address,
+ const MCDisassembler *Decoder) {
+ MCRegister Reg = RISCV::X0 + RegNo;
+ if (Reg != RISCV::X1 && Reg != RISCV::X5)
+ return MCDisassembler::Fail;
+
+ Inst.addOperand(MCOperand::createReg(Reg));
+ return MCDisassembler::Success;
+}
+
static DecodeStatus DecodeFPR16RegisterClass(MCInst &Inst, uint32_t RegNo,
uint64_t Address,
const MCDisassembler *Decoder) {
@@ -370,6 +381,10 @@ static DecodeStatus decodeZcmpRlist(MCInst &Inst, unsigned Imm,
static DecodeStatus decodeZcmpSpimm(MCInst &Inst, unsigned Imm,
uint64_t Address, const void *Decoder);
+static DecodeStatus decodeCSSPushPopchk(MCInst &Inst, uint32_t Insn,
+ uint64_t Address,
+ const MCDisassembler *Decoder);
+
#include "RISCVGenDisassemblerTables.inc"
static DecodeStatus decodeRVCInstrRdRs1ImmZero(MCInst &Inst, uint32_t Insn,
@@ -384,6 +399,16 @@ static DecodeStatus decodeRVCInstrRdRs1ImmZero(MCInst &Inst, uint32_t Insn,
return MCDisassembler::Success;
}
+static DecodeStatus decodeCSSPushPopchk(MCInst &Inst, uint32_t Insn,
+ uint64_t Address,
+ const MCDisassembler *Decoder) {
+ uint32_t Rs1 = fieldFromInstruction(Insn, 7, 5);
+ DecodeStatus Result = DecodeGPRX1X5RegisterClass(Inst, Rs1, Address, Decoder);
+ (void)Result;
+ assert(Result == MCDisassembler::Success && "Invalid register");
+ return MCDisassembler::Success;
+}
+
static DecodeStatus decodeRVCInstrRdSImm(MCInst &Inst, uint32_t Insn,
uint64_t Address,
const MCDisassembler *Decoder) {
@@ -527,6 +552,10 @@ DecodeStatus RISCVDisassembler::getInstruction(MCInst &MI, uint64_t &Size,
"RV32Zdinx table (Double in Integer and rv32)");
TRY_TO_DECODE_FEATURE(RISCV::FeatureStdExtZfinx, DecoderTableRVZfinx32,
"RVZfinx table (Float in Integer)");
+ TRY_TO_DECODE(STI.hasFeature(RISCV::FeatureStdExtZicfiss) &&
+ !STI.hasFeature(RISCV::Feature64Bit),
+ DecoderTableRV32Zicfiss32,
+ "RV32Zicfiss table (Shadow stack and rv32)");
TRY_TO_DECODE_FEATURE(RISCV::FeatureVendorXVentanaCondOps,
DecoderTableXVentana32, "Ventana custom opcode table");
TRY_TO_DECODE_FEATURE(RISCV::FeatureVendorXTHeadBa, DecoderTableXTHeadBa32,
diff --git a/llvm/lib/Target/RISCV/RISCVFeatures.td b/llvm/lib/Target/RISCV/RISCVFeatures.td
index 6381263b37613b3..286320c6c52eb12 100644
--- a/llvm/lib/Target/RISCV/RISCVFeatures.td
+++ b/llvm/lib/Target/RISCV/RISCVFeatures.td
@@ -79,6 +79,13 @@ def HasStdExtZihintntl : Predicate<"Subtarget->hasStdExtZihintntl()">,
AssemblerPredicate<(all_of FeatureStdExtZihintntl),
"'Zihintntl' (Non-Temporal Locality Hints)">;
+def FeatureStdExtZicfiss
+ : SubtargetFeature<"experimental-zicfiss", "HasStdExtZicfiss", "true",
+ "'Zicfiss' (Shadow stack)">;
+def HasStdExtZicfiss : Predicate<"Subtarget->hasStdExtZicfiss()">,
+ AssemblerPredicate<(all_of FeatureStdExtZicfiss),
+ "'Zicfiss' (Shadow stack)">;
+
def FeatureStdExtZifencei
: SubtargetFeature<"zifencei", "HasStdExtZifencei", "true",
"'Zifencei' (fence.i)">;
diff --git a/llvm/lib/Target/RISCV/RISCVFrameLowering.cpp b/llvm/lib/Target/RISCV/RISCVFrameLowering.cpp
index add933250f8473d..3ff335bb7304773 100644
--- a/llvm/lib/Target/RISCV/RISCVFrameLowering.cpp
+++ b/llvm/lib/Target/RISCV/RISCVFrameLowering.cpp
@@ -51,9 +51,14 @@ static void emitSCSPrologue(MachineFunction &MF, MachineBasicBlock &MBB,
CSI, [&](CalleeSavedInfo &CSR) { return CSR.getReg() == RAReg; }))
return;
+ const RISCVInstrInfo *TII = STI.getInstrInfo();
+ if (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 +111,14 @@ static void emitSCSEpilogue(MachineFunction &MF, MachineBasicBlock &MBB,
CSI, [&](CalleeSavedInfo &CSR) { return CSR.getReg() == RAReg; }))
return;
+ const RISCVInstrInfo *TII = STI.getInstrInfo();
+ if (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
diff --git a/llvm/lib/Target/RISCV/RISCVInstrInfo.td b/llvm/lib/Target/RISCV/RISCVInstrInfo.td
index 582fe60fd0368e9..e6a8ba1fe2fc17d 100644
--- a/llvm/lib/Target/RISCV/RISCVInstrInfo.td
+++ b/llvm/lib/Target/RISCV/RISCVInstrInfo.td
@@ -1970,14 +1970,15 @@ include "RISCVInstrInfoZk.td"
include "RISCVInstrInfoV.td"
include "RISCVInstrInfoZvk.td"
-// Integer
-include "RISCVInstrInfoZicbo.td"
-include "RISCVInstrInfoZicond.td"
-
// Compressed
include "RISCVInstrInfoC.td"
include "RISCVInstrInfoZc.td"
+// Integer
+include "RISCVInstrInfoZicbo.td"
+include "RISCVInstrInfoZicond.td"
+include "RISCVInstrInfoZicfiss.td"
+
//===----------------------------------------------------------------------===//
// Vendor extensions
//===----------------------------------------------------------------------===//
diff --git a/llvm/lib/Target/RISCV/RISCVInstrInfoZicfiss.td b/llvm/lib/Target/RISCV/RISCVInstrInfoZicfiss.td
new file mode 100644
index 000000000000000..95864664e43f2ae
--- /dev/null
+++ b/llvm/lib/Target/RISCV/RISCVInstrInfoZicfiss.td
@@ -0,0 +1,86 @@
+//===------ RISCVInstrInfoZicfiss.td - RISC-V Zicfiss -*- tablegen -*------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+//===----------------------------------------------------------------------===//
+// Instruction class templates
+//===----------------------------------------------------------------------===//
+
+class RVC_SSInst<bits<5> rs1val, RegisterClass reg_class, string opcodestr> :
+ RVInst16<(outs), (ins reg_class:$rs1), opcodestr, "$rs1", [], InstFormatOther> {
+ let Inst{15-13} = 0b011;
+ let Inst{12} = 0;
+ let Inst{11-7} = rs1val;
+ let Inst{6-2} = 0b00000;
+ let Inst{1-0} = 0b01;
+ let DecoderMethod = "decodeCSSPushPopchk";
+}
+
+//===----------------------------------------------------------------------===//
+// Instructions
+//===----------------------------------------------------------------------===//
+
+let Uses = [SSP], hasSideEffects = 0, mayLoad = 1, mayStore = 0 in {
+let DecoderNamespace = "RV32Zicfiss", Predicates = [HasStdExtZicfiss, IsRV32] in
+def SSLW : RVInstI<0b100, OPC_SYSTEM, (outs GPRX1X5:$rd), (ins), "sslw", "$rd"> {
+ let rs1 = 0;
+ let imm12 = 0b100000011100;
+}
+
+let Predicates = [HasStdExtZicfiss, IsRV64] in
+def SSLD : RVInstI<0b100, OPC_SYSTEM, (outs GPRX1X5:$rd), (ins), "ssld", "$rd"> {
+ let rs1 = 0;
+ let imm12 = 0b100000011100;
+}
+} // Uses = [SSP], hasSideEffects = 0, mayLoad = 1, mayStore = 0
+
+let Predicates = [HasStdExtZicfiss] in {
+let Uses = [SSP], hasSideEffects = 0, mayLoad = 1, mayStore = 0 in
+def SSPOPCHK : RVInstI<0b100, OPC_SYSTEM, (outs), (ins GPRX1X5:$rs1), "sspopchk",
+ "$rs1"> {
+ let rd = 0;
+ let imm12 = 0b100000011100;
+} // Uses = [SSP], hasSideEffects = 0, mayLoad = 1, mayStore = 0
+
+let Uses = [SSP], hasSideEffects = 0, mayLoad = 0, mayStore = 0 in {
+def SSINCP : RVInstI<0b100, OPC_SYSTEM, (outs), (ins), "ssincp", ""> {
+ let imm12 = 0b100000011100;
+ let rs1 = 0b00000;
+ let rd = 0b00000;
+}
+
+def SSRDP : RVInstI<0b100, OPC_SYSTEM, (outs GPRNoX0:$rd), (ins), "ssrdp", "$rd"> {
+ let imm12 = 0b100000011101;
+ let rs1 = 0b00000;
+}
+} // Uses = [SSP], hasSideEffects = 0, mayLoad = 0, mayStore = 0
+
+let Uses = [SSP], Defs = [SSP], hasSideEffects = 0, mayLoad = 0, mayStore = 1 in
+def SSPUSH : RVInstR<0b1000001, 0b100, OPC_SYSTEM, (outs), (ins GPRX1X5:$rs2),
+ "sspush", "$rs2"> {
+ let rd = 0b00000;
+ let rs1 = 0b00000;
+}
+} // Predicates = [HasStdExtZicfiss]
+
+let Predicates = [HasStdExtZicfiss, HasStdExtCOrZca] in {
+let Uses = [SSP], Defs = [SSP], hasSideEffects = 0, mayLoad = 0, mayStore = 1 in
+def C_SSPUSH : RVC_SSInst<0b00001, GPRX1, "c.sspush">;
+
+let Uses = [SSP], Defs = [SSP], hasSideEffects = 0, mayLoad = 0, mayStore = 1 in
+def C_SSPOPCHK : RVC_SSInst<0b00101, GPRX5, "c.sspopchk">;
+} // Predicates = [HasStdExtZicfiss, HasStdExtCOrZca]
+
+let Predicates = [HasStdExtZicfiss, HasStdExtC], Uses = [SSP], Defs = [SSP],
+ hasSideEffects = 0, mayLoad = 0, mayStore = 0 in
+def C_SSINCP : RVInst16<(outs), (ins), "c.ssincp", "", [], InstFormatOther> {
+ let Inst{15-13} = 0b011;
+ let Inst{12} = 0;
+ let Inst{11-7} = 0b00011;
+ let Inst{6-2} = 0b00000;
+ let Inst{1-0} = 0b01;
+}
diff --git a/llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp b/llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp
index 3e44cf4781f643c..7934da40ac4aadc 100644
--- a/llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp
+++ b/llvm/lib/Target/RISCV/RISCVRegisterInfo.cpp
@@ -119,6 +119,9 @@ BitVector RISCVRegisterInfo::getReservedRegs(const MachineFunction &MF) const {
markSuperRegs(Reserved, RISCV::FRM);
markSuperRegs(Reserved, RISCV::FFLAGS);
+ // Shadow stack pointer.
+ markSuperRegs(Reserved, RISCV::SSP);
+
assert(checkAllSuperRegsMarked(Reserved));
return Reserved;
}
diff --git a/llvm/lib/Target/RISCV/RISCVRegisterInfo.td b/llvm/lib/Target/RISCV/RISCVRegisterInfo.td
index 1a6145f92908134..5a6ad940398cac1 100644
--- a/llvm/lib/Target/RISCV/RISCVRegisterInfo.td
+++ b/llvm/lib/Target/RISCV/RISCVRegisterInfo.td
@@ -137,6 +137,8 @@ def GPR : GPRRegisterClass<(add (sequence "X%u", 10, 17),
(sequence "X%u", 0, 4))>;
def GPRX0 : GPRRegisterClass<(add X0)>;
+def GPRX1 : GPRRegisterClass<(add X1)>;
+def GPRX5 : GPRRegisterClass<(add X5)>;
def GPRNoX0 : GPRRegisterClass<(sub GPR, X0)>;
@@ -165,6 +167,10 @@ def SP : GPRRegisterClass<(add X2)>;
def SR07 : GPRRegisterClass<(add (sequence "X%u", 8, 9),
(sequence "X%u", 18, 23))>;
+def GPRX1X5 : RegisterClass<"RISCV", [XLenVT], 32, (add X1, X5)> {
+ let RegInfos = XLenRI;
+}
+
// Floating point registers
let RegAltNameIndices = [ABIRegAltName] in {
def F0_H : RISCVReg16<0, "f0", ["ft0"]>, DwarfRegNum<[32]>;
@@ -597,3 +603,6 @@ foreach m = LMULList in {
// Special registers
def FFLAGS : RISCVReg<0, "fflags">;
def FRM : RISCVReg<0, "frm">;
+
+// Shadow Stack register
+def SSP : RISCVReg<0, "ssp">;
diff --git a/llvm/test/CodeGen/RISCV/shadowcallstack.ll b/llvm/test/CodeGen/RISCV/shadowcallstack.ll
index fee067ee3ad141c..8fbe7ed9ca07663 100644
--- a/llvm/test/CodeGen/RISCV/shadowcallstack.ll
+++ b/llvm/test/CodeGen/RISCV/shadowcallstack.ll
@@ -3,6 +3,10 @@
; RUN: | FileCheck %s --check-prefix=RV32
; RUN: llc -mtriple=riscv64 -verify-machineinstrs < %s \
; RUN: | FileCheck %s --check-prefix=RV64
+; RUN: llc -mtriple=riscv32 -mattr=+experimental-zicfiss -verify-machineinstrs < %s \
+; RUN: | FileCheck %s --check-prefix=RV32-ZICFISS
+; RUN: llc -mtriple=riscv64 -mattr=+experimental-zicfiss -verify-machineinstrs < %s \
+; RUN: | FileCheck %s --check-prefix=RV64-ZICFISS
define void @f1() shadowcallstack {
; RV32-LABEL: f1:
@@ -12,6 +16,14 @@ define void @f1() shadowcallstack {
; RV64-LABEL: f1:
; RV64: # %bb.0:
; RV64-NEXT: ret
+;
+; RV32-ZICFISS-LABEL: f1:
+; RV32-ZICFISS: # %bb.0:
+; RV32-ZICFISS-NEXT: ret
+;
+; RV64-ZICFISS-LABEL: f1:
+; RV64-ZICFISS: # %bb.0:
+; RV64-ZICFISS-NEXT: ret
ret void
}
@@ -25,6 +37,14 @@ define void @f2() shadowcallstack {
; RV64-LABEL: f2:
; RV64: # %bb.0:
; RV64-NEXT: tail foo@plt
+;
+; RV32-ZICFISS-LABEL: f2:
+; RV32-ZICFISS: # %bb.0:
+; RV32-ZICFISS-NEXT: tail foo@plt
+;
+; RV64-ZICFISS-LABEL: f2:
+; RV64-ZICFISS: # %bb.0:
+; RV64-ZICFISS-NEXT: tail foo@plt
tail call void @foo()
ret void
}
@@ -65,6 +85,32 @@ define i32 @f3() shadowcallstack {
; RV64-NEXT: addi gp, gp, -8
; RV64-NEXT: .cfi_restore gp
; RV64-NEXT: ret
+;
+; RV32-ZICFISS-LABEL: f3:
+; RV32-ZICFISS: # %bb.0:
+; RV32-ZICFISS-NEXT: sspush ra
+; RV32-ZICFISS-NEXT: addi sp, sp, -16
+; RV32-ZICFISS-NEXT: .cfi_def_cfa_offset 16
+; RV32-ZICFISS-NEXT: sw ra, 12(sp) # 4-byte Folded Spill
+; RV32-ZICFISS-NEXT: .cfi_offset ra, -4
+; RV32-ZICFISS-NEXT: call bar@plt
+; RV32-ZICFISS-NEXT: lw ra, 12(sp) # 4-byte Folded Reload
+; RV32-ZICFISS-NEXT: addi sp, sp, 16
+; RV32-ZICFISS-NEXT: sspopchk ra
+; RV32-ZICFISS-NEXT: ret
+;
+; RV64-ZICFISS-LABEL: f3:
+; RV64-ZICFISS: # %bb.0:
+; RV64-ZICFISS-NEXT: sspush ra
+; RV64-ZICFISS-NEXT: addi sp, sp, -16
+; RV64-ZICFISS-NEXT: .cfi_def_cfa_offset 16
+; RV64-ZICFISS-NEXT: sd ra, 8(sp) # 8-byte Folded Spill
+; RV64-ZICFISS-NEXT: .cfi_offset ra, -8
+; RV64-ZICFISS-NEXT: call bar@plt
+; RV64-ZICFISS-NEXT: ld ra, 8(sp) # 8-byte Folded Reload
+; RV64-ZICFISS-NEXT: addi sp, sp, 16
+; RV64-ZICFISS-NEXT: sspopchk ra
+; RV64-ZICFISS-NEXT: ret
%res = call i32 @bar()
%res1 = add i32 %res, 1
ret i32 %res
@@ -140,6 +186,68 @@ define i32 @f4() shadowcallstack {
; RV64-NEXT: addi gp, gp, -8
; RV64-NEXT: .cfi_restore gp
; RV64-NEXT: ret
+;
+; RV32-ZICFISS-LABEL: f4:
+; RV32-ZICFISS: # %bb.0:
+; RV32-ZICFISS-NEXT: sspush ra
+; RV32-ZICFISS-NEXT: addi sp, sp, -16
+; RV32-ZICFISS-NEXT: .cfi_def_cfa_offset 16
+; RV32-ZICFISS-NEXT: sw ra, 12(sp) # 4-byte Folded Spill
+; RV32-ZICFISS-NEXT: sw s0, 8(sp) # 4-byte Folded Spill
+; RV32-ZICFISS-NEXT: sw s1, 4(sp) # 4-byte Folded Spill
+; RV32-ZICFISS-NEXT: sw s2, 0(sp) # 4-byte Folded Spill
+; RV32-ZICFISS-NEXT: .cfi_offset ra, -4
+; RV32-ZICFISS-NEXT: .cfi_offset s0, -8
+; RV32-ZICFISS-NEXT: .cfi_offset s1, -12
+; RV32-ZICFISS-NEXT: .cfi_offset s2, -16
+; RV32-ZICFISS-NEXT: call bar@plt
+; RV32-ZICFISS-NEXT: mv s0, a0
+; RV32-ZICFISS-NEXT: call bar@plt
+; RV32-ZICFISS-NEXT: mv s1, a0
+; RV32-ZICFISS-NEXT: call bar@plt
+; RV32-ZICFISS-NEXT: mv s2, a0
+; RV32-ZICFISS-NEXT: call bar@plt
+; RV32-ZICFISS-NEXT: add s0, s0, s1
+; RV32-ZICFISS-NEXT: add a0, s2, a0
+; RV32-ZICFISS-NEXT: add a0, s0, a0
+; RV32-ZICFISS-NEXT: lw ra, 12(sp) # 4-byte Folded Reload
+; RV32-ZICFISS-NEXT: lw s0, 8(sp) # 4-byte Folded Reload
+; RV32-ZICFISS-NEXT: lw s1, 4(sp) # 4-byte Folded Reload
+; RV32-ZICFISS-NEXT: lw s2, 0(sp) # 4-byte Folded Reload
+; RV32-ZICFISS-NEXT: addi sp, sp, 16
+; RV32-ZICFISS-NEXT: sspopchk ra
+; RV32-ZICFISS-NEXT: ret
+;
+; RV64-ZICFISS-LABEL: f4:
+; RV64-ZICFISS: # %bb.0:
+; RV64-ZICFISS-NEXT: sspush ra
+; RV64-ZICFISS-NEXT: addi sp, sp, -32
+; RV64-ZICFISS-NEXT: .cfi_def_cfa_offset 32
+; RV64-ZICFISS-NEXT: sd ra, 24(sp) # 8-byte Folded Spill
+; RV64-ZICFISS-NEXT: sd s0, 16(sp) # 8-byte Folded Spill
+; RV64-ZICFISS-NEXT: sd s1, 8(sp) # 8-byte Folded Spill
+; RV64-ZICFISS-NEXT: sd s2, 0(sp) # 8-byte Folded Spill
+; RV64-ZICFISS-NEXT: .cfi_offset ra, -8
+; RV64-ZICFISS-NEXT: .cfi_offset s0, -16
+; RV64-ZICFISS-NEXT: .cfi_offset s1, -24
+; RV64-ZICFISS-NEXT: .cfi_offset s2, -32
+; RV64-ZICFISS-NEXT: call bar@plt
+; RV64-ZICFISS-NEXT: mv s0, a0
+; RV64-ZICFISS-NEXT: call bar@plt
+; RV64-ZICFISS-NEXT: mv s1, a0
+; RV64-ZICFISS-NEXT: call bar@plt
+; RV64-ZICFISS-NEXT: mv s2, a0
+; RV64-ZICFISS-NEXT: call bar@plt
+; RV64-ZICFISS-NEXT: add s0, s0, s1
+; RV64-ZICFISS-NEXT: add a0, s2, a0
+; RV64-ZICFISS-NEXT: addw a0, s0, a0
+; RV64-ZICFISS-NEXT: ld ra, 24(sp) # 8-byte Folded Reload
+; RV64-ZICFISS-NEXT: ld s0, 16(sp) # 8-byte Folded Reload
+; RV64-ZICFISS-NEXT: ld s1, 8(sp) # 8-byte Folded Reload
+; RV64-ZICFISS-NEXT: ld s2, 0(sp) # 8-byte Folded Reload
+; RV64-ZICFISS-NEXT: addi sp, sp, 32
+; RV64-ZICFISS-NEXT: sspopchk ra
+; RV64-ZICFISS-NEXT: ret
%res1 = call i32 @bar()
%res2 = call i32 @bar()
%res3 = call i32 @bar()
@@ -176,6 +284,28 @@ define i32 @f5() shadowcallstack nounwind {
; RV64-NEXT: ld ra, -8(gp)
; RV64-NEXT: addi gp, gp, -8
; RV64-NEXT: ret
+;
+; RV32-ZICFISS-LABEL: f5:
+; RV32-ZICFISS: # %bb.0:
+; RV32-ZICFISS-NEXT: sspush ra
+; RV32-ZICFISS-NEXT: addi sp, sp, -16
+; RV32-ZICFISS-NEXT: sw ra, 12(sp) # 4-byte Folded Spill
+; RV32-ZICFISS-NEXT: call bar@plt
+; RV32-ZICFISS-NEXT: lw ra, 12(sp) # 4-byte Folded Reload
+; RV32-ZICFISS-NEXT: addi sp, sp, 16
+; RV32-ZICFISS-NEXT: sspopchk ra
+; RV32-ZICFISS-NEXT: ret
+;
+; RV64-ZICFISS-LABEL: f5:
+; R...
[truncated]
|
|
The patch basically changes the ShadowCallStack back-end to emit an sspush/sspopchk instead of the usual SCS push/pop, which seems like a reasonable approach to me. However, it would be helpful to mention the dependency on |
Thank you for the reminder. I have updated the document by e32fc6c. |
| @@ -106,9 +111,14 @@ static void emitSCSEpilogue(MachineFunction &MF, MachineBasicBlock &MBB, | |||
| CSI, [&](CalleeSavedInfo &CSR) { return CSR.getReg() == RAReg; })) | |||
| return; | |||
|
|
|||
| const RISCVInstrInfo *TII = STI.getInstrInfo(); | |||
| if (STI.hasFeature(RISCV::FeatureStdExtZicfiss)) { | |||
There was a problem hiding this comment.
Do we need an enable other than just the feature being in -march? The shadow stack pointer has to be set up when the application starts. Is this done by the kernel?
My concern is that if your -mcpu supports Zicfiss, but the kernel doesn't, this will generate code that doesn't work.
There was a problem hiding this comment.
I think this is probably handled by the loader. Its how it is currently handled for platforms like Fuchsia and Android when SCS is enabled. I'm not totally sure on the new spec, but the linked document above makes me think that is the case with this text:
The dynamic loader should activate the use of Zicfiss extension for an application only if all executables (the application and the dependent dynamically linked libraries) used by that application use the Zicfiss extension.
An application that has the Zicfiss extension active may request the dynamic loader at runtime to load a new dynamic shared object (using dlopen() for example). If the requested object does not have the Zicfiss attribute then the dynamic loader, based on its policy (e.g, established by the operating system or the administrator) configuration, could either deny the request or deactivate the Zicfiss extension for the application. It is strongly recommended that the policy enforces a strict security posture and denies the request.
I believe that setting up this memory and initializing the SCS reg would fall under activate the use of Zicfiss..., but I could be wrong.
There was a problem hiding this comment.
The plan is: adding GNU property and then check that during loader (and check at CRT stuff for static linked binary), that's mostly same as AArch64 and x86 for their CFI stuffs, and abort if system don't have either zicfiss and zimop, something begin implemented at our end around glibc, but we just didn't document everything down yet :P
There was a problem hiding this comment.
I believe that setting up this memory and initializing the SCS reg would fall under activate the use of Zicfiss..., but I could be wrong.
That suppose initialize SCS register by kernel, more precisely is user space will invoke some prctl system call to enable SCS and the kernel will allocate SCS page and then initialize the SCS register then back to user space.
There was a problem hiding this comment.
Do we need an enable other than just the feature being in -march? The shadow stack pointer has to be set up when the application starts. Is this done by the kernel?
My concern is that if your -mcpu supports Zicfiss, but the kernel doesn't, this will generate code that doesn't work.
oh, that's good point...let me check how other target do
There was a problem hiding this comment.
Today, if you use software SCS on a platform without support they just fail, since the SCS won't be setup and gp(probably) points to invalid memory. Is it unreasonable to have it work the same in this case?
Sounds reasonable to me, but having a flag that allows one to explicitly select between software and hardware shadow stacks is still a good idea IMO.
WDYT about phrasing this an an opt-out flag, like
-force-software-scs, and using a helper function, like :
For sanitizers, extra flags have typically been something like -fsanitize-[sanitizer]-[flag], e.g. -fsanitize-cfi-icall-generalize-pointers:
https://clang.llvm.org/docs/ControlFlowIntegrity.html#indirect-function-call-checking
It also occurs to me that X86 is getting/has backward edge CFI, right? how do we enable that in the x86 backend? How is GCC planning to spell these options, since I would suppose there needs to be some level of compatibility.
X86 uses the -fcf-protection flag for backward-edge at least, and AArch64 uses -mbranch-protection for both backward-edge (PAC) and forward-edge (BTI). It would be great not to invent another command line option specifically for RISC-V, so I quite like the idea of reusing the existing ShadowCallStack front-end for hardware shadow stacks too.
There was a problem hiding this comment.
Today, if you use software SCS on a platform without support they just fail, since the SCS won't be setup and gp(probably) points to invalid memory. Is it unreasonable to have it work the same in this case?
If the hardware shadow stack isn't set up, won't the instructions become silent NOPs rather than failing?
There was a problem hiding this comment.
I thought that the scenario was that the platform only supported software, but the CPU did support the instructions, and would therefore execute SSPUSH/SSPOPCHK. I would expect those instructions fail when the memory region wasn't initialized and the SSP register would point to invalid memory.
If that's not what you meant, then I've misunderstood the scenario.
There was a problem hiding this comment.
As @samitolvanen wrote earlier the SSPUSH and SSPOPCHK instruction fall back to NOPs based on configuration.
If the kernel doesn't support Zicfiss, it presumably also hasn't enabled the extension for U-mode. Therefore, my understanding is that the application loader can simply skip shadow stack setup for the program, including the prctl (which would fail with -EINVAL), and since the Zicfiss (Zimop) instructions are no-ops when the extension is disabled, the program would run normally, just without shadow stacks. Does this sound correct?
There was a problem hiding this comment.
Ah, you're totally right. I read that bit incorrectly. Thanks for pointing that out.
| @@ -27,6 +27,11 @@ | |||
|
|
|||
| using namespace llvm; | |||
|
|
|||
| static cl::opt<bool> | |||
There was a problem hiding this comment.
is it possible to use something other than a cl::opt? I know we make heavy use of them to control codegen options, but they tend to get dropped on the floor easily if you have to forward -mllvm options to the linker in LTO/ThinLTO builds. This is a constant pain point for a lot of folks who need to support multiple build configurations, like the Linux kernel, Fuchsia, Android, or Chrome.
There was a problem hiding this comment.
The new commit 2830d3e adds a new feature to replace this. In the feature, we can add clang option to pass the feature to llvm.
| @@ -106,9 +111,14 @@ static void emitSCSEpilogue(MachineFunction &MF, MachineBasicBlock &MBB, | |||
| CSI, [&](CalleeSavedInfo &CSR) { return CSR.getReg() == RAReg; })) | |||
| return; | |||
|
|
|||
| const RISCVInstrInfo *TII = STI.getInstrInfo(); | |||
| if (STI.hasFeature(RISCV::FeatureStdExtZicfiss)) { | |||
There was a problem hiding this comment.
What if we're compiling for a platform that only uses the software shadow stack and does not support the hardware shadow stack even if the CPU supports it?
Today, if you use software SCS on a platform without support they just fail, since the SCS won't be setup and gp(probably) points to invalid memory. Is it unreasonable to have it work the same in this case?
WDYT about phrasing this an an opt-out flag, like -force-software-scs, and using a helper function, like :
bool enableHWSCS(){ return !ForceSoftwareSCS && STI.hasFeature(RISCV::FeatureStdExtZicfiss); }
My thinking is that using HW SCS on platforms that have it is a good default, which we want to encourage, but we can still allow folks to force the software implementation if they want.
It also occurs to me that X86 is getting/has backward edge CFI, right? how do we enable that in the x86 backend? How is GCC planning to spell these options, since I would suppose there needs to be some level of compatibility.
|
Rebase and ping. I also update the first comment of the first comment of this pr since the control stack mode is removed and we add new feature |
clang/docs/ShadowCallStack.rst
Outdated
| 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 ``-mllvm -riscv-hardware-shadow-stack``. |
There was a problem hiding this comment.
Was -mllvm -riscv-hardware-shadow-stack removed and replaced with forced-sw-shadow-stack feature?
There was a problem hiding this comment.
Modified. The latest patch I add new options {m,mno}-forced-sw-shadow-stack to control feature forced-sw-shadow-stack
|
How is unwinding handled? The software shadow stack documentation says this
This patch skips the emission of the unwind info. Which it should since it uses X3. But how does unwinding work with hardware shadow stack? |
|
Unwinder could use property |
llvmbot
added
the
clang:driver
clang/docs/ShadowCallStack.rst
Outdated
| ``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). ``-mforced-sw-shadow-stack`` make risc-v backend generate | ||
| software shadow stack with `Zicfiss`_ when shadow stack enabled. |
There was a problem hiding this comment.
| software shadow stack with `Zicfiss`_ when shadow stack enabled. | |
| hardware shadow stack, which needs `Zicfiss`_. 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``. |
Maybe this is easier to follow? Might also make sense to outline when -mforced-sw-shadow-stack is expected to work somewhere, e.g., it has no effect w/o `Zicfiss``. TBH, in that case, the driver should probably issue an error, since its incompatible w/o the feature, but I'm not sure if we can do that check in the frontend or not.
There was a problem hiding this comment.
Done. Thank you for your suggestion.
clang/docs/ShadowCallStack.rst
Outdated
| ``-ffixed-x18`` unless your target already reserves ``x18``. On RISC-V with software | ||
| shadow stack, ``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. |
There was a problem hiding this comment.
We reserve gp even w/o SCS. The --no-relax-gp advice does only apply to the software SCS.
Maybe something along these lines?
| ``-ffixed-x18`` unless your target already reserves ``x18``. On RISC-V with software | |
| shadow stack, ``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. |
There was a problem hiding this comment.
Done. Thank you for your suggestion.
|
The title should probably be changed to something like:
The current title doesn't make much sense to me, but maybe I've misunderstood the intent? |
yetingk
changed the title
There are two shadow stack implements with Zicfiss in [spec] now. In Shadow stack mode, programs still store the return address to regular address. In Control stack mode, programs only store the return address to shadow stack. This patch only supports the shadow stack mode. [spec]: https://github.com/riscv/riscv-cfi/blob/main/cfi_backward.adoc#push-to-and-pop-from-the-shadow-stack
…enet for shadow stack.
|
Ping. |
| @@ -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.
Should the default be false?
There was a problem hiding this comment.
This true means HasForcedSWShadowStack set to true when +forced-sw-shadow-stack. So actually the default is false.
| 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`` |
There was a problem hiding this comment.
| hardware shadow stack, which needs `Zicfiss`_ and ``-mno-forced-sw-shadow-stack`` | |
| hardware shadow stack, which needs `Zicfiss` |
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 when Zicfiss is available, and shouldn't be something they have to opt out of...
There was a problem hiding this comment.
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.
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.
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.
@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.
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.
Related: what are our thoughts about a frontend warning/diagnostic when the mcpu supports the feature, but hasn't been selected?
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.
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 :-) )
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.
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.
(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)
clang/docs/ShadowCallStack.rst
Outdated
| ``-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 |
There was a problem hiding this comment.
Nit: this line is pretty long compared to the rest of the file.
clang/docs/ShadowCallStack.rst
Outdated
| 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``. |
There was a problem hiding this comment.
Nit: this line is long. Maybe reformat the sentence/paragraph?
| @@ -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 | |||
There was a problem hiding this comment.
After commenting about defaults, we should probably test the default, too.
|
Ping. |
| @@ -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)) { | |||
| @@ -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)) { | |||
|
Will riscv-non-isa/riscv-elf-psabi-doc#417 be implemented in a separate patch? |
Sure. |
This patch enable hardware shadow stack with
Zicifssandmno-forced-sw-shadow-stack. New feature forced-sw-shadow-stack disables hardware shadow stack even whenZicfissenabled.