From mboxrd@z Thu Jan 1 00:00:00 1970 From: Marcelo Tosatti Subject: Re: [PATCH 2/7] KVM: VMX: relax check for CS register in rmode_segment_valid() Date: Sat, 22 Dec 2012 12:55:43 -0200 Message-ID: <20121222145543.GA538@amt.cnet> References: <1355332255-26612-1-git-send-email-gleb@redhat.com> <1355332255-26612-3-git-send-email-gleb@redhat.com> <20121221231715.GA30710@amt.cnet> <20121222070241.GT29007@redhat.com> Mime-Version: 1.0 Content-Type: text/plain; charset=us-ascii Cc: kvm@vger.kernel.org To: Gleb Natapov Return-path: Received: from mx1.redhat.com ([209.132.183.28]:30332 "EHLO mx1.redhat.com" rhost-flags-OK-OK-OK-OK) by vger.kernel.org with ESMTP id S1751144Ab2LVO67 (ORCPT ); Sat, 22 Dec 2012 09:58:59 -0500 Received: from int-mx12.intmail.prod.int.phx2.redhat.com (int-mx12.intmail.prod.int.phx2.redhat.com [10.5.11.25]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id qBMEwvlM029692 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-SHA bits=256 verify=OK) for ; Sat, 22 Dec 2012 09:58:57 -0500 Content-Disposition: inline In-Reply-To: <20121222070241.GT29007@redhat.com> Sender: kvm-owner@vger.kernel.org List-ID: On Sat, Dec 22, 2012 at 09:02:41AM +0200, Gleb Natapov wrote: > On Fri, Dec 21, 2012 at 09:17:16PM -0200, Marcelo Tosatti wrote: > > On Wed, Dec 12, 2012 at 07:10:50PM +0200, Gleb Natapov wrote: > > > rmode_segment_valid() checks if segment descriptor can be used to enter > > > vm86 mode. VMX spec mandates that in vm86 mode CS register will be of > > > type data, not code. Lets allow guest entry with vm86 mode if the only > > > problem with CS register is incorrect type. Otherwise entire real mode > > > will be emulated. > > > > > > Signed-off-by: Gleb Natapov > > > --- > > > arch/x86/kvm/vmx.c | 2 ++ > > > 1 file changed, 2 insertions(+) > > > > > > diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c > > > index 4df3991..acbe86f 100644 > > > --- a/arch/x86/kvm/vmx.c > > > +++ b/arch/x86/kvm/vmx.c > > > @@ -3383,6 +3383,8 @@ static bool rmode_segment_valid(struct kvm_vcpu *vcpu, int seg) > > > var.dpl = 0x3; > > > var.g = 0; > > > var.db = 0; > > > + if (seg == VCPU_SREG_CS) > > > + var.type = 0x3; > > > ar = vmx_segment_access_rights(&var); > > > > > > if (var.base != (var.selector << 4)) > > > -- > > > 1.7.10.4 > > > > But with emulate_invalid_guest_state=1, segments are not fixed on > > transition to real mode. So this patch can result in > > invalid guest state entry failure. > > > At the point of this patch emulate_invalid_guest_state=1 is broken and > sate is fixed unconditionally during the call to vmx_set_segment(). Err, i meant: "But with emulate_invalid_guest_state=1, CS segment is not fixed on transition to real mode. So this patch can result in invalid guest state entry failure." Which is true, see enter_rmode. So unless i am missing something, the patch opens the possibility for an invalid vm-entry which was covered before. Should you force vmx_set_segment(CS) with emulate_invalid_guest_state=1? (then it would be fixed unconditionally). > See > big switch at the end of the function there. Note that here > rmode_segment_valid() does not check register properly anyway. It ignores > db value and allow guest entry with limit > 0xffff. My patches fixes > this. After the patches are applied rmode_segment_valid() still relax > dpl and CS.type check: > > var.dpl = 0x3; > if (seg == VCPU_SREG_CS) > var.type = 0x3; > > but fix_rmode_seg() unconditionally does exactly same: > > var.dpl = 0x3; > if (seg == VCPU_SREG_CS) > var.type = 0x3; > > > Does this defeat the purpose of emulate_invalid_guest_state=1? > No since changing dpl to 3 and segment register type to 3 does not > affect instruction execution in vm86 mode, so entering vcpu in vm86 > mode or fully emulate all instruction will yield exactly same result > no matter what instructions are executed. If we do not relax this check > in rmode_segment_valid() all real mode will be fully emulated because > no guest ever configure CS register to data type and set DPL of all > segments to 3 before entering real mode. > > -- > Gleb. OK, makes sense. See comment above about not fixing CS unconditionally on eigs=1.