Message-ID: <20190605023044.GB28360@local-michael-cet-test>
Date:   Wed, 5 Jun 2019 10:30:44 +0800
From:   Yang Weijiang <weijiang.yang@...el.com>
To:     Sean Christopherson <sean.j.christopherson@...el.com>
Cc:     Yang Weijiang <weijiang.yang@...el.com>, pbonzini@...hat.com,
        mst@...hat.com, rkrcmar@...hat.com, jmattson@...gle.com,
        linux-kernel@...r.kernel.org, kvm@...r.kernel.org,
        yu-cheng.yu@...el.com
Subject: Re: [PATCH v5 1/8] KVM: VMX: Define CET VMCS fields and control bits

On Tue, Jun 04, 2019 at 07:46:13AM -0700, Sean Christopherson wrote:
> On Wed, May 22, 2019 at 03:00:54PM +0800, Yang Weijiang wrote:
> > CET(Control-flow Enforcement Technology) is an upcoming Intel® processor
> > family feature that blocks return/jump-oriented programming (ROP) attacks.
> > It provides the following capabilities to defend
> > against ROP/JOP style control-flow subversion attacks:
> > 
> > - Shadow Stack (SHSTK):
> >   A second stack for the program that is used exclusively for
> >   control transfer operations.
> > 
> > - Indirect Branch Tracking (IBT):
> >   Free branch protection to defend against jump/call oriented
> >   programming.
> 
> What is "free" referring to here?  The software enabling certainly isn't
> free, and I doubt the hardware/ucode cost is completely free.
>
Thank you for pointing it out!
"free" comes from the spec., I guess the author means the major effort of
enabling IBT is in compiler and HW, free effort to SW enabling.
But as you mentioned, actually there's deficated effort to enable it,
will change it to other words.

> > Several new CET MSRs are defined in kernel to support CET:
> > MSR_IA32_{U,S}_CET - MSRs to control the CET settings for user
> > mode and suervisor mode respectively.
> > 
> > MSR_IA32_PL{0,1,2,3}_SSP - MSRs to store shadow stack pointers for
> > CPL-0,1,2,3 levels.
> > 
> > MSR_IA32_INT_SSP_TAB - MSR to store base address of shadow stack
> > pointer table.
> 
> For consistency (within the changelog), these should be list style, e.g.:
> 
> 
>   - MSR_IA32_{U,S}_CET: Control CET settings for user mode and suervisor
>                         mode respectively.
> 
>   - MSR_IA32_PL{0,1,2,3}_SSP: Store shadow stack pointers for CPL levels.
> 
>   - MSR_IA32_INT_SSP_TAB: Stores base address of shadow stack pointer
>                           table.
> 
OK, will change it in next version.
> > Two XSAVES state components are introduced for CET:
> > IA32_XSS:[bit 11] - bit for save/restor user mode CET states
> > IA32_XSS:[bit 12] - bit for save/restor supervisor mode CET states.
> 
> Likewise, use a consistent list format.
> 
> > 6 VMCS fields are introduced for CET, {HOST,GUEST}_S_CET is to store
> > CET settings in supervisor mode. {HOST,GUEST}_SSP is to store shadow
> > stack pointers in supervisor mode. {HOST,GUEST}_INTR_SSP_TABLE is to
> > store base address of shadow stack pointer table.
> 
> It'd probably be easier to use a list format for the fields, e.g.:
> 
> 6 VMCS fields are introduced for CET:
> 
>   - {HOST,GUEST}_S_CET: stores CET settings for supervisor mode.
> 
>   - {HOST,GUEST}_SSP: stores shadow stack pointers for supervisor mode.
> 
>   - {HOST,GUEST}_INTR_SSP_TABLE: stores the based address of the shadow
>                                  stack pointer table.
> 
OK, will modify it.
> > If VM_EXIT_LOAD_HOST_CET_STATE = 1, the host's CET MSRs are restored
> > from below VMCS fields at VM-Exit:
> > - HOST_S_CET
> > - HOST_SSP
> > - HOST_INTR_SSP_TABLE
> 
> Personal preference, I like indenting lists like this with a space or two
> so that the list is clearly delineated.
Good suggestion, thanks!
> 
> > If VM_ENTRY_LOAD_GUEST_CET_STATE = 1, the guest's CET MSRs are loaded
> > from below VMCS fields at VM-Entry:
> > - GUEST_S_CET
> > - GUEST_SSP
> > - GUEST_INTR_SSP_TABLE
> > 
> > Apart from VMCS auto-load fields, KVM calls kvm_load_guest_fpu() and
> > kvm_put_guest_fpu() to save/restore the guest CET MSR states at
> > VM exit/entry. XSAVES/XRSTORS are executed underneath these functions
> > if they are supported. The CET xsave area is consolidated with other
> > XSAVE components in thread_struct.fpu field.
> > 
> > When context switch happens during task switch/interrupt/exception etc.,
> > Kernel also relies on above functions to switch CET states properly.
> 
> These paragraphs about the FPU and KVM behavior don't belong in this
> patch.

OK. looks like it's redundant, will remve it.
>  
> > Signed-off-by: Yang Weijiang <weijiang.yang@...el.com>
> > Co-developed-by: Zhang Yi Z <yi.z.zhang@...ux.intel.com>
> 
> Co-developed-by needs to be accompanied by a SOB.  And your SOB should
> be last since you sent the patch.  This comment applies to all patches.
> 
> See "12) When to use Acked-by:, Cc:, and Co-developed-by:" in
> Documentation/process/submitting-patches.rst for details (I recommend
> looking at a v5.2-rc* version, a docs update was merged for v5.2).
Got it, will change all the signatures.

Powered by blists - more mailing lists