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

[Yang Weijiang]

On Wed, Oct 02, 2019 at 03:40:20PM -0700, Jim Mattson wrote:
> 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.
Thanks Jim for the detailed comments. 

I missed adding test platform and
result introduction in cover letter. This serial of patch has passed CET
test in guest on Intel x86 emulator platform and develop machine. 
Some feature mentioned in the spec. has not been implemented yet. e.g., 
"supervisor shadow stack control". 

CET feature itself is complex, most of the enabling work is
inside kernel, the role of KVM is to expose CET related CPUID and MSRs
etc. to guest, and make guest take over control of the MSRs directly so that
CET can work efficiently for guest. There're QEMU patches for CET too.

I'll review your comments carefully, thank you again!