KVM: nVMX: Clear pending KVM_REQ_GET_VMCS12_PAGES when leaving nested

Commit Message

Jan Kiszka July 21, 2019, 11:52 a.m. UTC

From: Jan Kiszka <[e-mail]>

Letting this pend may cause nested_get_vmcs12_pages to run against an
invalid state, corrupting the effective vmcs of L1.

This was triggerable in QEMU after a guest corruption in L2, followed by
a L1 reset.

Signed-off-by: Jan Kiszka <[e-mail]>
---

And another gremlin. I'm afraid there is at least one more because
vmport access from L2 is still failing in QEMU. This is just another
fallout from that. At least the host seems stable now.

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

--
2.16.4

Comments

Liran Alon July 21, 2019, 11:57 a.m. UTC | #1

> On 21 Jul 2019, at 14:52, Jan Kiszka <[e-mail]> wrote:
> 
> From: Jan Kiszka <[e-mail]>
> 
> Letting this pend may cause nested_get_vmcs12_pages to run against an
> invalid state, corrupting the effective vmcs of L1.
> 
> This was triggerable in QEMU after a guest corruption in L2, followed by
> a L1 reset.
> 
> Signed-off-by: Jan Kiszka <[e-mail]>

Good catch.
Reviewed-by: Liran Alon <[e-mail]>

This would have been more easily diagnosed in case free_nested() would NULL cached_vmcs12 and cached_shadow_vmcs12
after kfree() and add to get_vmcs12() & get_shadow_vmcs12() a relevant BUG_ON() call.

I would submit such a patch separately.

-Liran

> ---
> 
> And another gremlin. I'm afraid there is at least one more because
> vmport access from L2 is still failing in QEMU. This is just another
> fallout from that. At least the host seems stable now.
> 
> arch/x86/kvm/vmx/nested.c | 2 ++
> 1 file changed, 2 insertions(+)
> 
> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index 0f1378789bd0..4cdab4b4eff1 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -220,6 +220,8 @@ static void free_nested(struct kvm_vcpu *vcpu)
> 	if (!vmx->nested.vmxon && !vmx->nested.smm.vmxon)
> 		return;
> 
> +	kvm_clear_request(KVM_REQ_GET_VMCS12_PAGES, vcpu);
> +
> 	vmx->nested.vmxon = false;
> 	vmx->nested.smm.vmxon = false;
> 	free_vpid(vmx->nested.vpid02);
> --
> 2.16.4

Jan Kiszka July 21, 2019, 2 p.m. UTC | #2

On 21.07.19 13:57, Liran Alon wrote:
>
>
>> On 21 Jul 2019, at 14:52, Jan Kiszka <[e-mail]> wrote:
>>
>> From: Jan Kiszka <[e-mail]>
>>
>> Letting this pend may cause nested_get_vmcs12_pages to run against an
>> invalid state, corrupting the effective vmcs of L1.
>>
>> This was triggerable in QEMU after a guest corruption in L2, followed by
>> a L1 reset.
>>
>> Signed-off-by: Jan Kiszka <[e-mail]>
>
> Good catch.
> Reviewed-by: Liran Alon <[e-mail]>
>
> This would have been more easily diagnosed in case free_nested() would NULL cached_vmcs12 and cached_shadow_vmcs12
> after kfree() and add to get_vmcs12() & get_shadow_vmcs12() a relevant BUG_ON() call.

The NULL'ifying makes sense, patch follows. But the helpers are too often called
unconditionally, thus cause false positives when adding the BUG_ON.

Jan

>
> I would submit such a patch separately.
>
> -Liran
>
>> ---
>>
>> And another gremlin. I'm afraid there is at least one more because
>> vmport access from L2 is still failing in QEMU. This is just another
>> fallout from that. At least the host seems stable now.
>>
>> arch/x86/kvm/vmx/nested.c | 2 ++
>> 1 file changed, 2 insertions(+)
>>
>> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
>> index 0f1378789bd0..4cdab4b4eff1 100644
>> --- a/arch/x86/kvm/vmx/nested.c
>> +++ b/arch/x86/kvm/vmx/nested.c
>> @@ -220,6 +220,8 @@ static void free_nested(struct kvm_vcpu *vcpu)
>> 	if (!vmx->nested.vmxon && !vmx->nested.smm.vmxon)
>> 		return;
>>
>> +	kvm_clear_request(KVM_REQ_GET_VMCS12_PAGES, vcpu);
>> +
>> 	vmx->nested.vmxon = false;
>> 	vmx->nested.smm.vmxon = false;
>> 	free_vpid(vmx->nested.vpid02);
>> --
>> 2.16.4

Liran Alon July 21, 2019, 2:03 p.m. UTC | #3

> On 21 Jul 2019, at 17:00, Jan Kiszka <[e-mail]> wrote:
> 
> On 21.07.19 13:57, Liran Alon wrote:
>> 
>> 
>>> On 21 Jul 2019, at 14:52, Jan Kiszka <[e-mail]> wrote:
>>> 
>>> From: Jan Kiszka <[e-mail]>
>>> 
>>> Letting this pend may cause nested_get_vmcs12_pages to run against an
>>> invalid state, corrupting the effective vmcs of L1.
>>> 
>>> This was triggerable in QEMU after a guest corruption in L2, followed by
>>> a L1 reset.
>>> 
>>> Signed-off-by: Jan Kiszka <[e-mail]>
>> 
>> Good catch.
>> Reviewed-by: Liran Alon <[e-mail]>
>> 
>> This would have been more easily diagnosed in case free_nested() would NULL cached_vmcs12 and cached_shadow_vmcs12
>> after kfree() and add to get_vmcs12() & get_shadow_vmcs12() a relevant BUG_ON() call.
> 
> The NULL'ifying makes sense, patch follows. But the helpers are too often called
> unconditionally, thus cause false positives when adding the BUG_ON.

How would having a BUG_ON(!cached_vmcs12) on get_vmcs12() will cause false positive?
I don’t see any legit case it is called and return NULL.

-Liran

> 
> Jan
> 
>> 
>> I would submit such a patch separately.
>> 
>> -Liran
>> 
>>> ---
>>> 
>>> And another gremlin. I'm afraid there is at least one more because
>>> vmport access from L2 is still failing in QEMU. This is just another
>>> fallout from that. At least the host seems stable now.
>>> 
>>> arch/x86/kvm/vmx/nested.c | 2 ++
>>> 1 file changed, 2 insertions(+)
>>> 
>>> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
>>> index 0f1378789bd0..4cdab4b4eff1 100644
>>> --- a/arch/x86/kvm/vmx/nested.c
>>> +++ b/arch/x86/kvm/vmx/nested.c
>>> @@ -220,6 +220,8 @@ static void free_nested(struct kvm_vcpu *vcpu)
>>> 	if (!vmx->nested.vmxon && !vmx->nested.smm.vmxon)
>>> 		return;
>>> 
>>> +	kvm_clear_request(KVM_REQ_GET_VMCS12_PAGES, vcpu);
>>> +
>>> 	vmx->nested.vmxon = false;
>>> 	vmx->nested.smm.vmxon = false;
>>> 	free_vpid(vmx->nested.vpid02);
>>> --
>>> 2.16.4

Paolo Bonzini July 21, 2019, 4:42 p.m. UTC | #4

On 21/07/19 16:03, Liran Alon wrote:
> How would having a BUG_ON(!cached_vmcs12) on get_vmcs12() will cause false positive?
> I don’t see any legit case it is called and return NULL.

For example, vmx_read_l1_tsc_offset and vmx_write_l1_tsc_offset call it
unconditionally, but then only use it if is_guest_mode(vcpu).

Paolo

Paolo Bonzini July 22, 2019, 11:52 a.m. UTC | #5

On 21/07/19 13:52, Jan Kiszka wrote:
> From: Jan Kiszka <[e-mail]>
> 
> Letting this pend may cause nested_get_vmcs12_pages to run against an
> invalid state, corrupting the effective vmcs of L1.
> 
> This was triggerable in QEMU after a guest corruption in L2, followed by
> a L1 reset.
> 
> Signed-off-by: Jan Kiszka <[e-mail]>
> ---
> 
> And another gremlin. I'm afraid there is at least one more because
> vmport access from L2 is still failing in QEMU. This is just another
> fallout from that. At least the host seems stable now.
> 
>  arch/x86/kvm/vmx/nested.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index 0f1378789bd0..4cdab4b4eff1 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -220,6 +220,8 @@ static void free_nested(struct kvm_vcpu *vcpu)
>  	if (!vmx->nested.vmxon && !vmx->nested.smm.vmxon)
>  		return;
> 
> +	kvm_clear_request(KVM_REQ_GET_VMCS12_PAGES, vcpu);
> +
>  	vmx->nested.vmxon = false;
>  	vmx->nested.smm.vmxon = false;
>  	free_vpid(vmx->nested.vpid02);
> --
> 2.16.4
> 

Queued, thanks.

Paolo

Patch

diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
index 0f1378789bd0..4cdab4b4eff1 100644
--- a/arch/x86/kvm/vmx/nested.c
+++ b/arch/x86/kvm/vmx/nested.c
@@ -220,6 +220,8 @@  static void free_nested(struct kvm_vcpu *vcpu)
 	if (!vmx->nested.vmxon && !vmx->nested.smm.vmxon)
 		return;

+	kvm_clear_request(KVM_REQ_GET_VMCS12_PAGES, vcpu);
+
 	vmx->nested.vmxon = false;
 	vmx->nested.smm.vmxon = false;
 	free_vpid(vmx->nested.vpid02);