linux-kernel - Re: [PATCH v3 1/3] KVM: nVMX: Sync L2 guest CET states between L1/L2

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [thread-next>] [day] [month] [year] [list]

Message-ID: <YEv5IFrh/HBUsMR/@google.com>
Date:   Fri, 12 Mar 2021 15:28:32 -0800
From:   Sean Christopherson <seanjc@...gle.com>
To:     Yang Weijiang <weijiang.yang@...el.com>
Cc:     pbonzini@...hat.com, vkuznets@...hat.com, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org
Subject: Re: [PATCH v3 1/3] KVM: nVMX: Sync L2 guest CET states between L1/L2

On Mon, Mar 08, 2021, Yang Weijiang wrote:
> On Thu, Mar 04, 2021 at 08:46:45AM -0800, Sean Christopherson wrote:
> > On Thu, Mar 04, 2021, Yang Weijiang wrote:
> > > @@ -3375,6 +3391,12 @@ enum nvmx_vmentry_status nested_vmx_enter_non_root_mode(struct kvm_vcpu *vcpu,
> > >  	if (kvm_mpx_supported() &&
> > >  		!(vmcs12->vm_entry_controls & VM_ENTRY_LOAD_BNDCFGS))
> > >  		vmx->nested.vmcs01_guest_bndcfgs = vmcs_read64(GUEST_BNDCFGS);
> > > +	if (kvm_cet_supported() &&
> > > +		!(vmcs12->vm_entry_controls & VM_ENTRY_LOAD_CET_STATE)) {
> > 
> > Alignment.
> > 
> > > +		vmx->nested.vmcs01_guest_ssp = vmcs_readl(GUEST_SSP);
> > > +		vmx->nested.vmcs01_guest_s_cet = vmcs_readl(GUEST_S_CET);
> > > +		vmx->nested.vmcs01_guest_ssp_tbl = vmcs_readl(GUEST_INTR_SSP_TABLE);
> > > +	}
> > >  
> > >  	/*
> > >  	 * Overwrite vmcs01.GUEST_CR3 with L1's CR3 if EPT is disabled *and*
> > > @@ -4001,6 +4023,9 @@ static bool is_vmcs12_ext_field(unsigned long field)
> > >  	case GUEST_IDTR_BASE:
> > >  	case GUEST_PENDING_DBG_EXCEPTIONS:
> > >  	case GUEST_BNDCFGS:
> > > +	case GUEST_SSP:
> > > +	case GUEST_INTR_SSP_TABLE:
> > > +	case GUEST_S_CET:
> > >  		return true;
> > >  	default:
> > >  		break;
> > > @@ -4052,6 +4077,11 @@ static void sync_vmcs02_to_vmcs12_rare(struct kvm_vcpu *vcpu,
> > >  		vmcs_readl(GUEST_PENDING_DBG_EXCEPTIONS);
> > >  	if (kvm_mpx_supported())
> > >  		vmcs12->guest_bndcfgs = vmcs_read64(GUEST_BNDCFGS);
> > > +	if (kvm_cet_supported()) {
> > 
> > Isn't the existing kvm_mpx_supported() check wrong in the sense that KVM only
> > needs to sync to vmcs12 if KVM and the guest both support MPX?  
> 
> For MPX, if guest_cpuid_has() is not efficent, can it be checked by BNDCFGS EN bit?
> E.g.:
> 
> if (kvm_mpx_supported() && (vmcs12->guest_bndcfgs & 1))
> 
> > Same would apply to CET. Not sure it'd be a net positive in terms of performance since
> > guest_cpuid_has() can be quite slow, but overwriting vmcs12 fields that technically don't exist
> > feels wrong.
> 
> For CET, can we get equivalent effect by checking vmcs12->guest_cr4.CET?
> E.g.:
> if (kvm_cet_supported() && (vmcs12->guest_cr4 & X86_CR4_CET))

No, because the existence of the fields does not depend on them being enabled.
E.g. things will go sideways if the values change while L2 is running, L2
disables CET, and then an exit occurs.

This is already a slow path, maybe the guest_cpuid_has() checks are a non-issue.


> 
> > 
> > > +		vmcs12->guest_ssp = vmcs_readl(GUEST_SSP);
> > > +		vmcs12->guest_s_cet = vmcs_readl(GUEST_S_CET);
> > > +		vmcs12->guest_ssp_tbl = vmcs_readl(GUEST_INTR_SSP_TABLE);
> > > +	}
> > >  
> > >  	vmx->nested.need_sync_vmcs02_to_vmcs12_rare = false;
> > >  }
> > > diff --git a/arch/x86/kvm/vmx/vmx.h b/arch/x86/kvm/vmx/vmx.h
> > > index 9d3a557949ac..36dc4fdb0909 100644
> > > --- a/arch/x86/kvm/vmx/vmx.h
> > > +++ b/arch/x86/kvm/vmx/vmx.h
> > > @@ -155,6 +155,9 @@ struct nested_vmx {
> > >  	/* to migrate it to L2 if VM_ENTRY_LOAD_DEBUG_CONTROLS is off */
> > >  	u64 vmcs01_debugctl;
> > >  	u64 vmcs01_guest_bndcfgs;
> > > +	u64 vmcs01_guest_ssp;
> > > +	u64 vmcs01_guest_s_cet;
> > > +	u64 vmcs01_guest_ssp_tbl;
> > >  
> > >  	/* to migrate it to L1 if L2 writes to L1's CR8 directly */
> > >  	int l1_tpr_threshold;
> > > -- 
> > > 2.26.2
> > >