| Message ID | 1355332255-26612-3-git-send-email-gleb@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
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 <gleb@redhat.com> > --- > 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. Does this defeat the purpose of emulate_invalid_guest_state=1? -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <gleb@redhat.com> > > --- > > 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(). 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. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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 <gleb@redhat.com> > > > --- > > > 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. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
On Sat, Dec 22, 2012 at 12:55:43PM -0200, Marcelo Tosatti wrote: > 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 <gleb@redhat.com> > > > > --- > > > > 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). Oh, patch 3 fixes that, sorry. ACK. -- To unsubscribe from this list: send the line "unsubscribe kvm" in the body of a message to majordomo@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
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))
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 <gleb@redhat.com> --- arch/x86/kvm/vmx.c | 2 ++ 1 file changed, 2 insertions(+)