Re: [PATCH v7 0/7] Introduce support for Guest CET feature

[Jim Mattson]

On Thu, Sep 26, 2019 at 7:17 PM Yang Weijiang <weijiang.yang@xxxxxxxxx> wrote:
>
> Control-flow Enforcement Technology (CET) provides protection against
> Return/Jump-Oriented Programming (ROP/JOP) attack. It includes two
> sub-features: Shadow Stack (SHSTK) and Indirect Branch Tracking (IBT).
>
> KVM modification is required to support Guest CET feature.
> This patch serial implemented CET related CPUID/XSAVES enumeration, MSRs
> and VMEntry configuration etc.so that Guest kernel can setup CET
> runtime infrastructure based on them. Some MSRs and related feature
> flags used in the patches reference the definitions in kernel patch.

I am still trying to make my way through the 358 page (!) spec for
this feature, but I already have some questions/comments about this
series:

1. Does CET "just work" with shadow paging? Shadow paging knows
nothing about "shadow-stack pages," and it's not clear to me how
shadow-stack pages will interact with dirty tracking.
2. I see non-trivial changes to task switch under CET. Does
emulator_do_task_switch need to be updated?
3. What about all of the emulator routines that emulate control
transfers (e.g. em_jmp_{far,abs}, em_call_(near_abs,far},
em_ret_{far,far_imm,near_imm}, etc)? Don't these have to be modified
to work correctly when CR4.CET is set?
4. You don't use the new "enable supervisor shadow stack control" bit
in the EPTP. I assume that this is entirely optional, right?
5. I think the easiest way to handle the nested issue (rather than
your explicit check for vmxon when setting CR4.CET when the vCPU is in
VMX operation) is just to leave CR4.CET out of IA32_VMX_CR4_FIXED1
(which is already the case).
6. The function, exception_class(), in x86.c, should be updated to
categorize #CP as contributory.
7. The function, x86_exception_has_error_code(), in x86.h, should be
updated to include #CP.
8. There appear to be multiple changes to SMM that you haven't
implemented (e.g saving/restoring the SSP registers in/from SMRAM.

CET is quite complex. Without any tests, I don't see how you can have
any confidence in the correctness of this patch series.