[1/1,SRU,B/C] KVM: Fix UAF in nested posted interrupt processing

Message ID 1547074093-6066-2-git-send-email-tyhicks@canonical.com
State New
Headers show
Series
  • CVE-2018-16882 - Nested KVM DoS
Related show

Commit Message

Tyler Hicks Jan. 9, 2019, 10:48 p.m.
From: Cfir Cohen <cfir@google.com>

nested_get_vmcs12_pages() processes the posted_intr address in vmcs12. It
caches the kmap()ed page object and pointer, however, it doesn't handle
errors correctly: it's possible to cache a valid pointer, then release
the page and later dereference the dangling pointer.

I was able to reproduce with the following steps:

1. Call vmlaunch with valid posted_intr_desc_addr but an invalid
MSR_EFER. This causes nested_get_vmcs12_pages() to cache the kmap()ed
pi_desc_page and pi_desc. Later the invalid EFER value fails
check_vmentry_postreqs() which fails the first vmlaunch.

2. Call vmlanuch with a valid EFER but an invalid posted_intr_desc_addr
(I set it to 2G - 0x80). The second time we call nested_get_vmcs12_pages
pi_desc_page is unmapped and released and pi_desc_page is set to NULL
(the "shouldn't happen" clause). Due to the invalid
posted_intr_desc_addr, kvm_vcpu_gpa_to_page() fails and
nested_get_vmcs12_pages() returns. It doesn't return an error value so
vmlaunch proceeds. Note that at this time we have a dangling pointer in
vmx->nested.pi_desc and POSTED_INTR_DESC_ADDR in L0's vmcs.

3. Issue an IPI in L2 guest code. This triggers a call to
vmx_complete_nested_posted_interrupt() and pi_test_and_clear_on() which
dereferences the dangling pointer.

Vulnerable code requires nested and enable_apicv variables to be set to
true. The host CPU must also support posted interrupts.

Fixes: 5e2f30b756a37 "KVM: nVMX: get rid of nested_get_page()"
Cc: stable@vger.kernel.org
Reviewed-by: Andy Honig <ahonig@google.com>
Signed-off-by: Cfir Cohen <cfir@google.com>
Reviewed-by: Liran Alon <liran.alon@oracle.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>

CVE-2018-16882

(cherry picked from commit c2dd5146e9fe1f22c77c1b011adf84eea0245806)
Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
---
 arch/x86/kvm/vmx.c | 2 ++
 1 file changed, 2 insertions(+)

Comments

Stefan Bader Jan. 10, 2019, 8:45 a.m. | #1
On 09.01.19 23:48, Tyler Hicks wrote:
> From: Cfir Cohen <cfir@google.com>
> 
> nested_get_vmcs12_pages() processes the posted_intr address in vmcs12. It
> caches the kmap()ed page object and pointer, however, it doesn't handle
> errors correctly: it's possible to cache a valid pointer, then release
> the page and later dereference the dangling pointer.
> 
> I was able to reproduce with the following steps:
> 
> 1. Call vmlaunch with valid posted_intr_desc_addr but an invalid
> MSR_EFER. This causes nested_get_vmcs12_pages() to cache the kmap()ed
> pi_desc_page and pi_desc. Later the invalid EFER value fails
> check_vmentry_postreqs() which fails the first vmlaunch.
> 
> 2. Call vmlanuch with a valid EFER but an invalid posted_intr_desc_addr
> (I set it to 2G - 0x80). The second time we call nested_get_vmcs12_pages
> pi_desc_page is unmapped and released and pi_desc_page is set to NULL
> (the "shouldn't happen" clause). Due to the invalid
> posted_intr_desc_addr, kvm_vcpu_gpa_to_page() fails and
> nested_get_vmcs12_pages() returns. It doesn't return an error value so
> vmlaunch proceeds. Note that at this time we have a dangling pointer in
> vmx->nested.pi_desc and POSTED_INTR_DESC_ADDR in L0's vmcs.
> 
> 3. Issue an IPI in L2 guest code. This triggers a call to
> vmx_complete_nested_posted_interrupt() and pi_test_and_clear_on() which
> dereferences the dangling pointer.
> 
> Vulnerable code requires nested and enable_apicv variables to be set to
> true. The host CPU must also support posted interrupts.
> 
> Fixes: 5e2f30b756a37 "KVM: nVMX: get rid of nested_get_page()"
> Cc: stable@vger.kernel.org
> Reviewed-by: Andy Honig <ahonig@google.com>
> Signed-off-by: Cfir Cohen <cfir@google.com>
> Reviewed-by: Liran Alon <liran.alon@oracle.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> 
> CVE-2018-16882
> 
> (cherry picked from commit c2dd5146e9fe1f22c77c1b011adf84eea0245806)
> Signed-off-by: Tyler Hicks <tyhicks@canonical.com>
Acked-by: Stefan Bader <stefan.bader@canonical.com>
> ---
>  arch/x86/kvm/vmx.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 9fcc3ec3ab78..cb8c74ab0586 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -11028,6 +11028,8 @@ static void nested_get_vmcs12_pages(struct kvm_vcpu *vcpu,
>  			kunmap(vmx->nested.pi_desc_page);
>  			kvm_release_page_dirty(vmx->nested.pi_desc_page);
>  			vmx->nested.pi_desc_page = NULL;
> +			vmx->nested.pi_desc = NULL;
> +			vmcs_write64(POSTED_INTR_DESC_ADDR, -1ull);
>  		}
>  		page = kvm_vcpu_gpa_to_page(vcpu, vmcs12->posted_intr_desc_addr);
>  		if (is_error_page(page))
>
Kleber Souza Jan. 10, 2019, 2:41 p.m. | #2
On 1/9/19 11:48 PM, Tyler Hicks wrote:
> From: Cfir Cohen <cfir@google.com>
>
> nested_get_vmcs12_pages() processes the posted_intr address in vmcs12. It
> caches the kmap()ed page object and pointer, however, it doesn't handle
> errors correctly: it's possible to cache a valid pointer, then release
> the page and later dereference the dangling pointer.
>
> I was able to reproduce with the following steps:
>
> 1. Call vmlaunch with valid posted_intr_desc_addr but an invalid
> MSR_EFER. This causes nested_get_vmcs12_pages() to cache the kmap()ed
> pi_desc_page and pi_desc. Later the invalid EFER value fails
> check_vmentry_postreqs() which fails the first vmlaunch.
>
> 2. Call vmlanuch with a valid EFER but an invalid posted_intr_desc_addr
> (I set it to 2G - 0x80). The second time we call nested_get_vmcs12_pages
> pi_desc_page is unmapped and released and pi_desc_page is set to NULL
> (the "shouldn't happen" clause). Due to the invalid
> posted_intr_desc_addr, kvm_vcpu_gpa_to_page() fails and
> nested_get_vmcs12_pages() returns. It doesn't return an error value so
> vmlaunch proceeds. Note that at this time we have a dangling pointer in
> vmx->nested.pi_desc and POSTED_INTR_DESC_ADDR in L0's vmcs.
>
> 3. Issue an IPI in L2 guest code. This triggers a call to
> vmx_complete_nested_posted_interrupt() and pi_test_and_clear_on() which
> dereferences the dangling pointer.
>
> Vulnerable code requires nested and enable_apicv variables to be set to
> true. The host CPU must also support posted interrupts.
>
> Fixes: 5e2f30b756a37 "KVM: nVMX: get rid of nested_get_page()"
> Cc: stable@vger.kernel.org
> Reviewed-by: Andy Honig <ahonig@google.com>
> Signed-off-by: Cfir Cohen <cfir@google.com>
> Reviewed-by: Liran Alon <liran.alon@oracle.com>
> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
>
> CVE-2018-16882
>
> (cherry picked from commit c2dd5146e9fe1f22c77c1b011adf84eea0245806)
> Signed-off-by: Tyler Hicks <tyhicks@canonical.com>


Acked-by: Kleber Sacilotto de Souza <kleber.souza@canonical.com>


> ---
>  arch/x86/kvm/vmx.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
> index 9fcc3ec3ab78..cb8c74ab0586 100644
> --- a/arch/x86/kvm/vmx.c
> +++ b/arch/x86/kvm/vmx.c
> @@ -11028,6 +11028,8 @@ static void nested_get_vmcs12_pages(struct kvm_vcpu *vcpu,
>  			kunmap(vmx->nested.pi_desc_page);
>  			kvm_release_page_dirty(vmx->nested.pi_desc_page);
>  			vmx->nested.pi_desc_page = NULL;
> +			vmx->nested.pi_desc = NULL;
> +			vmcs_write64(POSTED_INTR_DESC_ADDR, -1ull);
>  		}
>  		page = kvm_vcpu_gpa_to_page(vcpu, vmcs12->posted_intr_desc_addr);
>  		if (is_error_page(page))

Patch

diff --git a/arch/x86/kvm/vmx.c b/arch/x86/kvm/vmx.c
index 9fcc3ec3ab78..cb8c74ab0586 100644
--- a/arch/x86/kvm/vmx.c
+++ b/arch/x86/kvm/vmx.c
@@ -11028,6 +11028,8 @@  static void nested_get_vmcs12_pages(struct kvm_vcpu *vcpu,
 			kunmap(vmx->nested.pi_desc_page);
 			kvm_release_page_dirty(vmx->nested.pi_desc_page);
 			vmx->nested.pi_desc_page = NULL;
+			vmx->nested.pi_desc = NULL;
+			vmcs_write64(POSTED_INTR_DESC_ADDR, -1ull);
 		}
 		page = kvm_vcpu_gpa_to_page(vcpu, vmcs12->posted_intr_desc_addr);
 		if (is_error_page(page))