* [PATCH] KVM: nVMX: Clear pending KVM_REQ_GET_VMCS12_PAGES when leaving nested
@ 2019-07-21 11:52 Jan Kiszka
2019-07-21 11:57 ` Liran Alon
2019-07-22 11:52 ` Paolo Bonzini
0 siblings, 2 replies; 6+ messages in thread
From: Jan Kiszka @ 2019-07-21 11:52 UTC (permalink / raw)
To: Paolo Bonzini, kvm; +Cc: Liran Alon, Jim Mattson, KarimAllah Ahmed
From: Jan Kiszka <jan.kiszka@siemens.com>
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 <jan.kiszka@siemens.com>
---
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
^ permalink raw reply related [flat|nested] 6+ messages in thread
* Re: [PATCH] KVM: nVMX: Clear pending KVM_REQ_GET_VMCS12_PAGES when leaving nested
2019-07-21 11:52 [PATCH] KVM: nVMX: Clear pending KVM_REQ_GET_VMCS12_PAGES when leaving nested Jan Kiszka
@ 2019-07-21 11:57 ` Liran Alon
2019-07-21 14:00 ` Jan Kiszka
2019-07-22 11:52 ` Paolo Bonzini
1 sibling, 1 reply; 6+ messages in thread
From: Liran Alon @ 2019-07-21 11:57 UTC (permalink / raw)
To: Jan Kiszka; +Cc: Paolo Bonzini, kvm, Jim Mattson, KarimAllah Ahmed
> On 21 Jul 2019, at 14:52, Jan Kiszka <jan.kiszka@web.de> wrote:
>
> From: Jan Kiszka <jan.kiszka@siemens.com>
>
> 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 <jan.kiszka@siemens.com>
Good catch.
Reviewed-by: Liran Alon <liran.alon@oracle.com>
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] KVM: nVMX: Clear pending KVM_REQ_GET_VMCS12_PAGES when leaving nested
2019-07-21 11:57 ` Liran Alon
@ 2019-07-21 14:00 ` Jan Kiszka
2019-07-21 14:03 ` Liran Alon
0 siblings, 1 reply; 6+ messages in thread
From: Jan Kiszka @ 2019-07-21 14:00 UTC (permalink / raw)
To: Liran Alon; +Cc: Paolo Bonzini, kvm, Jim Mattson, KarimAllah Ahmed
On 21.07.19 13:57, Liran Alon wrote:
>
>
>> On 21 Jul 2019, at 14:52, Jan Kiszka <jan.kiszka@web.de> wrote:
>>
>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>
>> 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 <jan.kiszka@siemens.com>
>
> Good catch.
> Reviewed-by: Liran Alon <liran.alon@oracle.com>
>
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] KVM: nVMX: Clear pending KVM_REQ_GET_VMCS12_PAGES when leaving nested
2019-07-21 14:00 ` Jan Kiszka
@ 2019-07-21 14:03 ` Liran Alon
2019-07-21 16:42 ` Paolo Bonzini
0 siblings, 1 reply; 6+ messages in thread
From: Liran Alon @ 2019-07-21 14:03 UTC (permalink / raw)
To: Jan Kiszka; +Cc: Paolo Bonzini, kvm, Jim Mattson, KarimAllah Ahmed
> On 21 Jul 2019, at 17:00, Jan Kiszka <jan.kiszka@web.de> wrote:
>
> On 21.07.19 13:57, Liran Alon wrote:
>>
>>
>>> On 21 Jul 2019, at 14:52, Jan Kiszka <jan.kiszka@web.de> wrote:
>>>
>>> From: Jan Kiszka <jan.kiszka@siemens.com>
>>>
>>> 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 <jan.kiszka@siemens.com>
>>
>> Good catch.
>> Reviewed-by: Liran Alon <liran.alon@oracle.com>
>>
>> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] KVM: nVMX: Clear pending KVM_REQ_GET_VMCS12_PAGES when leaving nested
2019-07-21 14:03 ` Liran Alon
@ 2019-07-21 16:42 ` Paolo Bonzini
0 siblings, 0 replies; 6+ messages in thread
From: Paolo Bonzini @ 2019-07-21 16:42 UTC (permalink / raw)
To: Liran Alon, Jan Kiszka; +Cc: kvm, Jim Mattson, KarimAllah Ahmed
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
^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [PATCH] KVM: nVMX: Clear pending KVM_REQ_GET_VMCS12_PAGES when leaving nested
2019-07-21 11:52 [PATCH] KVM: nVMX: Clear pending KVM_REQ_GET_VMCS12_PAGES when leaving nested Jan Kiszka
2019-07-21 11:57 ` Liran Alon
@ 2019-07-22 11:52 ` Paolo Bonzini
1 sibling, 0 replies; 6+ messages in thread
From: Paolo Bonzini @ 2019-07-22 11:52 UTC (permalink / raw)
To: Jan Kiszka, kvm; +Cc: Liran Alon, Jim Mattson, KarimAllah Ahmed
On 21/07/19 13:52, Jan Kiszka wrote:
> From: Jan Kiszka <jan.kiszka@siemens.com>
>
> 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 <jan.kiszka@siemens.com>
> ---
>
> 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
^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2019-07-22 11:52 UTC | newest]
Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2019-07-21 11:52 [PATCH] KVM: nVMX: Clear pending KVM_REQ_GET_VMCS12_PAGES when leaving nested Jan Kiszka
2019-07-21 11:57 ` Liran Alon
2019-07-21 14:00 ` Jan Kiszka
2019-07-21 14:03 ` Liran Alon
2019-07-21 16:42 ` Paolo Bonzini
2019-07-22 11:52 ` Paolo Bonzini
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).