implement shadow stacks

[Freax13]

This PR implements shadow stacks.

Shadow stacks are enabled/disabled at compile time. AFAIK all processors supporting either AMD SEV-SNP or Intel TDX support shadow stacks, so no runtime checks are implemented. This also avoids the pitfall where the hypervisor lies about shadow stack availability to maliciously lower the security of the SVSM.

KVM intercepts accesses to the MSRs used for shadow stacks, so some host-side modifications are required to make this work.

I implemented a #CP exception handler to display diagnostic information when a shadow stack-related issue occurs. We might want to disable this exception handler for release builds and cause a triple fault instead. #CP exceptions are likely a sign of something fishy going on and we might want to terminate the guest just to be sure.

Currently, shadow stacks are disabled by default for a couple of reasons:

1. The KVM patches mentioned above are needed to make this work.
2. We haven't implemented proper user syscalls (using syscall & sysret) yet and I don't want to hinder any efforts in that direction by forcing a proper syscall implementation to support shadow stacks out of the box. We should probably get syscalls without shadow stacks working first and once that's merged, we can add shadow stack support later and eventually enable shadow stacks by default.
3. I haven't tested shadow-stacks together with enable-gdb yet, but I suspect this probably needs some additional work.

[joergroedel]

This is impressive, thanks for implementing this feature!

There are some open questions, one around the task-switch handling. I left a comment there. The other question is, whether there is a way to enable/disable this feature at boot-time, instead of compile time.

Finally, I found that it uses the old PageRef interface. Since the new one is merged now, this can be re-based on top of latest HEAD.

[Freax13]

There are some open questions, one around the task-switch handling. I left a comment there. The other question is, whether there is a way to enable/disable this feature at boot-time, instead of compile time.

I added a patch that tries to detect CET support at runtime by enabling CET in CR4 and catching any faults that might occur if not supported. This still doesn't technically detect shadow stack support because AFAICT CPUs could theoretically only support CET lBT (indirect branch tracking) and not CET SHSTK (shadow stacks), but in practice no such CPUs exist (and AFAICT TDX only allows turning all of CET on or off, not individual sub-parts).

Finally, I found that it uses the old PageRef interface. Since the new one is merged now, this can be re-based on top of latest HEAD.

Done.

[msft-jlange]

It doesn't look like this change is compatible with the existing #HV handling code. For example, there is code in asm_entry_hv which specifically checks to see whether a recursive #HV has been delivered while executing an IRET sequence, and if so, it overwrites the old IRET data with the newly pushed IRET data. The shadow stack equivalent of this code must be written, but it is also inherently unsafe, because it requires popping values from the shadow stack and using the WRSS instruction to write them to a previous location on the shadow stack. Regardless of what approach we ultimately decide to take here, it is critical to support Restricted Injection correctly, and we can't allow shadow stack support to break it.

[Freax13]

It doesn't look like this change is compatible with the existing #HV handling code. For example, there is code in asm_entry_hv which specifically checks to see whether a recursive #HV has been delivered while executing an IRET sequence, and if so, it overwrites the old IRET data with the newly pushed IRET data. The shadow stack equivalent of this code must be written, but it is also inherently unsafe, because it requires popping values from the shadow stack and using the WRSS instruction to write them to a previous location on the shadow stack. Regardless of what approach we ultimately decide to take here, it is critical to support Restricted Injection correctly, and we can't allow shadow stack support to break it.

Interesting, thanks for pointing that out, I'll take a look at that. Do you know if there are KVM patches implementing restricted injection that I can test with?

[msft-jlange]

Do you know if there are KVM patches implementing restricted injection that I can test with?

Unfortunately, there are none that I am aware of. However, I'm happy to review and test any changes you have in our environment which does support restricted injection. Let me know if you want a more thorough description of how the existing #HV handler works and interacts with the IRET flow.

[msft-jlange]

AFAIK all processors supporting either AMD SEV-SNP or Intel TDX support shadow stacks

That's not actually true. The Zen3 architecture supports SEV-SNP but not shadow stacks.

[Freax13]

AFAIK all processors supporting either AMD SEV-SNP or Intel TDX support shadow stacks

That's not actually true. The Zen3 architecture supports SEV-SNP but not shadow stacks.

EPYC Milan (based on Zen 3) supports shadow stacks. I used an EPYC Milan based machine to test these patches.

[msft-jlange]

EPYC Milan (based on Zen 3) supports shadow stacks. I used an EPYC Milan based machine to test these patches.

You are correct; I was misinformed.

[Freax13]

It doesn't look like this change is compatible with the existing #HV handling code. For example, there is code in asm_entry_hv which specifically checks to see whether a recursive #HV has been delivered while executing an IRET sequence, and if so, it overwrites the old IRET data with the newly pushed IRET data. The shadow stack equivalent of this code must be written, but it is also inherently unsafe, because it requires popping values from the shadow stack and using the WRSS instruction to write them to a previous location on the shadow stack. Regardless of what approach we ultimately decide to take here, it is critical to support Restricted Injection correctly, and we can't allow shadow stack support to break it.

I added a patch to adjust the shadow stack in the #HV.

Do you know if there are KVM patches implementing restricted injection that I can test with?

Unfortunately, there are none that I am aware of. However, I'm happy to review and test any changes you have in our environment which does support restricted injection. Let me know if you want a more thorough description of how the existing #HV handler works and interacts with the IRET flow.

Could you please test my patches on your environment? I tried testing by manually adding int $28 to some places, but I still don't have a proper test setup, so I might have missed something. I also came across some bugs/regressions unrelated to shadow stacks, so I opened a separate PR to fix those: #466. You might need those changes to properly test the changes in this PR.

[msft-jlange]

Could you please test my patches on your environment? I tried testing by manually adding int $28 to some places, but I still don't have a proper test setup, so I might have missed something. I also came across some bugs/regressions unrelated to shadow stacks, so I opened a separate PR to fix those: #466. You might need those changes to properly test the changes in this PR.

Other than the specific stack adjustment problem I commented on (the one that's missing), everything appears to work. The other two shadow stack adjustments are correct. And I found another problem with the #HV handler that's not addressed by your other PR so I'll submit a fix for that too. Thanks for prompting me to review this code again!

[ereshetova]

KVM intercepts accesses to the MSRs used for shadow stacks, so some host-side modifications are required to make this work.

Not very relevant yet, but this is not true for TDX case. The CET/shadow stack related MSRs are handled natively for the guest (given that the feature is enabled for the guest during its creation via config) with no host involvement (wont be secure otherwise). TDX module ensures this. So you should be able to enable this for TDX without any host/kvm support (but enabling TD_PARAMS.XFAM[11] || TD_PARAMS.XFAM[12] - matching XSAVE[11],XSAVE[12] for a given guest).

[msft-jlange]

There's still one issue. You changed the size of the exception frame, and you've adjusted most of the places that refer to the stack frame accordingly. However, the reference at line 151 was never updated, and it is now inaccurate. So this sequence:

        testl   $0x200, 18*8(%rcx)
        jz      postpone_hv

needs to become

        testl   $0x200, 20*8(%rcx)
        jz      postpone_hv

If I make that change locally, then everything works as expected (or would work, if all of the other #HV-related PRs were included).

[Freax13]

There's still one issue. You changed the size of the exception frame, and you've adjusted most of the places that refer to the stack frame accordingly. However, the reference at line 151 was never updated, and it is now inaccurate. So this sequence:

        testl   $0x200, 18*8(%rcx)
        jz      postpone_hv

needs to become

        testl   $0x200, 20*8(%rcx)
        jz      postpone_hv

If I make that change locally, then everything works as expected (or would work, if all of the other #HV-related PRs were included).

Good catch, should be fixed now! Thank you very much for your review!

[Freax13]

@joergroedel I think this PR is ready.

[msft-jlange]

I just found one additional serious problem.

There are functions that generate page table flags to represent different page permissions. However, some of these functions will make the PTE dirty without also marking it writable, and these functions will effectively generate shadow stack permissions, which is not correct for those permission types. Specifically, all methods of PTEntryFlags other than PTEntryFlags::data() and PTEntryFlags::task_data() need to remove the DIRTY bit (anything that does not set WRITABLE should not set DIRTY). These must be fixed or else the shadow stack logic will not be secure.

[Freax13]

I just found one additional serious problem.

There are functions that generate page table flags to represent different page permissions. However, some of these functions will make the PTE dirty without also marking it writable, and these functions will effectively generate shadow stack permissions, which is not correct for those permission types. Specifically, all methods of PTEntryFlags other than PTEntryFlags::data() and PTEntryFlags::task_data() need to remove the DIRTY bit (anything that does not set WRITABLE should not set DIRTY). These must be fixed or else the shadow stack logic will not be secure.

Good catch! Should be fixed now.

[joergroedel]

LGTM.