Re: [PATCH 11/24] Implement VMPTRST

[Gleb Natapov <gleb@redhat.com>]

On Mon, Jun 14, 2010 at 12:15:10PM +0300, Avi Kivity wrote:
> On 06/13/2010 03:28 PM, Nadav Har'El wrote:
> >This patch implements the VMPTRST instruction.
> >
> >Signed-off-by: Nadav Har'El<nyh@il.ibm.com>
> >---
> >--- .before/arch/x86/kvm/x86.c	2010-06-13 15:01:29.000000000 +0300
> >+++ .after/arch/x86/kvm/x86.c	2010-06-13 15:01:29.000000000 +0300
> >@@ -3301,7 +3301,7 @@ static int kvm_read_guest_virt_system(gv
> >  	return kvm_read_guest_virt_helper(addr, val, bytes, vcpu, 0, error);
> >  }
> >
> >-static int kvm_write_guest_virt_system(gva_t addr, void *val,
> >+int kvm_write_guest_virt_system(gva_t addr, void *val,
> >  				       unsigned int bytes,
> >  				       struct kvm_vcpu *vcpu,
> >  				       u32 *error)
> 
> write_guest_virt_system() is used by writes which need to ignore the
> cpl, for example when a cpl 3 instruction loads a segment, the
> processor needs to update the accessed flag even though it is only
> accessible to cpl 0.  That's not your case, you need the ordinary
> write_guest_virt().
> 
> Um, I see there is no kvm_write_guest_virt(), you'll have to introduce it.
> 
the code uses this function after checking cpl to be zero, so may be it
is ok, not to pretty though. I was actually hoping to get rid of all
kvm_(read|write)_guest_virt* and replace existing uses with
emulator_(read|write)_emulated, but this patch series adds more users
that will be hard to replace :(

> >
> >+/* Emulate the VMPTRST instruction */
> >+static int handle_vmptrst(struct kvm_vcpu *vcpu)
> >+{
> >+	int r = 0;
> >+	unsigned long exit_qualification = vmcs_readl(EXIT_QUALIFICATION);
> >+	u32 vmx_instruction_info = vmcs_read32(VMX_INSTRUCTION_INFO);
> >+	gva_t vmcs_gva;
> >+
> >+	if (!nested_vmx_check_permission(vcpu))
> >+		return 1;
> >+
> >+	vmcs_gva = get_vmx_mem_address(vcpu, exit_qualification,
> >+				       vmx_instruction_info);
> >+	if (vmcs_gva == 0)
> >+		return 1;
> 
> What's wrong with gva 0?  It's favoured by exploiters everywhere.
> 
> >+	r = kvm_write_guest_virt_system(vmcs_gva,
> >+				 (void *)&to_vmx(vcpu)->nested.current_vmptr,
> >+				 sizeof(u64), vcpu, NULL);
> >+	if (r) {
> 
> Check against the X86EMUL return codes.  You'll need to inject a
> page fault on failure.
> 
> >+		printk(KERN_INFO "%s failed to write vmptr\n", __func__);
> >+		return 1;
> >+	}
> >+	clear_rflags_cf_zf(vcpu);
> >+	skip_emulated_instruction(vcpu);
> >+	return 1;
> >+}
> >+
> 
> -- 
> error compiling committee.c: too many arguments to function
> 
> --
> 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

--
			Gleb.