Received-SPF: pass (google.com: domain of linux-kernel-owner@vger.kernel.org designates 2620:137:e000::1:20 as permitted sender) client-ip=2620:137:e000::1:20;
Date:   Fri, 11 Feb 2022 00:24:00 +0000
From:   Sean Christopherson <seanjc@google.com>
To:     Paolo Bonzini <pbonzini@redhat.com>
Cc:     linux-kernel@vger.kernel.org, kvm@vger.kernel.org,
        vkuznets@redhat.com, mlevitsk@redhat.com, dmatlack@google.com
Subject: Re: [PATCH 05/12] KVM: MMU: avoid NULL-pointer dereference on page
 freeing bugs
Message-ID: <YgWsoKskWnahgR8j@google.com>
References: <20220209170020.1775368-1-pbonzini@redhat.com>
 <20220209170020.1775368-6-pbonzini@redhat.com>
MIME-Version: 1.0
Content-Type: text/plain; charset=us-ascii
Content-Disposition: inline
In-Reply-To: <20220209170020.1775368-6-pbonzini@redhat.com>
Precedence: bulk

On Wed, Feb 09, 2022, Paolo Bonzini wrote:
> If kvm_mmu_free_roots encounters a PAE page table where a 64-bit page
> table is expected, the result is a NULL pointer dereference.  Instead
> just WARN and exit.

This confused the heck out of me, because we obviously free PAE page tables.  What
we don't do is back the root that gets shoved into CR3 with a shadow page.  It'd
be especially confusing without the context that this WARN was helpful during
related development, as it's not super obvious why mmu_free_root_page() is a special
snowflake and deserves a WARN.

Something like this?

  WARN and bail if KVM attempts to free a root that isn't backed by a shadow
  page.  KVM allocates a bare page for "special" roots, e.g. when using PAE
  paging or shadowing 2/3/4-level page tables with 4/5-level, and so root_hpa
  will be valid but won't be backed by a shadow page.  It's all too easy to
  blindly call mmu_free_root_page() on root_hpa, be nice and WARN instead of
  crashing KVM and possibly the kernel.

> Signed-off-by: Paolo Bonzini <pbonzini@redhat.com>
> ---
>  arch/x86/kvm/mmu/mmu.c | 2 ++
>  1 file changed, 2 insertions(+)
> 
> diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> index 7b5765ced928..d0f2077bd798 100644
> --- a/arch/x86/kvm/mmu/mmu.c
> +++ b/arch/x86/kvm/mmu/mmu.c
> @@ -3201,6 +3201,8 @@ static void mmu_free_root_page(struct kvm *kvm, hpa_t *root_hpa,
>  		return;
>  
>  	sp = to_shadow_page(*root_hpa & PT64_BASE_ADDR_MASK);
> +	if (WARN_ON(!sp))

Should this be KVM_BUG_ON()?  I.e. when you triggered these, would continuing on
potentially corrupt guest data, or was it truly benign-ish?

> +		return;
>  
>  	if (is_tdp_mmu_page(sp))
>  		kvm_tdp_mmu_put_root(kvm, sp, false);
> -- 
> 2.31.1
> 
>