Re: [PATCH v4 4/4] KVM: mmu: remove over-aggressive warnings

[Sean Christopherson <seanjc@google.com>]

On Wed, Oct 13, 2021, David Stevens wrote:
> On Wed, Oct 13, 2021 at 9:02 AM Sean Christopherson <seanjc@google.com> wrote:
> >
> > On Wed, Sep 29, 2021, David Stevens wrote:
> > > From: David Stevens <stevensd@chromium.org>
> > >
> > > Remove two warnings that require ref counts for pages to be non-zero, as
> > > mapped pfns from follow_pfn may not have an initialized ref count.
> > >
> > > Signed-off-by: David Stevens <stevensd@chromium.org>
> > > ---
> > >  arch/x86/kvm/mmu/mmu.c | 7 -------
> > >  virt/kvm/kvm_main.c    | 2 +-
> > >  2 files changed, 1 insertion(+), 8 deletions(-)
> > >
> > > diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
> > > index 5a1adcc9cfbc..3b469df63bcf 100644
> > > --- a/arch/x86/kvm/mmu/mmu.c
> > > +++ b/arch/x86/kvm/mmu/mmu.c
> > > @@ -617,13 +617,6 @@ static int mmu_spte_clear_track_bits(struct kvm *kvm, u64 *sptep)
> > >
> > >       pfn = spte_to_pfn(old_spte);
> > >
> > > -     /*
> > > -      * KVM does not hold the refcount of the page used by
> > > -      * kvm mmu, before reclaiming the page, we should
> > > -      * unmap it from mmu first.
> > > -      */
> > > -     WARN_ON(!kvm_is_reserved_pfn(pfn) && !page_count(pfn_to_page(pfn)));
> >
> > Have you actually observed false positives with this WARN?  I would expect anything
> > without a struct page to get filtered out by !kvm_is_reserved_pfn(pfn).
> 
> Those are the type of pfns that were responsible for CVE-2021-22543
> [1]. One specific example is that amdgpu uses ttm_pool, which makes
> higher order, non-compound allocation. Without the head/tail metadata,
> only the first base page in such an allocation has non-zero
> page_count.

Huh.  I hadn't actually read the CVE, or obviously thought critically about the
problem. :-)

> [1] https://github.com/google/security-research/security/advisories/GHSA-7wq5-phmq-m584
> 
> > If you have observed false positives, I would strongly prefer we find a way to
> > keep the page_count() sanity check, it has proven very helpful in the past in
> > finding/debugging bugs during MMU development.
> 
> When we see a refcount of zero, I think we can look up spte->(gfn,
> slot)->hva->vma and determine whether or not the zero refcount is
> okay, based on vm_flags. That's kind of heavy for a debug check,
> although at least we'd only pay the cost for unusual mappings. But it
> still might make sense to switch to a MMU_WARN_ON, in that case. Or we
> could just ignore the cost, since at least from a superficial reading
> and some basic tests, tdp_mmu doesn't seem to execute this code path.
> 
> Thoughts? I'd lean towards MMU_WARN_ON, but I'd like to know what the
> maintainers' preferences are before sending an updated patch series.

MMU_WARN_ON is a poor choice, but only because no one turns it on.  I think we've
discussed turning it into a proper Kconfig (and killing off mmu_audit.c) multiple
times, but no one has actually followed through.

The TDP MMU indeed doesn't hit this path.  So I'd say just keep this patch as is
and punt the whole MMU_WARN_ON / audit cleanup to the future.  I bet if we spend
any time at all, we can think of a big pile of MMU sanity checks we could add to
KVM, i.e. this can be but one of many checks that applies to all flavors of MMUs.