Re: [PATCH 11/24] Implement VMPTRST
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'Eln...@il.ibm.com
---
--- .before/arch/x86/kvm/x86.c 2010-06-13 15:01:29.0 +0300
+++ .after/arch/x86/kvm/x86.c2010-06-13 15:01:29.0 +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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Gleb.
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 11/24] Implement VMPTRST
On Wed, Jun 16, 2010, Gleb Natapov wrote about Re: [PATCH 11/24] Implement VMPTRST: On Mon, Jun 14, 2010 at 12:15:10PM +0300, Avi Kivity wrote: 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 :( If I remember the history correctly, this is exactly what happened in this code. We used to use kvm_write_guest_virt(), until a few months ago it disappeared. I thought it to be fine to call write_guest_virt_system() because, like you said, we only already check above that cpl=0, so it is fine to assume we have cpl 0 privileges. So while it might look a bit strange at first, I think it should be fine and there's no need to create more functions. -- Nadav Har'El|Wednesday, Jun 16 2010, 4 Tammuz 5770 n...@math.technion.ac.il |- Phone +972-523-790466, ICQ 13349191 |Amateurs built the ark - professionals http://nadav.harel.org.il |built the Titanic. -- To unsubscribe from this list: send the line unsubscribe kvm in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 11/24] Implement VMPTRST
On 06/13/2010 03:28 PM, Nadav Har'El wrote:
This patch implements the VMPTRST instruction.
Signed-off-by: Nadav Har'Eln...@il.ibm.com
---
--- .before/arch/x86/kvm/x86.c 2010-06-13 15:01:29.0 +0300
+++ .after/arch/x86/kvm/x86.c 2010-06-13 15:01:29.0 +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.
+/* 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 majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 11/24] Implement VMPTRST
This patch implements the VMPTRST instruction.
Signed-off-by: Nadav Har'El n...@il.ibm.com
---
--- .before/arch/x86/kvm/x86.c 2010-06-13 15:01:29.0 +0300
+++ .after/arch/x86/kvm/x86.c 2010-06-13 15:01:29.0 +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)
@@ -,6 +,7 @@ static int kvm_write_guest_virt_system(g
out:
return r;
}
+EXPORT_SYMBOL_GPL(kvm_write_guest_virt_system);
static int emulator_read_emulated(unsigned long addr,
void *val,
--- .before/arch/x86/kvm/x86.h 2010-06-13 15:01:29.0 +0300
+++ .after/arch/x86/kvm/x86.h 2010-06-13 15:01:29.0 +0300
@@ -78,6 +78,9 @@ void kvm_after_handle_nmi(struct kvm_vcp
int kvm_read_guest_virt(gva_t addr, void *val, unsigned int bytes,
struct kvm_vcpu *vcpu, u32 *error);
+int kvm_write_guest_virt_system(gva_t addr, void *val, unsigned int bytes,
+struct kvm_vcpu *vcpu, u32 *error);
+
extern int nested;
#endif
--- .before/arch/x86/kvm/vmx.c 2010-06-13 15:01:29.0 +0300
+++ .after/arch/x86/kvm/vmx.c 2010-06-13 15:01:29.0 +0300
@@ -3940,6 +3940,33 @@ static int handle_vmptrld(struct kvm_vcp
return 1;
}
+/* 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;
+ r = kvm_write_guest_virt_system(vmcs_gva,
+(void *)to_vmx(vcpu)-nested.current_vmptr,
+sizeof(u64), vcpu, NULL);
+ if (r) {
+ printk(KERN_INFO %s failed to write vmptr\n, __func__);
+ return 1;
+ }
+ clear_rflags_cf_zf(vcpu);
+ skip_emulated_instruction(vcpu);
+ return 1;
+}
+
static int handle_invlpg(struct kvm_vcpu *vcpu)
{
unsigned long exit_qualification = vmcs_readl(EXIT_QUALIFICATION);
@@ -4225,7 +4252,7 @@ static int (*kvm_vmx_exit_handlers[])(st
[EXIT_REASON_VMCLEAR] = handle_vmclear,
[EXIT_REASON_VMLAUNCH]= handle_vmx_insn,
[EXIT_REASON_VMPTRLD] = handle_vmptrld,
- [EXIT_REASON_VMPTRST] = handle_vmx_insn,
+ [EXIT_REASON_VMPTRST] = handle_vmptrst,
[EXIT_REASON_VMREAD] = handle_vmx_insn,
[EXIT_REASON_VMRESUME]= handle_vmx_insn,
[EXIT_REASON_VMWRITE] = handle_vmx_insn,
--
To unsubscribe from this list: send the line unsubscribe kvm in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
