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

Message ID 1508936039-21254-3-git-send-email-stefan.bader@canonical.com
State New
Headers show
Series
  • [1/2] KVM: nVMX: update last_nonleaf_level when initializing nested EPT
Related show

Commit Message

Stefan Bader Oct. 25, 2017, 12:53 p.m.
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.

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>

CVE-2017-12188
(cherry-picked from commit 829ee279aed43faa5cb1e4d65c0cad52f2426c53)
Signed-off-by: Stefan Bader <stefan.bader@canonical.com>
---
 arch/x86/kvm/mmu.c         | 14 +++++++-------
 arch/x86/kvm/paging_tmpl.h |  3 ++-
 2 files changed, 9 insertions(+), 8 deletions(-)

Comments

Colin King Oct. 25, 2017, 1:06 p.m. | #1
On 25/10/17 14:53, Stefan Bader wrote:
> 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.
> 
> 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>
> 
> CVE-2017-12188
> (cherry-picked from commit 829ee279aed43faa5cb1e4d65c0cad52f2426c53)
> Signed-off-by: Stefan Bader <stefan.bader@canonical.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 1e8a0e0..1c24c45 100644
> --- a/arch/x86/kvm/mmu.c
> +++ b/arch/x86/kvm/mmu.c
> @@ -3638,19 +3638,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 a011054..3736390 100644
> --- a/arch/x86/kvm/paging_tmpl.h
> +++ b/arch/x86/kvm/paging_tmpl.h
> @@ -324,10 +324,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;
>  
> 
Clean cherry pick.

Acked-by: Colin Ian King <colin.king@canonical.com>

Patch

diff --git a/arch/x86/kvm/mmu.c b/arch/x86/kvm/mmu.c
index 1e8a0e0..1c24c45 100644
--- a/arch/x86/kvm/mmu.c
+++ b/arch/x86/kvm/mmu.c
@@ -3638,19 +3638,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 a011054..3736390 100644
--- a/arch/x86/kvm/paging_tmpl.h
+++ b/arch/x86/kvm/paging_tmpl.h
@@ -324,10 +324,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;