[PATCH 0/2] KVM: nVMX: fix out-of-bounds access (CVE-2017-12188)

[PATCH 0/2] KVM: nVMX: fix out-of-bounds access (CVE-2017-12188)

[Paolo Bonzini]

Due to a combination of a bug in nEPT (patch 1), and a broken safety
net elsewhere in the MMU code (patch 2), a malicious guest could use
nested EPT to overwrite kernel memory.  In particular, the arrays in
struct guest_walker could be accessed with index -1 and the "level" and
"max_level" fields overwritten:

struct guest_walker {
        int level;
        unsigned max_level;
        gfn_t table_gfn[PT_MAX_FULL_LEVELS];
	...
}

Because the level field is used as an index into array, it is at least
possible to overwrite the kernel stack and this should be treated as a
possible guest-to-host escape on Intel hosts with nested virtualization
enabled.

While the incorrect code in patch 1 is present since Linux 3.12, the
bug only affects Linux kernels 4.6 and newer.  Therefore, stable kernels
only need to apply the second patch, which has the advantage of applying
more cleanly.

The bug was discovered by Ladislav (Ladi) Prosek from Red Hat.

Thanks,

Paolo

Ladi Prosek (2):
  KVM: nVMX: update last_nonleaf_level when initializing nested EPT
  KVM: MMU: always terminate page walks at level 1

 arch/x86/kvm/mmu.c         | 15 ++++++++-------
 arch/x86/kvm/paging_tmpl.h |  3 ++-
 2 files changed, 10 insertions(+), 8 deletions(-)

-- 
1.8.3.1

[PATCH 1/2] KVM: nVMX: update last_nonleaf_level when initializing nested EPT

[Paolo Bonzini]

From: Ladi Prosek <lprosek@redhat.com>

The function updates context->root_level but didn't call
update_last_nonleaf_level so the previous and potentially wrong value
was used for page walks.  For example, a zero value of last_nonleaf_level
would allow a potential out-of-bounds access in arch/x86/mmu/paging_tmpl.h's
walk_addr_generic function (CVE-2017-12188).

Fixes: 155a97a3d7c78b46cef6f1a973c831bc5a4f82bb
Signed-off-by: Ladi Prosek <lprosek@redhat.com>
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/kvm/mmu.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index 106d4a029a8a..3c25f20115bc 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -4555,6 +4555,7 @@ void kvm_init_shadow_ept_mmu(struct kvm_vcpu *vcpu, bool execonly,
 
 	update_permission_bitmask(vcpu, context, true);
 	update_pkru_bitmask(vcpu, context, true);
+	update_last_nonleaf_level(vcpu, context);
 	reset_rsvds_bits_mask_ept(vcpu, context, execonly);
 	reset_ept_shadow_zero_bits_mask(vcpu, context, execonly);
 }
-- 
1.8.3.1

[PATCH 2/2] KVM: MMU: always terminate page walks at level 1

[Paolo Bonzini]

From: Ladi Prosek <lprosek@redhat.com>

is_last_gpte() is not equivalent to the pseudo-code given in commit
6bb69c9b69c31 ("KVM: MMU: simplify last_pte_bitmap") because an incorrect
value of last_nonleaf_level may override the result even if level == 1.

It is critical for is_last_gpte() to return true on level == 1 to
terminate page walks. Otherwise memory corruption may occur as level
is used as an index to various data structures throughout the page
walking code.  Even though the actual bug would be wherever the MMU is
initialized (as in the previous patch), be defensive and ensure here
that is_last_gpte() returns the correct value.

This patch is also enough to fix CVE-2017-12188, and suggested for
stable and distro kernels.

Fixes: 6bb69c9b69c315200ddc2bc79aee14c0184cf5b2
Cc: stable@vger.kernel.org
Cc: Andy Honig <ahonig@google.com>
Signed-off-by: Ladi Prosek <lprosek@redhat.com>
[Panic if walk_addr_generic gets an incorrect level; this is a serious
 bug and it's not worth a WARN_ON where the recovery path might hide
 further exploitable issues; suggested by Andrew Honig. - Paolo]
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/kvm/mmu.c         | 14 +++++++-------
 arch/x86/kvm/paging_tmpl.h |  3 ++-
 2 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index 3c25f20115bc..7a69cf053711 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -3974,19 +3974,19 @@ static inline bool is_last_gpte(struct kvm_mmu *mmu,
 				unsigned level, unsigned gpte)
 {
 	/*
-	 * PT_PAGE_TABLE_LEVEL always terminates.  The RHS has bit 7 set
-	 * iff level <= PT_PAGE_TABLE_LEVEL, which for our purpose means
-	 * level == PT_PAGE_TABLE_LEVEL; set PT_PAGE_SIZE_MASK in gpte then.
-	 */
-	gpte |= level - PT_PAGE_TABLE_LEVEL - 1;
-
-	/*
 	 * The RHS has bit 7 set iff level < mmu->last_nonleaf_level.
 	 * If it is clear, there are no large pages at this level, so clear
 	 * PT_PAGE_SIZE_MASK in gpte if that is the case.
 	 */
 	gpte &= level - mmu->last_nonleaf_level;
 
+	/*
+	 * PT_PAGE_TABLE_LEVEL always terminates.  The RHS has bit 7 set
+	 * iff level <= PT_PAGE_TABLE_LEVEL, which for our purpose means
+	 * level == PT_PAGE_TABLE_LEVEL; set PT_PAGE_SIZE_MASK in gpte then.
+	 */
+	gpte |= level - PT_PAGE_TABLE_LEVEL - 1;
+
 	return gpte & PT_PAGE_SIZE_MASK;
 }
 
diff --git a/arch/x86/kvm/paging_tmpl.h b/arch/x86/kvm/paging_tmpl.h
index 86b68dc5a649..f18d1f8d332b 100644
--- a/arch/x86/kvm/paging_tmpl.h
+++ b/arch/x86/kvm/paging_tmpl.h
@@ -334,10 +334,11 @@ static int FNAME(walk_addr_generic)(struct guest_walker *walker,
 		--walker->level;
 
 		index = PT_INDEX(addr, walker->level);
-
 		table_gfn = gpte_to_gfn(pte);
 		offset    = index * sizeof(pt_element_t);
 		pte_gpa   = gfn_to_gpa(table_gfn) + offset;
+
+		BUG_ON(walker->level < 1);
 		walker->table_gfn[walker->level - 1] = table_gfn;
 		walker->pte_gpa[walker->level - 1] = pte_gpa;
 
-- 
1.8.3.1

[PATCH 0/2] KVM: nVMX: fix out-of-bounds access (CVE-2017-12188)

[Paolo Bonzini]

Due to a combination of a bug in nEPT (patch 1), and a broken safety
net elsewhere in the MMU code (patch 2), a malicious guest could use
nested EPT to overwrite kernel memory.  In particular, the arrays in
struct guest_walker could be accessed with index -1 and the "level" and
"max_level" fields overwritten:

struct guest_walker {
        int level;
        unsigned max_level;
        gfn_t table_gfn[PT_MAX_FULL_LEVELS];
	...
}

Because the level field is used as an index into array, it is at least
possible to overwrite the kernel stack and this should be treated as a
possible guest-to-host escape on Intel hosts with nested virtualization
enabled.

While the incorrect code in patch 1 is present since Linux 3.12, the
bug only affects Linux kernels 4.6 and newer.  Therefore, stable kernels
only need to apply the second patch, which has the advantage of applying
more cleanly.

The bug was discovered by Ladislav (Ladi) Prosek from Red Hat.

Thanks,

Paolo

Ladi Prosek (2):
  KVM: nVMX: update last_nonleaf_level when initializing nested EPT
  KVM: MMU: always terminate page walks at level 1

 arch/x86/kvm/mmu.c         | 15 ++++++++-------
 arch/x86/kvm/paging_tmpl.h |  3 ++-
 2 files changed, 10 insertions(+), 8 deletions(-)

-- 
1.8.3.1

[PATCH 2/2] KVM: MMU: always terminate page walks at level 1

[Paolo Bonzini]

From: Ladi Prosek <lprosek@redhat.com>

is_last_gpte() is not equivalent to the pseudo-code given in commit
6bb69c9b69c31 ("KVM: MMU: simplify last_pte_bitmap") because an incorrect
value of last_nonleaf_level may override the result even if level == 1.

It is critical for is_last_gpte() to return true on level == 1 to
terminate page walks. Otherwise memory corruption may occur as level
is used as an index to various data structures throughout the page
walking code.  Even though the actual bug would be wherever the MMU is
initialized (as in the previous patch), be defensive and ensure here
that is_last_gpte() returns the correct value.

This patch is also enough to fix CVE-2017-12188, and suggested for
stable and distro kernels.

Fixes: 6bb69c9b69c315200ddc2bc79aee14c0184cf5b2
Cc: stable@vger.kernel.org
Cc: Andy Honig <ahonig@google.com>
Signed-off-by: Ladi Prosek <lprosek@redhat.com>
[Panic if walk_addr_generic gets an incorrect level; this is a serious
 bug and it's not worth a WARN_ON where the recovery path might hide
 further exploitable issues; suggested by Andrew Honig. - Paolo]
Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
---
 arch/x86/kvm/mmu.c         | 14 +++++++-------
 arch/x86/kvm/paging_tmpl.h |  3 ++-
 2 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index 3c25f20115bc..7a69cf053711 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -3974,19 +3974,19 @@ static inline bool is_last_gpte(struct kvm_mmu *mmu,
 				unsigned level, unsigned gpte)
 {
 	/*
-	 * PT_PAGE_TABLE_LEVEL always terminates.  The RHS has bit 7 set
-	 * iff level <= PT_PAGE_TABLE_LEVEL, which for our purpose means
-	 * level == PT_PAGE_TABLE_LEVEL; set PT_PAGE_SIZE_MASK in gpte then.
-	 */
-	gpte |= level - PT_PAGE_TABLE_LEVEL - 1;
-
-	/*
 	 * The RHS has bit 7 set iff level < mmu->last_nonleaf_level.
 	 * If it is clear, there are no large pages at this level, so clear
 	 * PT_PAGE_SIZE_MASK in gpte if that is the case.
 	 */
 	gpte &= level - mmu->last_nonleaf_level;
 
+	/*
+	 * PT_PAGE_TABLE_LEVEL always terminates.  The RHS has bit 7 set
+	 * iff level <= PT_PAGE_TABLE_LEVEL, which for our purpose means
+	 * level == PT_PAGE_TABLE_LEVEL; set PT_PAGE_SIZE_MASK in gpte then.
+	 */
+	gpte |= level - PT_PAGE_TABLE_LEVEL - 1;
+
 	return gpte & PT_PAGE_SIZE_MASK;
 }
 
diff --git a/arch/x86/kvm/paging_tmpl.h b/arch/x86/kvm/paging_tmpl.h
index 86b68dc5a649..f18d1f8d332b 100644
--- a/arch/x86/kvm/paging_tmpl.h
+++ b/arch/x86/kvm/paging_tmpl.h
@@ -334,10 +334,11 @@ static int FNAME(walk_addr_generic)(struct guest_walker *walker,
 		--walker->level;
 
 		index = PT_INDEX(addr, walker->level);
-
 		table_gfn = gpte_to_gfn(pte);
 		offset    = index * sizeof(pt_element_t);
 		pte_gpa   = gfn_to_gpa(table_gfn) + offset;
+
+		BUG_ON(walker->level < 1);
 		walker->table_gfn[walker->level - 1] = table_gfn;
 		walker->pte_gpa[walker->level - 1] = pte_gpa;
 
-- 
1.8.3.1