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

[Colin Ian King]

On 25/10/17 14:53, Stefan Bader wrote:
> From: Ladi Prosek <lprosek at 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 at vger.kernel.org
> Cc: Andy Honig <ahonig at google.com>
> Signed-off-by: Ladi Prosek <lprosek at 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 at redhat.com>
> 
> CVE-2017-12188
> (cherry-picked from commit 829ee279aed43faa5cb1e4d65c0cad52f2426c53)
> Signed-off-by: Stefan Bader <stefan.bader at 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 at canonical.com>