Skip to content
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

Support Zicfiss (shadow stack access) with CFI extension v0.4.0 #1560

Merged
merged 1 commit into from
Apr 29, 2024

Conversation

SuHo-llrr
Copy link
Contributor

@SuHo-llrr SuHo-llrr commented Jan 9, 2024

We implement the following task for support Zicfiss and reference from document cfi v0.4.0

  1. Add EXT_ZICFISS for enable Zicfiss with zicfiss extension name
  2. Add new software exception with tval 3 for shadow stack
  3. Implement sspush_x1/sspush_x5/sspopchk_x1/sspopchk_x5/ssrdp/ssamoswap_w/ssamoswap_d
  4. Implement c_sspush_x1/c_sspopchk_x5 in c_lui.h which has same encoding
  5. Add new special access type shstk in xlate_flags_t for checking special read/write permission in SS(Shadow Stack) Page.
  6. Add new shstk_load/shstk_store to enable shstk flag
  7. Allow special PTE (R=0, W=1, X=0) of SS page when LOAD/STORE with shstk type
  8. Set menvcfg.SSE and senvcfg.SSE bit on when extension enable

The Implement above requires the combination of the two patches below

  1. riscv-pk parsing .note.gnu.property [riscv-pk] (Enable shadow stack from .note.gnu.property riscv-pk#310)
  2. Compiler should emit .note.gnu.property section llvm-project

Some tasks require discussion:

  • Should we need the new PROT_SHSTK protection flag? (or use PROT_WRITE ?)

@aswaterman
Copy link
Collaborator

aswaterman commented Jan 9, 2024

Sorry for the misfire; my previous (deleted) comment was directed at pk.

@ved-rivos please review.

riscv/insns/c_lui.h Outdated Show resolved Hide resolved
riscv/insns/c_lui.h Outdated Show resolved Hide resolved
riscv/insns/c_lui.h Outdated Show resolved Hide resolved
riscv/insns/c_lui.h Outdated Show resolved Hide resolved
throw trap_illegal_instruction(insn.bits());
else if (prev_prv == PRV_U && STATE.v && !get_field(STATE.senvcfg->read(), SENVCFG_SSE))
throw trap_illegal_instruction(insn.bits());

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

See comments in c_sspopchk_x5.h

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think I can implement the following code.
After checking xSSE, we can do mop/cmop or Zicfiss

if (prv == PRV_M)
  xSSE = 0;
else if (prv == PRV_S || prv == PRV_HS)
  xSSE = get_field(STATE.menvcfg->read(), MENVCFG_SSE);
else if (p->extension_enabled('S'))
  xSSE = get_field(STATE.senvcfg->read(), SENVCFG_SSE);
else
  xSSE = false;

// For VS-mode
if (prv == PRV_S && STATE.v)
  xSSE &= get_field(STATE.henvcfg->read(), HENVCFG_SSE);

if (!xSSE) {
  #include "c_mop_N.h"
} else {
  do somthing
}

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

For instructions other than ssrdp the following suffices

if (xSSE) {
  do somthing
}

For ssrdp, we would include the mop_N.h as you suggested

riscv/insns/c_sspush_x1.h Outdated Show resolved Hide resolved
riscv/insns/ssamoswap_d.h Outdated Show resolved Hide resolved
riscv/insns/ssamoswap_d.h Outdated Show resolved Hide resolved
riscv/insns/ssamoswap_d.h Outdated Show resolved Hide resolved
else if (prev_prv == PRV_U && STATE.v && !get_field(STATE.senvcfg->read(), SENVCFG_SSE))
throw trap_illegal_instruction(insn.bits());
else
WRITE_RD(MMU.amo<uint64_t>(RS1, [&](uint64_t UNUSED lhs) { return RS2; }));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

How does this code indicate to MMU that the amo should be performed using shadow stack load and stores?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry, I missed implementing SS AMO. like following and we can use ss_load/ss_store
WRITE_RD(MMU.ss_amo<uint64_t>(RS1, [&](uint64_t UNUSED lhs) { return RS2; }));

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

it may suffice to be just since there is only one amo operation - so we could call this ssamoswap

riscv/insns/sspopchk_x1.h Outdated Show resolved Hide resolved
riscv/insns/sspopchk_x5.h Outdated Show resolved Hide resolved
riscv/insns/sspush_x1.h Outdated Show resolved Hide resolved
riscv/insns/sspush_x5.h Outdated Show resolved Hide resolved
require_extension(EXT_ZICFISS);

reg_t ssp = STATE.ssp->read();

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Need to qualify by xSSE. When xSSE is 0, the instruction reverts to being a zimop.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add check implementation

riscv/mmu.cc Outdated Show resolved Hide resolved
riscv/mmu.h Outdated Show resolved Hide resolved
riscv/mmu.cc Outdated
@@ -531,18 +531,26 @@ reg_t mmu_t::walk(mem_access_info_t access_info)
base = ppn << PGSHIFT;
} else if ((pte & PTE_U) ? s_mode && (type == FETCH || !sum) : !s_mode) {
break;
} else if (!(pte & PTE_V) || (!(pte & PTE_R) && (pte & PTE_W))) {
} else if (!(pte & PTE_V) || (!(pte & PTE_R) && (pte & PTE_W) && !access_info.flags.shstk)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Various aspects are missing:
All of below should be based on effective privilege of the memory access and not current privilege.
2. If virtual memory system is Bare then ss load and store cause SAMO access-fault
3. SS load and store cause SAMO access fault on access to RW, RWX, X, RX pages
4. SS load and store cause SAMO page fault on access to RO, invalid, and malformed PTEs
5. Non-SS store to SS pages cause SAMO access fault
6. Instruction fetches to SS pages cause instruction access fault

SS encodings in PTEs stay reserved if SS extension is not active.

Copy link
Contributor Author

@SuHo-llrr SuHo-llrr Jan 10, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for explaining the rule in such detail.
The rules for checking the PTE rule are a bit confusing, and I want to separate each action independently.

Directly check at the end whether the SS page is VW permission. like following code.

else if (check_ss && ((pte & vw) != vw)) {
      // ss load/store page with R=0, W=1, X=0 and valid
      break;

After this, check the PTE permission of the SS page for invalidity.

  if (check_ss) {
    if (!(invalid_pte & PTE_V))
      throw trap_store_page_fault(virt, addr, 0, 0);
    else if (invalid_pte & PTE_W || invalid_pte & PTE_X)
      throw trap_store_access_fault(virt, addr, 0, 0);
    else
      throw trap_store_page_fault(virt, addr, 0, 0);
  }

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think we could do this:

+  bool ss_access = access_info.ss_access;
+
+  if (ss_access) {
+    if (vm.levels == 0)
+        throw trap_store_access_fault(virt, addr, 0, 0);
+    type = STORE;
+  }
+
   if (vm.levels == 0)
     return s2xlate(addr, addr & ((reg_t(2) << (proc->xlen-1))-1), type, type, virt, hlvx, false) & ~page_mask; // zero-extend from xlen
 
@@ -516,6 +524,7 @@ reg_t mmu_t::walk(mem_access_info_t access_info)
     reg_t ppn = (pte & ~reg_t(PTE_ATTR)) >> PTE_PPN_SHIFT;
     bool pbmte = virt ? (proc->get_state()->henvcfg->read() & HENVCFG_PBMTE) : (proc->get_state()->menvcfg->read() & MENVCFG_PBMTE);
     bool hade = virt ? (proc->get_state()->henvcfg->read() & HENVCFG_ADUE) : (proc->get_state()->menvcfg->read() & MENVCFG_ADUE);
+    bool sse = virt ? (proc->get_state()->henvcfg->read() & HENVCFG_SSE) : (proc->get_state()->menvcfg->read() & MENVCFG_SSE);
 
     if (pte & PTE_RSVD) {
       break;
@@ -531,11 +540,21 @@ reg_t mmu_t::walk(mem_access_info_t access_info)
       base = ppn << PGSHIFT;
     } else if ((pte & PTE_U) ? s_mode && (type == FETCH || !sum) : !s_mode) {
       break;
-    } else if (!(pte & PTE_V) || (!(pte & PTE_R) && (pte & PTE_W))) {
+    } else if (!(pte & PTE_V) ||
+              (!(pte & PTE_R) && (pte & PTE_W) && ((!sse && !(pte & PTE_X)) || (pte & PTE_X)))) {
       break;
+    } else if ((!(pte & PTE_R) && (pte & PTE_W) && !(pte & PTE_X)) && (type == STORE && !ss_access)) {
+      // non-shadow-stack stores to shadow stack pages cause store access-fault
+      throw trap_store_access_fault(virt, addr, 0, 0);
+    } else if ((!(pte & PTE_R) && (pte & PTE_W) && !(pte & PTE_X)) && type == FETCH) {
+      // fetch from shadow stack pages cause instruction access-fault
+      throw trap_instruction_access_fault(virt, addr, 0, 0);
+    } else if ((((pte & PTE_R) && (pte & PTE_W)) || (pte & PTE_X)) && ss_access) {
+      // shadow stack access cause store access fault if xwr!=010 and xwr!=001
+      throw trap_store_access_fault(virt, addr, 0, 0);
     } else if (type == FETCH || hlvx ? !(pte & PTE_X) :
                type == LOAD          ? !(pte & PTE_R) && !(mxr && (pte & PTE_X)) :
-                                       !((pte & PTE_R) && (pte & PTE_W))) {
+ 

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should also be added to pmp_ok:

if (ss_access)
   type = STORE;

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for your narrow down rule.
I add some more detailed descriptions and remove the STORE check in the end

else if (type == FETCH || hlvx ? !(pte & PTE_X) :
               type == LOAD && !(pte & PTE_R) && !(mxr && (pte & PTE_X))) {

@SuHo-llrr SuHo-llrr force-pushed the cfi-ext branch 2 times, most recently from 35126a4 to 398c0a9 Compare January 10, 2024 11:43
riscv/mmu.h Outdated Show resolved Hide resolved
Copy link
Collaborator

@scottj97 scottj97 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I have not done a thorough review but I did notice one issue.

riscv/processor.cc Outdated Show resolved Hide resolved
@ved-rivos
Copy link
Contributor

ved-rivos commented Mar 21, 2024

@SuHo-llrr - would it be possible to update this PR to use designated initializers?
may also need a rebase to address conflicts.

@SuHo-llrr SuHo-llrr force-pushed the cfi-ext branch 2 times, most recently from e296569 to 85c1e2a Compare March 22, 2024 06:10
@SuHo-llrr
Copy link
Contributor Author

@SuHo-llrr - would it be possible to update this PR to use designated initializers? may also need a rebase to address conflicts.
@ved-rivos
I finished using designated initializers and rebased the code.

@SuHo-llrr
Copy link
Contributor Author

@scottj97 I already changed the code for your request. Thanks

@SuHo-llrr
Copy link
Contributor Author

@ved-rivos @scottj97
I already rebased this patch. Do you have any other suggestions?

@SuHo-llrr
Copy link
Contributor Author

@aswaterman
It's been a few weeks since this patch was updated, do you have any suggestions? Thank you very much!

@aswaterman
Copy link
Collaborator

I won't step in to override the other reviewers. If @ved-rivos thinks it is done, I can take a look next week, giving others a chance to perform their review.

@ved-rivos
Copy link
Contributor

@aswaterman - I have reviewed and tested this (minus the last update to use designated initializers). This looks good to me.

riscv/processor.cc Outdated Show resolved Hide resolved
Copy link
Collaborator

@aswaterman aswaterman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM once my comments are addressed. Thanks for your patience, @SuHo-llrr!

riscv/processor.cc Outdated Show resolved Hide resolved
Copy link
Collaborator

@aswaterman aswaterman left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM after my last nit is addressed

1. Add EXT_ZICFISS for enable Zicfiss with zicfiss extension name.
2. Add new software exception with tval 3 for shadow stack.
3. Implement sspush_x1/sspush_x5/sspopchk_x1/sspopchk_x5/ssrdp/ssamoswap_w/ssamoswap_d.
4. Implement c_sspush_x1/c_sspopchk_x5 in c_lui.h which has same encoding.
5. Add new special access type ss_access in xlate_flags_t for checking special read/write permission in SS(Shadow Stack) page.
6. Add new ss_load/ss_store/ssamoswap to enable ss_access flag.
7. Check special pte(xwr=010) of SS page.
@SuHo-llrr
Copy link
Contributor Author

Hi @aswaterman,
excuse me, do you know how to proceed with this PR? it’s block with a changed request, but I couldn't get approved.

@aswaterman aswaterman dismissed scottj97’s stale review April 29, 2024 00:35

Feedback has been addressed

@aswaterman aswaterman merged commit b3bcc12 into riscv-software-src:master Apr 29, 2024
3 checks passed
@aswaterman
Copy link
Collaborator

@SuHo-llrr I just noticed that you manually modified encoding.h. In the future, please do not do that: it is an auto-generated file (whose source is https://github.com/riscv/riscv-opcodes). No need to take any immediate action, though; I will fix it up.

@SuHo-llrr
Copy link
Contributor Author

@aswaterman
Thank you for correcting my mistake of forgetting to update the riscv-opcodes.

@aswaterman
Copy link
Collaborator

No problem at all: we should have caught this during review. And no harm was done. Thank you!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants