Re: [PATCH v7 0/7] Introduce support for Guest CET feature
On Thu, Oct 03, 2019 at 09:33:45AM -0700, Jim Mattson wrote:
> On Thu, Oct 3, 2019 at 5:59 AM Yang Weijiang wrote:
> >
> > On Wed, Oct 02, 2019 at 03:40:20PM -0700, Jim Mattson wrote:
> > > On Thu, Sep 26, 2019 at 7:17 PM Yang Weijiang
> > > 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".
>
> What I should have said is that I'd like to see kvm-unit-tests to
> exercise the new functionality, even if no one outside Intel can run
> them yet.
>
OK, let me figure out how to test it either in unit-test or selftest.
Thank you!
> > 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!
Re: [PATCH v7 0/7] Introduce support for Guest CET feature
On Thu, Oct 3, 2019 at 5:59 AM Yang Weijiang wrote:
>
> On Wed, Oct 02, 2019 at 03:40:20PM -0700, Jim Mattson wrote:
> > On Thu, Sep 26, 2019 at 7:17 PM Yang Weijiang
> > 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".
What I should have said is that I'd like to see kvm-unit-tests to
exercise the new functionality, even if no one outside Intel can run
them yet.
> 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!
Re: [PATCH v7 0/7] Introduce support for Guest CET feature
On Wed, Oct 02, 2019 at 03:40:20PM -0700, Jim Mattson wrote:
> On Thu, Sep 26, 2019 at 7:17 PM Yang Weijiang 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!
Re: [PATCH v7 0/7] Introduce support for Guest CET feature
On Thu, Sep 26, 2019 at 7:17 PM Yang Weijiang 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.
[PATCH v7 0/7] Introduce support for Guest CET feature
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. CET kernel patches is here: https://lkml.org/lkml/2019/8/13/1110 https://lkml.org/lkml/2019/8/13/1109 v6 -> v7: - Rebased patch to kernel v5.3 - Sean suggested to change CPUID(0xd, n) enumeration code as alined with existing one, and I think it's better to make the fix as an independent patch since XSS MSR are being used widely on X86 platforms. - Check more host and guest status before configure guest CET per Sean's feedback. - Add error-check before guest accesses CET MSRs per Sean's feedback. - Other minor fixes suggested by Sean. v5 -> v6: - Rebase patch to kernel v5.2. - Move CPUID(0xD, n>=1) helper to a seperate patch. - Merge xsave size fix with other patch. - Other minor fixes per community feedback. v4 -> v5: - Rebase patch to kernel v5.1. - Wrap CPUID(0xD, n>=1) code to a helper function. - Pass through MSR_IA32_PL1_SSP and MSR_IA32_PL2_SSP to Guest. - Add Co-developed-by expression in patch description. - Refine patch description. v3 -> v4: - Add Sean's patch for loading Guest fpu state before access XSAVES managed CET MSRs. - Melt down CET bits setting into CPUID configuration patch. - Add VMX interface to query Host XSS. - Check Host and Guest XSS support bits before set Guest XSS. - Make Guest SHSTK and IBT feature enabling independent. - Do not report CET support to Guest when Host CET feature is Disabled. v2 -> v3: - Modified patches to make Guest CET independent to Host enabling. - Added patch 8 to add user space access for Guest CET MSR access. - Modified code comments and patch description to reflect changes. v1 -> v2: - Re-ordered patch sequence, combined one patch. - Added more description for CET related VMCS fields. - Added Host CET capability check while enabling Guest CET loading bit. - Added Host CET capability check while reporting Guest CPUID(EAX=7, EXC=0). - Modified code in reporting Guest CPUID(EAX=D,ECX>=1), make it clearer. - Added Host and Guest XSS mask check while setting bits for Guest XSS. PATCH 1: Fix CPUID(0xD, n) enumeration to support XSS MSR. PATCH 2: Define CET VMCS fields and bits. PATCH 3: Pass through CET MSRs to Guest. PATCH 4: Load Guest CET states when CET is enabled. PATCH 5: Add CET CR4 bit and CET xsaves support in XSS MSR. PATCH 6: Load Guest FPU states for XSAVES managed MSRs. PATCH 7: Add user-space CET MSR access interface. Sean Christopherson (1): KVM: x86: Load Guest fpu state when accessing MSRs managed by XSAVES Yang Weijiang (6): KVM: CPUID: Fix IA32_XSS support in CPUID(0xd,i) enumeration KVM: VMX: Define CET VMCS fields and CPUID flags KVM: VMX: Pass through CET related MSRs to Guest KVM: VMX: Load Guest CET via VMCS when CET is enabled in Guest KVM: X86: Add CET CR4 bit and XSS support KVM: X86: Add user-space access interface for CET MSRs arch/x86/include/asm/kvm_host.h | 5 +- arch/x86/include/asm/vmx.h | 8 ++ arch/x86/kvm/cpuid.c| 110 ++--- arch/x86/kvm/cpuid.h| 2 + arch/x86/kvm/svm.c | 7 ++ arch/x86/kvm/vmx/vmx.c | 169 +++- arch/x86/kvm/x86.c | 22 - arch/x86/kvm/x86.h | 8 ++ 8 files changed, 287 insertions(+), 44 deletions(-) -- 2.17.2
