KVM: VMX: fix use after free of vmx->loaded_vmcs

PATCH Marcelo Tosatti 2 participants 4 messages Jan 3, 2014

Subsystems (3)

x86 KVM KVM x86

lore

Share & tools

lore

Marcelo Tosatti

author

2014-01-03 19:03 UTC #0 lore raw

After free_loaded_vmcs executes, the "loaded_vmcs" structure is kfreed, and now vmx->loaded_vmcs points to a kfreed area. Subsequent free_loaded_vmcs then attempts to manipulate vmx->loaded_vmcs.

Switch the order to avoid the problem.

https://bugzilla.redhat.com/show_bug.cgi?id=1047892

Signed-off-by Marcelo Tosatti <[email protected]>

arch/x86/kvm/vmx.c +1 -1

index da7837e..2efa33f0 100644
@@ -7332,8 +7332,8 @@ static void vmx_free_vcpu(struct kvm_vcpu *vcpu)
 	struct vcpu_vmx *vmx = to_vmx(vcpu);
 
 	free_vpid(vmx);
-	free_nested(vmx);
 	free_loaded_vmcs(vmx->loaded_vmcs);
+	free_nested(vmx);
 	kfree(vmx->guest_msrs);
 	kvm_vcpu_uninit(vcpu);
 	kmem_cache_free(kvm_vcpu_cache, vmx);

Jan Kiszka

2014-01-03 19:27 UTC #1 lore raw

Quoted 5 lines · Marcelo Tosatti

After free_loaded_vmcs executes, the "loaded_vmcs" structure is kfreed, and now vmx->loaded_vmcs points to a kfreed area. Subsequent free_loaded_vmcs then attempts to manipulate vmx->loaded_vmcs.

Cannot follow yet. How precisely do we call free_loaded_vmcs twice on the same loaded_vmcs? I thought the frees triggered by free_nested -> nested_free_all_saved_vmcss stay away from vmx->loaded_vmcs, no?

Jan

Quoted 22 lines

Switch the order to avoid the problem.

https://bugzilla.redhat.com/show_bug.cgi?id=1047892

Signed-off-by Marcelo Tosatti <[email protected]>

arch/x86/kvm/vmx.c +1 -1

index da7837e..2efa33f0 100644
@@ -7332,8 +7332,8 @@ static void vmx_free_vcpu(struct kvm_vcpu *vcpu)
 	struct vcpu_vmx *vmx = to_vmx(vcpu);
 
 	free_vpid(vmx);
-	free_nested(vmx);
 	free_loaded_vmcs(vmx->loaded_vmcs);
+	free_nested(vmx);
 	kfree(vmx->guest_msrs);
 	kvm_vcpu_uninit(vcpu);
 	kmem_cache_free(kvm_vcpu_cache, vmx);

Marcelo Tosatti

author

2014-01-03 19:36 UTC #2 lore raw

Quoted 9 lines · Jan Kiszka

Quoted 5 lines · Marcelo Tosatti
After free_loaded_vmcs executes, the "loaded_vmcs" structure is kfreed, and now vmx->loaded_vmcs points to a kfreed area. Subsequent free_loaded_vmcs then attempts to manipulate vmx->loaded_vmcs.
Cannot follow yet. How precisely do we call free_loaded_vmcs twice on the same loaded_vmcs?

You don't:

nested_free_all_saved_vmcss calls kfree(item). item is struct vmcs02_list *, which is:

/* Used to remember the last vmcs02 used for some recently used vmcs12s
* */

struct vmcs02_list {
        struct list_head list;
        gpa_t vmptr;
        struct loaded_vmcs vmcs02;
};

And vmx->loaded_vmcs = &item->vmcs02.

Quoted 2 lines

I thought the frees triggered by free_nested -> nested_free_all_saved_vmcss stay away from vmx->loaded_vmcs, no?

Stays away as far as free_loaded_vmcs, yes.
Except it frees the structure pointed to by vmx->loaded_vmcs.

The separate question is about when is vmcs01 ever allocated again if freed by nested_free_all_saved_vmcss (the other email).

Quoted 26 lines

Jan
Quoted 22 lines
Switch the order to avoid the problem.
https://bugzilla.redhat.com/show_bug.cgi?id=1047892
Signed-off-by Marcelo Tosatti <[email protected]>
arch/x86/kvm/vmx.c +1 -1
index da7837e..2efa33f0 100644
@@ -7332,8 +7332,8 @@ static void vmx_free_vcpu(struct kvm_vcpu *vcpu)
 	struct vcpu_vmx *vmx = to_vmx(vcpu);
 
 	free_vpid(vmx);
-	free_nested(vmx);
 	free_loaded_vmcs(vmx->loaded_vmcs);
+	free_nested(vmx);
 	kfree(vmx->guest_msrs);
 	kvm_vcpu_uninit(vcpu);
 	kmem_cache_free(kvm_vcpu_cache, vmx);

Jan Kiszka

Reviewed-by

2014-01-03 19:54 UTC #3 lore raw

Quoted 18 lines · Marcelo Tosatti

Quoted 9 lines · Jan Kiszka
Quoted 5 lines · Marcelo Tosatti
After free_loaded_vmcs executes, the "loaded_vmcs" structure is kfreed, and now vmx->loaded_vmcs points to a kfreed area. Subsequent free_loaded_vmcs then attempts to manipulate vmx->loaded_vmcs.
Cannot follow yet. How precisely do we call free_loaded_vmcs twice on the same loaded_vmcs?
You don't:
nested_free_all_saved_vmcss calls kfree(item). item is struct vmcs02_list *, which is:
/* Used to remember the last vmcs02 used for some recently used vmcs12s
* */

Quoted 7 lines

struct vmcs02_list {
        struct list_head list;
        gpa_t vmptr;
        struct loaded_vmcs vmcs02;
};

And vmx->loaded_vmcs = &item->vmcs02.

Yeah, now I see. You may add my

Reviewed-by Jan Kiszka <[email protected]>

Quoted 40 lines

Quoted 3 lines
I thought the frees triggered by free_nested -> nested_free_all_saved_vmcss stay away from vmx->loaded_vmcs, no?
Stays away as far as free_loaded_vmcs, yes.
Except it frees the structure pointed to by vmx->loaded_vmcs.
The separate question is about when is vmcs01 ever allocated again if freed by nested_free_all_saved_vmcss (the other email).
Quoted 26 lines
Jan
Quoted 22 lines
Switch the order to avoid the problem.
https://bugzilla.redhat.com/show_bug.cgi?id=1047892
Signed-off-by Marcelo Tosatti <[email protected]>
arch/x86/kvm/vmx.c +1 -1
index da7837e..2efa33f0 100644
@@ -7332,8 +7332,8 @@ static void vmx_free_vcpu(struct kvm_vcpu *vcpu)
 	struct vcpu_vmx *vmx = to_vmx(vcpu);
 
 	free_vpid(vmx);
-	free_nested(vmx);
 	free_loaded_vmcs(vmx->loaded_vmcs);
+	free_nested(vmx);
 	kfree(vmx->guest_msrs);
 	kvm_vcpu_uninit(vcpu);
 	kmem_cache_free(kvm_vcpu_cache, vmx);