Re: 2.6.38-rc6: general protection error inside KVM 64 bits guest

[Gleb Natapov <gleb@redhat.com>]

On Sun, Mar 06, 2011 at 02:55:17PM +0200, Avi Kivity wrote:
> On 03/06/2011 02:42 PM, Gleb Natapov wrote:
> >>
> >IO permission checking for 64-bit guest in KVM is wrong. The patch bellow
> >should fix it.
> >
> >
> >diff --git a/arch/x86/include/asm/kvm_emulate.h b/arch/x86/include/asm/kvm_emulate.h
> >index 50ebc32..7ef5b86 100644
> >--- a/arch/x86/include/asm/kvm_emulate.h
> >+++ b/arch/x86/include/asm/kvm_emulate.h
> >@@ -142,10 +142,8 @@ struct x86_emulate_ops {
> >  	int (*pio_out_emulated)(int size, unsigned short port, const void *val,
> >  				unsigned int count, struct kvm_vcpu *vcpu);
> >
> >-	bool (*get_cached_descriptor)(struct desc_struct *desc,
> >-				      int seg, struct kvm_vcpu *vcpu);
> >-	void (*set_cached_descriptor)(struct desc_struct *desc,
> >-				      int seg, struct kvm_vcpu *vcpu);
> >+	bool (*get_cached_descriptor)(void *p, int seg, struct kvm_vcpu *vcpu);
> >+	void (*set_cached_descriptor)(void *p, int seg, struct kvm_vcpu *vcpu);
> 
> Ugh, void *.
> 
> Add a u64 *highword parameter, or something.
> 
I modeled this after set_tssldt_descriptor() which takes void and cast
it to different structures depending on 64bit/32bit, but u32 *highword
will work too.

> >@@ -1764,25 +1764,35 @@ static bool emulator_io_port_access_allowed(struct x86_emulate_ctxt *ctxt,
> >  					    struct x86_emulate_ops *ops,
> >  					    u16 port, u16 len)
> >  {
> >-	struct desc_struct tr_seg;
> >+	union {
> >+		struct desc_struct tss32;
> >+#ifdef CONFIG_X86_64
> >+		struct ldttss_desc64 tss64;
> >+#endif
> >+	} tr_seg;
> >  	int r;
> >  	u16 io_bitmap_ptr;
> >  	u8 perm, bit_idx = port&  0x7;
> >  	unsigned mask = (1<<  len) - 1;
> >+	unsigned long base;
> >
> >  	ops->get_cached_descriptor(&tr_seg, VCPU_SREG_TR, ctxt->vcpu);
> >-	if (!tr_seg.p)
> >+	if (!tr_seg.tss32.p)
> >  		return false;
> >-	if (desc_limit_scaled(&tr_seg)<  103)
> >+	if (desc_limit_scaled(&tr_seg.tss32)<  103)
> >  		return false;
> >-	r = ops->read_std(get_desc_base(&tr_seg) + 102,&io_bitmap_ptr, 2,
> >-			  ctxt->vcpu, NULL);
> >+	base = get_desc_base(&tr_seg.tss32);
> >+#ifdef CONFIG_X86_64
> >+	if (ctxt->mode == X86EMUL_MODE_PROT64)
> >+		base |= ((u64)tr_seg.tss64.base3)<<  32;
> >+#endif
> >+	r = ops->read_std(base + 102,&io_bitmap_ptr, 2, ctxt->vcpu, NULL);
> >  	if (r != X86EMUL_CONTINUE)
> >  		return false;
> 
> Note: if we get a fault here, we ought to propagate it.  Only
> happens if there's a race, since the cpu checks for these
> exceptions.
> 
Do you mean we need to propagate fault that read_std() returns and not
always inject GP like we do now?

--
			Gleb.