Re: [PATCH 1/2] KVM: x86/mmu: Fix write-protection of PTs mapped by the TDP MMU

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



On Thu, Jan 13, 2022 at 9:04 AM David Matlack <dmatlack@xxxxxxxxxx> wrote:
>
> On Wed, Jan 12, 2022 at 4:29 PM Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
> >
> > On Wed, Jan 12, 2022, David Matlack wrote:
> > > On Wed, Jan 12, 2022 at 3:14 PM Sean Christopherson <seanjc@xxxxxxxxxx> wrote:
> > > >
> > > > On Wed, Jan 12, 2022, David Matlack wrote:
> > > > > When the TDP MMU is write-protection GFNs for page table protection (as
> > > > > opposed to for dirty logging, or due to the HVA not being writable), it
> > > > > checks if the SPTE is already write-protected and if so skips modifying
> > > > > the SPTE and the TLB flush.
> > > > >
> > > > > This behavior is incorrect because the SPTE may be write-protected for
> > > > > dirty logging. This implies that the SPTE could be locklessly be made
> > > > > writable on the next write access, and that vCPUs could still be running
> > > > > with writable SPTEs cached in their TLB.
> > > > >
> > > > > Fix this by unconditionally setting the SPTE and only skipping the TLB
> > > > > flush if the SPTE was already marked !MMU-writable or !Host-writable,
> > > > > which guarantees the SPTE cannot be locklessly be made writable and no
> > > > > vCPUs are running the writable SPTEs cached in their TLBs.
> > > > >
> > > > > Technically it would be safe to skip setting the SPTE as well since:
> > > > >
> > > > >   (a) If MMU-writable is set then Host-writable must be cleared
> > > > >       and the only way to set Host-writable is to fault the SPTE
> > > > >       back in entirely (at which point any unsynced shadow pages
> > > > >       reachable by the new SPTE will be synced and MMU-writable can
> > > > >       be safetly be set again).
> > > > >
> > > > >   and
> > > > >
> > > > >   (b) MMU-writable is never consulted on its own.
> > > > >
> > > > > And in fact this is what the shadow MMU does when write-protecting guest
> > > > > page tables. However setting the SPTE unconditionally is much easier to
> > > > > reason about and does not require a huge comment explaining why it is safe.
> > > >
> > > > I disagree.  I looked at the code+comment before reading the full changelog and
> > > > typed up a response saying the code should be:
> > > >
> > > >                 if (!is_writable_pte(iter.old_spte) &&
> > > >                     !spte_can_locklessly_be_made_writable(spte))
> > > >                         break;
> > > >
> > > > Then I went read the changelog and here we are :-)
> > > >
> > > > I find that much more easier to grok, e.g. in plain English: "if the SPTE isn't
> > > > writable and can't be made writable, there's nothing to do".
> > >
> > > Oh interesting. I actually find that confusing because it can easily
> > > lead to the MMU-writable bit staying set. Here we are protecting GFNs
> > > and we're opting to leave the MMU-writable bit set. It takes a lot of
> > > digging to figure out that this is safe because if MMU-writable is set
> > > and the SPTE cannot be locklessly be made writable then it implies
> > > Host-writable is clear, and Host-writable can't be reset without
> > > syncing the all shadow pages reachable by the MMU. Oh and the
> > > MMU-writable bit is never consulted on its own (e.g. We never iterate
> > > through all SPTEs to find the ones that are !MMU-writable).
> >
> > Ah, you've missed the other wrinkle: MMU-writable can bet set iff Host-writable
> > is set.  In other words, the MMU-writable bit is never left set because it can't
> > be set if spte_can_locklessly_be_made_writable() returns false.

The changed_pte notifier looks like it clears Host-writable without
clearing MMU-writable. Specifically the call chain:

kvm_mmu_notifier_change_pte()
  kvm_set_spte_gfn()
    kvm_tdp_mmu_set_spte_gfn()
      set_spte_gfn()
        kvm_mmu_changed_pte_notifier_make_spte()

Is there some guarantee that old_spte is !MMU-writable at this point?
If not I could easily change kvm_mmu_changed_pte_notifier_make_spte()
to also clear MMU-writable and preserve the invariant.


>
> Ohhh I did miss that and yes that explains it. I'll send another
> version of this patch that skips setting the SPTE unnecessarily.
>
> >
> > To reduce confusion, we can and probably should do:
> >
> > diff --git a/arch/x86/kvm/mmu/spte.h b/arch/x86/kvm/mmu/spte.h
> > index a4af2a42695c..bc691ff72cab 100644
> > --- a/arch/x86/kvm/mmu/spte.h
> > +++ b/arch/x86/kvm/mmu/spte.h
> > @@ -316,8 +316,7 @@ static __always_inline bool is_rsvd_spte(struct rsvd_bits_validate *rsvd_check,
> >
> >  static inline bool spte_can_locklessly_be_made_writable(u64 spte)
> >  {
> > -       return (spte & shadow_host_writable_mask) &&
> > -              (spte & shadow_mmu_writable_mask);
> > +       return (spte & shadow_mmu_writable_mask);
> >  }
> >
> >  static inline u64 get_mmio_spte_generation(u64 spte)
> >
> > Though it'd be nice to have a WARN somewhere to enforce that MMU-Writable isn't
> > set without Host-writable.
> >
> > We could also rename the helper to is_mmu_writable_spte(), though I'm not sure
> > that's actually better.
> >
> > Yet another option would be to invert the flag and make it shadow_mmu_pt_protected_mask
> > or something, i.e. make it more explicitly a flag that says "this thing is write-protected
> > for shadowing a page table".



[Index of Archives]     [Linux Kernel]     [Kernel Development Newbies]     [Linux USB Devel]     [Video for Linux]     [Linux Audio Users]     [Yosemite Hiking]     [Linux Kernel]     [Linux SCSI]

  Powered by Linux