linux-kernel - Re: [RFC PATCH v5 048/104] KVM: x86/tdp_mmu: Support TDX private mapping for TDP MMU

lists.openwall.net		lists / announce owl-users owl-dev john-users john-dev passwdqc-users yescrypt popa3d-users / oss-security kernel-hardening musl sabotage tlsify passwords / crypt-dev xvendor / Bugtraq Full-Disclosure linux-kernel linux-netdev linux-ext4 linux-hardening linux-cve-announce PHC
Open Source and information security mailing list archives

Hash Suite: Windows password security audit tool. GUI, reports in PDF.

[<prev] [next>] [<thread-prev] [day] [month] [year] [list]

Date:   Fri, 29 Apr 2022 00:46:08 +0000
From:   Sean Christopherson <seanjc@...gle.com>
To:     Sagi Shahar <sagis@...gle.com>
Cc:     "Yamahata, Isaku" <isaku.yamahata@...el.com>, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org, isaku.yamahata@...il.com,
        Paolo Bonzini <pbonzini@...hat.com>,
        Jim Mattson <jmattson@...gle.com>,
        Erdem Aktas <erdemaktas@...gle.com>,
        Connor Kuehl <ckuehl@...hat.com>
Subject: Re: [RFC PATCH v5 048/104] KVM: x86/tdp_mmu: Support TDX private
 mapping for TDP MMU

On Thu, Apr 28, 2022, Sagi Shahar wrote:
> > @@ -468,23 +503,49 @@ static void __handle_changed_spte(struct kvm *kvm, int as_id, gfn_t gfn,
> >
> >         if (was_leaf && is_dirty_spte(old_spte) &&
> >             (!is_present || !is_dirty_spte(new_spte) || pfn_changed))
> > -               kvm_set_pfn_dirty(spte_to_pfn(old_spte));
> > +               kvm_set_pfn_dirty(old_pfn);
> > +
> > +       /*
> > +        * Special handling for the private mapping.  We are either
> > +        * setting up new mapping at middle level page table, or leaf,
> > +        * or tearing down existing mapping.
> > +        */
> > +       if (private_spte) {
> > +               void *sept_page = NULL;
> > +
> > +               if (is_present && !is_leaf) {
> > +                       struct kvm_mmu_page *sp = to_shadow_page(pfn_to_hpa(new_pfn));
> > +
> > +                       sept_page = kvm_mmu_private_sp(sp);
> > +                       WARN_ON(!sept_page);
> > +                       WARN_ON(sp->role.level + 1 != level);
> > +                       WARN_ON(sp->gfn != gfn);
> > +               }
> > +
> > +               static_call(kvm_x86_handle_changed_private_spte)(
> > +                       kvm, gfn, level,
> > +                       old_pfn, was_present, was_leaf,
> > +                       new_pfn, is_present, is_leaf, sept_page);
> > +       }
> >
> >         /*
> >          * Recursively handle child PTs if the change removed a subtree from
> >          * the paging structure.
> >          */
> > -       if (was_present && !was_leaf && (pfn_changed || !is_present))
> > +       if (was_present && !was_leaf && (pfn_changed || !is_present)) {
> > +               WARN_ON(private_spte !=
> > +                       is_private_spte(spte_to_child_pt(old_spte, level)));

This sanity check is pointless.  The private flag comes from the parent shadow
page role, and that's not changing.

> > @@ -1015,6 +1137,12 @@ int kvm_tdp_mmu_map(struct kvm_vcpu *vcpu, struct kvm_page_fault *fault)
> >                     is_large_pte(iter.old_spte)) {
> >                         if (!tdp_mmu_zap_spte_atomic(vcpu->kvm, &iter))
> >                                 break;
> > +                       /*
> > +                        * TODO: large page support.
> > +                        * Doesn't support large page for TDX now
> > +                        */
> > +                       WARN_ON(is_private_spte(&iter.old_spte));
> 
> The above line is causing a null ptr dereferencing when running the
> KVM unit tests.
> It should be is_private_spte(iter.sptep) instead of
> is_private_spte(&iter.old_spte)
> While old_spte holds a snapshot of the value pointed to by sptep,
> &old_spte is not equivalent to sptep.

Bug aside, the name is really, really bad.  All of the existing helpers with an
"is_blah_spte()" name take an SPTE value, not a pointer to an SPTE.

is_private_sptep() is the obvious choice.  That makes me a bit nervous too, and
I don't love having to go back to the parent to query private vs shared.

That said, I think it's worth waiting to see the next version of this series before
going behind the bikeshed, I suspect many/most of the calls will go away, i.e. we
might find a better option presents itself.