KVM: nVMX: Disable preemption while reading from shadow VMCS

PATCH In review Jan Kiszka 4 participants 6 messages Oct 8, 2014 – Oct 9, 2014

Subsystems (3)

x86 KVM KVM x86

lore

Share & tools

lore

Jan Kiszka

author

2014-10-08 16:05 UTC #0 lore raw

PATCH 1/1 KVM: nVMX: Disable preemption while reading from shadow VMCS

In order to access the shadow VMCS, we need to load it. At this point, vmx->loaded_vmcs->vmcs and the actually loaded one start to differ. If we now get preempted by Linux, vmx_vcpu_put and, on return, the vmx_vcpu_load will work against the wrong vmcs. That can cause copy_shadow_to_vmcs12 to corrupt the vmcs12 state.

Fix the issue by disabling preemption during the copy operation.

copy_vmcs12_to_shadow is safe from this issue as it is executed by vmx_vcpu_run when preemption is already disabled before vmentry.

Signed-off-by Jan Kiszka <[email protected]>

---

This fixes specifically Jailhouse in KVM on CPUs with shadow VMCS support.

 arch/x86/kvm/vmx.c | 4 ++++
 1 file changed, 4 insertions(+)

arch/x86/kvm/vmx.c +4 -0

index 04fa1b8..f3de106 100644
@@ -6417,6 +6417,8 @@ static void copy_shadow_to_vmcs12(struct vcpu_vmx *vmx)
 	const unsigned long *fields = shadow_read_write_fields;
 	const int num_fields = max_shadow_read_write_fields;
 
+	preempt_disable();
+
 	vmcs_load(shadow_vmcs);
 
 	for (i = 0; i < num_fields; i++) {

@@ -6440,6 +6442,8 @@ static void copy_shadow_to_vmcs12(struct vcpu_vmx *vmx)
 
 	vmcs_clear(shadow_vmcs);
 	vmcs_load(vmx->loaded_vmcs->vmcs);
+
+	preempt_enable();
 }
 
 static void copy_vmcs12_to_shadow(struct vcpu_vmx *vmx)

Signature · 6 lines

1.8.4.5

-- 
You received this message because you are subscribed to the Google Groups "Jailhouse" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [email protected].
For more options, visit https://groups.google.com/d/optout.

Bandan Das

2014-10-08 19:07 UTC #1 lore raw

Quoted 5 lines · Jan Kiszka

In order to access the shadow VMCS, we need to load it. At this point, vmx->loaded_vmcs->vmcs and the actually loaded one start to differ. If we now get preempted by Linux, vmx_vcpu_put and, on return, the vmx_vcpu_load will work against the wrong vmcs. That can cause copy_shadow_to_vmcs12 to corrupt the vmcs12 state.

Ouch! I apologize if I missed this in the previous discussion but why do we never get into this condition while running a Linux guest ?

Will there be a performance impact of this change ? I hope it's negligible though..

Quoted 36 lines

Fix the issue by disabling preemption during the copy operation.
copy_vmcs12_to_shadow is safe from this issue as it is executed by vmx_vcpu_run when preemption is already disabled before vmentry.
Signed-off-by Jan Kiszka <[email protected]>
---
This fixes specifically Jailhouse in KVM on CPUs with shadow VMCS support.
 arch/x86/kvm/vmx.c | 4 ++++
 1 file changed, 4 insertions(+)
arch/x86/kvm/vmx.c +4 -0
index 04fa1b8..f3de106 100644
@@ -6417,6 +6417,8 @@ static void copy_shadow_to_vmcs12(struct vcpu_vmx *vmx)
 	const unsigned long *fields = shadow_read_write_fields;
 	const int num_fields = max_shadow_read_write_fields;
 
+	preempt_disable();
+
 	vmcs_load(shadow_vmcs);
 
 	for (i = 0; i < num_fields; i++) {
@@ -6440,6 +6442,8 @@ static void copy_shadow_to_vmcs12(struct vcpu_vmx *vmx)
 
 	vmcs_clear(shadow_vmcs);
 	vmcs_load(vmx->loaded_vmcs->vmcs);
+
+	preempt_enable();
 }
 
 static void copy_vmcs12_to_shadow(struct vcpu_vmx *vmx)

Signature · 3 lines

You received this message because you are subscribed to the Google Groups "Jailhouse" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [email protected].
For more options, visit https://groups.google.com/d/optout.

Jan Kiszka

author

2014-10-08 19:19 UTC #2 lore raw

Quoted 10 lines · Bandan Das

Quoted 5 lines · Jan Kiszka
In order to access the shadow VMCS, we need to load it. At this point, vmx->loaded_vmcs->vmcs and the actually loaded one start to differ. If we now get preempted by Linux, vmx_vcpu_put and, on return, the vmx_vcpu_load will work against the wrong vmcs. That can cause copy_shadow_to_vmcs12 to corrupt the vmcs12 state.
Ouch! I apologize if I missed this in the previous discussion but why do we never get into this condition while running a Linux guest ?

Well, you need high load on the host, preemption at the "wrong" time, and some relevant difference between the still to-be-copied shadow vmcs fields between the proper vmcs and the real one that was used by KVM at that point. I don't think it is a Jailhouse-only issue, but other hypervisors may be less sensitive.

Quoted 3 lines

Will there be a performance impact of this change ? I hope it's negligible though..

The preemption counter is cheap, the only impact should be on scheduling latency. But that is irrelevant as long as we have the counterpart under the same preemption lock.

Jan

Quoted 37 lines

Quoted 37 lines
Fix the issue by disabling preemption during the copy operation.
copy_vmcs12_to_shadow is safe from this issue as it is executed by vmx_vcpu_run when preemption is already disabled before vmentry.
Signed-off-by Jan Kiszka <[email protected]>
---
This fixes specifically Jailhouse in KVM on CPUs with shadow VMCS support.
 arch/x86/kvm/vmx.c | 4 ++++
 1 file changed, 4 insertions(+)
arch/x86/kvm/vmx.c +4 -0
index 04fa1b8..f3de106 100644
@@ -6417,6 +6417,8 @@ static void copy_shadow_to_vmcs12(struct vcpu_vmx *vmx)
 	const unsigned long *fields = shadow_read_write_fields;
 	const int num_fields = max_shadow_read_write_fields;
 
+	preempt_disable();
+
 	vmcs_load(shadow_vmcs);
 
 	for (i = 0; i < num_fields; i++) {
@@ -6440,6 +6442,8 @@ static void copy_shadow_to_vmcs12(struct vcpu_vmx *vmx)
 
 	vmcs_clear(shadow_vmcs);
 	vmcs_load(vmx->loaded_vmcs->vmcs);
+
+	preempt_enable();
 }
 
 static void copy_vmcs12_to_shadow(struct vcpu_vmx *vmx)

Signature · 7 lines

Siemens AG, Corporate Technology, CT RTC ITP SES-DE
Corporate Competence Center Embedded Linux

-- 
You received this message because you are subscribed to the Google Groups "Jailhouse" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [email protected].
For more options, visit https://groups.google.com/d/optout.

Paolo Bonzini

maintainer

2014-10-08 20:21 UTC #3 lore raw

Il 08/10/2014 21:19, Jan Kiszka ha scritto:

Quoted 8 lines

Quoted 2 lines
Ouch! I apologize if I missed this in the previous discussion but why do we never get into this condition while running a Linux guest ?
Well, you need high load on the host, preemption at the "wrong" time, and some relevant difference between the still to-be-copied shadow vmcs fields between the proper vmcs and the real one that was used by KVM at that point. I don't think it is a Jailhouse-only issue, but other hypervisors may be less sensitive.

Indeed; in the particular case of the virtual interrupt pending bit it's easy to see why it is not a problem for Linux guests:
handle_interrupt_window hardly does anything, you will just have a useless KVM_REQ_EVENT request.

Running many different L2 guests (with an overcommitted host) should expose it. Note that the memory map and selectors are relatively similar among similar guests, and the most scary fields (RIP and RSP), are copied first when preemption is less likely. It's possible that moving them last would increase the chance of breakage.

Paolo

Signature · 3 lines

You received this message because you are subscribed to the Google Groups "Jailhouse" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [email protected].
For more options, visit https://groups.google.com/d/optout.

Paolo Bonzini

maintainer

2014-10-08 20:03 UTC #4 lore raw

In reply to Jan Kiszka

Il 08/10/2014 18:05, Jan Kiszka ha scritto:

Quoted 8 lines

In order to access the shadow VMCS, we need to load it. At this point, vmx->loaded_vmcs->vmcs and the actually loaded one start to differ. If we now get preempted by Linux, vmx_vcpu_put and, on return, the vmx_vcpu_load will work against the wrong vmcs. That can cause copy_shadow_to_vmcs12 to corrupt the vmcs12 state.
Fix the issue by disabling preemption during the copy operation.

Quoted 8 lines

copy_vmcs12_to_shadow is safe from this issue as it is executed by vmx_vcpu_run when preemption is already disabled before vmentry.
Signed-off-by Jan Kiszka <[email protected]>
---
This fixes specifically Jailhouse in KVM on CPUs with shadow VMCS support.

Thanks. I'll add this above your SoB line:

This bug is exposed by running Jailhouse within KVM on CPUs with shadow VMCS support. Jailhouse never expects an interrupt pending vmexit, but the bug can cause it if, after copy_shadow_to_vmcs12 is preempted, the active VMCS happens to have the virtual interrupt pending flag set in the CPU-based execution controls.

Paolo

Signature · 3 lines

You received this message because you are subscribed to the Google Groups "Jailhouse" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [email protected].
For more options, visit https://groups.google.com/d/optout.

Wanpeng Li

Reviewed-by on v1 1/1

2014-10-09 00:00 UTC #5 lore raw

In reply to Jan Kiszka

Quoted 6 lines · Jan Kiszka

In order to access the shadow VMCS, we need to load it. At this point, vmx->loaded_vmcs->vmcs and the actually loaded one start to differ. If we now get preempted by Linux, vmx_vcpu_put and, on return, the vmx_vcpu_load will work against the wrong vmcs. That can cause copy_shadow_to_vmcs12 to corrupt the vmcs12 state.

Quoted 7 lines

Fix the issue by disabling preemption during the copy operation.
copy_vmcs12_to_shadow is safe from this issue as it is executed by vmx_vcpu_run when preemption is already disabled before vmentry.
Signed-off-by Jan Kiszka <[email protected]>
---

Reviewed-by Wanpeng Li <[email protected]>

Regards,
Wanpeng Li

Quoted 31 lines

This fixes specifically Jailhouse in KVM on CPUs with shadow VMCS support.

arch/x86/kvm/vmx.c | 4 ++++

1 file changed, 4 insertions(+)

arch/x86/kvm/vmx.c +4 -0

index 04fa1b8..f3de106 100644
@@ -6417,6 +6417,8 @@ static void copy_shadow_to_vmcs12(struct vcpu_vmx *vmx)
 	const unsigned long *fields = shadow_read_write_fields;
 	const int num_fields = max_shadow_read_write_fields;
 
+	preempt_disable();
+
 	vmcs_load(shadow_vmcs);
 
 	for (i = 0; i < num_fields; i++) {

@@ -6440,6 +6442,8 @@ static void copy_shadow_to_vmcs12(struct vcpu_vmx *vmx)
 
 	vmcs_clear(shadow_vmcs);
 	vmcs_load(vmx->loaded_vmcs->vmcs);
+
+	preempt_enable();
 }
 
 static void copy_vmcs12_to_shadow(struct vcpu_vmx *vmx)

Signature · 1 line

1.8.4.5

Signature · 3 lines

You received this message because you are subscribed to the Google Groups "Jailhouse" group.
To unsubscribe from this group and stop receiving emails from it, send an email to [email protected].
For more options, visit https://groups.google.com/d/optout.