linux-kernel - Re: [PATCH 1/2] KVM: x86: Add emulation support for #GP triggered by VM instructions

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: <X/3fbaO1ZarMdjft@google.com>
Date:   Tue, 12 Jan 2021 09:42:05 -0800
From:   Sean Christopherson <seanjc@...gle.com>
To:     Paolo Bonzini <pbonzini@...hat.com>
Cc:     Wei Huang <wei.huang2@....com>, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org, vkuznets@...hat.com, joro@...tes.org,
        bp@...en8.de, tglx@...utronix.de, mingo@...hat.com, x86@...nel.org,
        jmattson@...gle.com, wanpengli@...cent.com, bsd@...hat.com,
        dgilbert@...hat.com, mlevitsk@...hat.com
Subject: Re: [PATCH 1/2] KVM: x86: Add emulation support for #GP triggered by
 VM instructions

On Tue, Jan 12, 2021, Paolo Bonzini wrote:
> On 12/01/21 07:37, Wei Huang wrote:
> >   static int gp_interception(struct vcpu_svm *svm)
> >   {
> >   	struct kvm_vcpu *vcpu = &svm->vcpu;
> >   	u32 error_code = svm->vmcb->control.exit_info_1;
> > -
> > -	WARN_ON_ONCE(!enable_vmware_backdoor);
> > +	int rc;
> >   	/*
> > -	 * VMware backdoor emulation on #GP interception only handles IN{S},
> > -	 * OUT{S}, and RDPMC, none of which generate a non-zero error code.
> > +	 * Only VMware backdoor and SVM VME errata are handled. Neither of
> > +	 * them has non-zero error codes.
> >   	 */
> >   	if (error_code) {
> >   		kvm_queue_exception_e(vcpu, GP_VECTOR, error_code);
> >   		return 1;
> >   	}
> > -	return kvm_emulate_instruction(vcpu, EMULTYPE_VMWARE_GP);
> > +
> > +	rc = kvm_emulate_instruction(vcpu, EMULTYPE_PARAVIRT_GP);
> > +	if (rc > 1)
> > +		rc = svm_emulate_vm_instr(vcpu, rc);
> > +	return rc;
> >   }
> 
> Passing back the third byte is quick hacky.  Instead of this change to
> kvm_emulate_instruction, I'd rather check the instruction bytes in
> gp_interception before calling kvm_emulate_instruction.

Agreed.  And I'd also prefer that any pure refactoring is done in separate
patch(es) so that the actual functional change is better isolated.

On a related topic, it feels like nested should be disabled by default on SVM
until it's truly ready for primetime, with the patch tagged for stable.  That
way we don't have to worry about crafting non-trivial fixes (like this one) to
make them backport-friendly.