Re: [PATCH v3 6/8] KVM:VMX: Load Guest CET via VMCS when CET is enabled in Guest

[Sean Christopherson]

On Mon, Mar 04, 2019 at 05:56:40PM +0800, Yang Weijiang wrote:
> On Mon, Mar 04, 2019 at 10:43:07AM -0800, Sean Christopherson wrote:
> > On Sun, Mar 03, 2019 at 08:26:08PM +0800, Yang Weijiang wrote:
> > > On Fri, Mar 01, 2019 at 06:58:19AM -0800, Sean Christopherson wrote:
> > > > On Thu, Feb 28, 2019 at 04:38:44PM +0800, Yang Weijiang wrote:
> > > > > On Thu, Feb 28, 2019 at 08:17:15AM -0800, Sean Christopherson wrote:
> > > > > > On Mon, Feb 25, 2019 at 09:27:14PM +0800, Yang Weijiang wrote:
> > > > > > > "Load Guest CET state" bit controls whether guest CET states
> > > > > > > will be loaded at Guest entry. Before doing that, KVM needs
> > > > > > > to check if CPU CET feature is available.
> > > > > > > 
> > > > > > > Signed-off-by: Zhang Yi Z <yi.z.zhang@xxxxxxxxxxxxxxx>
> > > > > > > Signed-off-by: Yang Weijiang <weijiang.yang@xxxxxxxxx>
> > > > > > > ---
> > > > > > >  arch/x86/kvm/vmx.c | 32 ++++++++++++++++++++++++++++++++
> > > > > > >  1 file changed, 32 insertions(+)
> > > > > > > 
> > > > > > > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> > > > > > > index 89ee086e1729..d32cee9ee079 100644
> > > > > > > --- a/arch/x86/kvm/vmx.c
> > > > > > > +++ b/arch/x86/kvm/vmx.c
> > > > > > > @@ -55,6 +55,7 @@
> > > > > > >  #include <asm/mmu_context.h>
> > > > > > >  #include <asm/spec-ctrl.h>
> > > > > > >  #include <asm/mshyperv.h>
> > > > > > > +#include <asm/cet.h>
> > > > > > >  
> > > > > > >  #include "trace.h"
> > > > > > >  #include "pmu.h"
> > > > > > > @@ -4065,6 +4066,20 @@ static inline bool vmx_feature_control_msr_valid(struct kvm_vcpu *vcpu,
> > > > > > >  	return !(val & ~valid_bits);
> > > > > > >  }
> > > > > > >  
> > > > > > > +static int vmx_guest_cet_cap(struct kvm_vcpu *vcpu)
> > > > > > > +{
> > > > > > > +	u32 eax, ebx, ecx, edx;
> > > > > > > +
> > > > > > > +	/*
> > > > > > > +	 * Guest CET can work as long as HW supports the feature, independent
> > > > > > > +	 * to Host SW enabling status.
> > > > > > > +	 */
> > > > > > > +	cpuid_count(7, 0, &eax, &ebx, &ecx, &edx);
> > > > > > > +
> > > > > > > +	return ((ecx & bit(X86_FEATURE_SHSTK)) |
> > > > > > > +		(edx & bit(X86_FEATURE_IBT))) ? 1 : 0;
> > > > > > 
> > > > > > Given the holes in the (current) architecture/spec, I think KVM has to
> > > > > > require both features to be supported in the guest to allow CR4.CET to
> > > > > > be enabled.
> > > > > The reason why I use a "OR" here is to keep CET enabling control the
> > > > > same as that on host, right now on host, users can select to enable  SHSTK or IBT
> > > > > feature by disabling the unexpected one. It's free to select SHSTK & IBT
> > > > > or SHSTK | IBT.
> > > > 
> > > > Which is not the same as SHSTK != IBT in *hardware*, which is effectively
> > > > what this is allowing for the guest.  The problem is that the architecture
> > > > doesn't cleanly separate the two features, i.e. we'd have a virtualization
> > > > hole where the guest could touch state for a disabled feature.
> > > > 
> > > > Regardless, the guest would still be able to selectively enable each CET
> > > > feature, it would just never see a model where SHSTK != IBT.
> > > Hi, Sean,
> > > I'd like to understand your concerns. From my point of view, e.g., 
> > > when only IBT is enabled, PL3_SSP MSR would be unnecessrily exposed,
> > > this is the design "limitation", but PL3_SSP keeps 0 if SHSTK is not
> > > configured. Could you detail your concerns?
> > 
> > In your approach, IA32_{S,U}_CET can be written if SHSTK or IBT is exposed
> > to the guest.  If only SHSTK is exposed, a devious guest can still use IBT
> > because it can set CR4.CET as well as the enable bits in IA32_{S,U}_CET.
> > Preventing the guest from using IBT in this scenario is infeasible as it
> > would require trapping and emulating the XSAVE as well as the relevent CET
> > MSRs.
> Cannot agree with you more!
> This is some design limitation, but from my point of view, once vmm
> exposes CET capability to guest via CPUID, it grants the guest kernel freedom to choose
> which features to be enabled, we don't need to add extra constraints on
> the usage.

But if KVM allows SHSTK and IBT to be toggled independently then the VMM
has only exposed SHSTK or IBT, not CET as whole.

Even if SHSTK and IBT are bundled together the guest still has to opt-in
to enabling each feature.  I don't see what we gain by pretending that
SHSTK/IBT can be individually exposed to the guest, and on the flip side
doing so creates a virtualization hole.