linux-kernel - RE: [PATCH v5 12/21] kvm: x86: Intercept #NM for saving IA32_XFD

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: <BN9PR11MB52760C5B8289B2DEF32AC63C8C4C9@BN9PR11MB5276.namprd11.prod.outlook.com>
Date:   Thu, 6 Jan 2022 00:55:40 +0000
From:   "Tian, Kevin" <kevin.tian@...el.com>
To:     "Christopherson,, Sean" <seanjc@...gle.com>,
        "Zhong, Yang" <yang.zhong@...el.com>
CC:     "x86@...nel.org" <x86@...nel.org>,
        "kvm@...r.kernel.org" <kvm@...r.kernel.org>,
        "linux-kernel@...r.kernel.org" <linux-kernel@...r.kernel.org>,
        "linux-doc@...r.kernel.org" <linux-doc@...r.kernel.org>,
        "linux-kselftest@...r.kernel.org" <linux-kselftest@...r.kernel.org>,
        "tglx@...utronix.de" <tglx@...utronix.de>,
        "mingo@...hat.com" <mingo@...hat.com>,
        "bp@...en8.de" <bp@...en8.de>,
        "dave.hansen@...ux.intel.com" <dave.hansen@...ux.intel.com>,
        "pbonzini@...hat.com" <pbonzini@...hat.com>,
        "corbet@....net" <corbet@....net>,
        "shuah@...nel.org" <shuah@...nel.org>,
        "Nakajima, Jun" <jun.nakajima@...el.com>,
        "jing2.liu@...ux.intel.com" <jing2.liu@...ux.intel.com>,
        "Liu, Jing2" <jing2.liu@...el.com>,
        "Zeng, Guang" <guang.zeng@...el.com>,
        "Wang, Wei W" <wei.w.wang@...el.com>
Subject: RE: [PATCH v5 12/21] kvm: x86: Intercept #NM for saving IA32_XFD_ERR

> From: Sean Christopherson <seanjc@...gle.com>
> Sent: Thursday, January 6, 2022 6:22 AM
> 
> On Wed, Jan 05, 2022, Yang Zhong wrote:
> > @@ -6399,6 +6424,26 @@ static void handle_interrupt_nmi_irqoff(struct
> kvm_vcpu *vcpu,
> >  	kvm_after_interrupt(vcpu);
> >  }
> >
> > +static void handle_nm_fault_irqoff(struct kvm_vcpu *vcpu)
> > +{
> > +	/*
> > +	 * Save xfd_err to guest_fpu before interrupt is enabled, so the
> > +	 * MSR value is not clobbered by the host activity before the guest
> > +	 * has chance to consume it.
> > +	 *
> > +	 * We should not blindly read xfd_err here, since this exception
> 
> Nit, avoid "we", and explain what KVM does (or doesn't) do, not what KVM
> "should"
> do, e.g. just say
> 
> 	 * Do not blindly read ...
> 
> > +	 * might be caused by L1 interception on a platform which doesn't
> > +	 * support xfd at all.
> > +	 *
> > +	 * Do it conditionally upon guest_fpu::xfd. xfd_err matters
> > +	 * only when xfd contains a non-zero value.
> > +	 *
> > +	 * Queuing exception is done in vmx_handle_exit. See comment
> there.
> 
> Another nit, it's worth explaining why XFD_ERR needs to be read here
> regardless
> of is_guest_mode().  E.g.
> 
> 	 * Injecting the #NM back into the guest is handled in the standard
> path
> 	 * as an #NM in L2 may be reflected into L1 as a VM-Exit.  Read
> XFD_ERR
> 	 * even if the #NM is from L2, as L1 may have exposed XFD to L2.

sounds good

> 
> Side topic, in a follow up series/patch, it's probably worth adding support in
> nested_vmx_prepare_msr_bitmap() to allow passthrough of the MSRs to L2.

will do.

> 
> > +	 */
> > +	if (vcpu->arch.guest_fpu.fpstate->xfd)
> > +		rdmsrl(MSR_IA32_XFD_ERR, vcpu->arch.guest_fpu.xfd_err);
> > +}
> > +
> >  static void handle_exception_nmi_irqoff(struct vcpu_vmx *vmx)
> >  {
> >  	const unsigned long nmi_entry = (unsigned long)asm_exc_nmi_noist;
> > @@ -6407,6 +6452,9 @@ static void handle_exception_nmi_irqoff(struct
> vcpu_vmx *vmx)
> >  	/* if exit due to PF check for async PF */
> >  	if (is_page_fault(intr_info))
> >  		vmx->vcpu.arch.apf.host_apf_flags =
> kvm_read_and_reset_apf_flags();
> > +	/* if exit due to NM, handle before interrupts are enabled */
> 
> Nit, drop this comment, it's slightly misleading since the #NM isn't fully
> handled
> here.  The comment in handle_nm_fault_irqoff() is more than sufficient.
> 
> > +	else if (is_nm_fault(intr_info))
> > +		handle_nm_fault_irqoff(&vmx->vcpu);
> >  	/* Handle machine checks before interrupts are enabled */
> >  	else if (is_machine_check(intr_info))
> >  		kvm_machine_check();
> > diff --git a/arch/x86/kvm/x86.c b/arch/x86/kvm/x86.c
> > index 21ce65220e38..2c988f8ca616 100644
> > --- a/arch/x86/kvm/x86.c
> > +++ b/arch/x86/kvm/x86.c
> > @@ -9953,6 +9953,9 @@ static int vcpu_enter_guest(struct kvm_vcpu
> *vcpu)
> >  	if (test_thread_flag(TIF_NEED_FPU_LOAD))
> >  		switch_fpu_return();
> >
> > +	if (vcpu->arch.guest_fpu.xfd_err)
> > +		wrmsrl(MSR_IA32_XFD_ERR, vcpu->arch.guest_fpu.xfd_err);
> > +
> >  	if (unlikely(vcpu->arch.switch_db_regs)) {
> >  		set_debugreg(0, 7);
> >  		set_debugreg(vcpu->arch.eff_db[0], 0);
> > @@ -10016,6 +10019,9 @@ static int vcpu_enter_guest(struct kvm_vcpu
> *vcpu)
> >
> >  	static_call(kvm_x86_handle_exit_irqoff)(vcpu);
> >
> > +	if (vcpu->arch.guest_fpu.xfd_err)
> > +		wrmsrl(MSR_IA32_XFD_ERR, 0);
> > +
> >  	/*
> >  	 * Consume any pending interrupts, including the possible source of
> >  	 * VM-Exit on SVM and any ticks that occur between VM-Exit and
> now.