mbox series

[0/4] KVM: x86/mmu: Fix TDP MMU dirty logging bug L2 running with EPT disabled

Message ID 20240315230541.1635322-1-dmatlack@google.com (mailing list archive)
Headers show
Series KVM: x86/mmu: Fix TDP MMU dirty logging bug L2 running with EPT disabled | expand

Message

David Matlack March 15, 2024, 11:05 p.m. UTC
Fix a bug in the TDP MMU caught by syzkaller and CONFIG_KVM_PROVE_MMU
that causes writes made by L2 to no be reflected in the dirty log when
L1 has disabled EPT.

Patch 1 contains the fix. Patch 2 and 3 fix comments related to clearing
dirty bits in the TDP MMU. Patch 4 adds selftests coverage of dirty
logging of L2 when L1 has disabled EPT. i.e.  a regression test for this
bug.

David Matlack (4):
  KVM: x86/mmu: Check kvm_mmu_page_ad_need_write_protect() when clearing
    TDP MMU dirty bits
  KVM: x86/mmu: Remove function comments above
    clear_dirty_{gfn_range,pt_masked}()
  KVM: x86/mmu: Fix and clarify comments about clearing D-bit vs.
    write-protecting
  KVM: selftests: Add coverage of EPT-disabled to vmx_dirty_log_test

 arch/x86/kvm/mmu/tdp_mmu.c                    | 51 +++++++---------
 .../selftests/kvm/x86_64/vmx_dirty_log_test.c | 60 ++++++++++++++-----
 2 files changed, 68 insertions(+), 43 deletions(-)


base-commit: 687d8f4c3dea0758afd748968d91288220bbe7e3

Comments

Sean Christopherson April 10, 2024, 12:19 a.m. UTC | #1
On Fri, 15 Mar 2024 16:05:37 -0700, David Matlack wrote:
> Fix a bug in the TDP MMU caught by syzkaller and CONFIG_KVM_PROVE_MMU
> that causes writes made by L2 to no be reflected in the dirty log when
> L1 has disabled EPT.
> 
> Patch 1 contains the fix. Patch 2 and 3 fix comments related to clearing
> dirty bits in the TDP MMU. Patch 4 adds selftests coverage of dirty
> logging of L2 when L1 has disabled EPT. i.e.  a regression test for this
> bug.
> 
> [...]

Applied to kvm-x86 fixes, with the various tweaks mentioned in reply, and the
s/READ_ONCE/WRITE_ONCE fixup.  A sanity check would be nice though, I botched
the first attempt at the fixup (the one time I _should_ have copy+pasted code...).

Thanks!

[1/4] KVM: x86/mmu: Check kvm_mmu_page_ad_need_write_protect() when clearing TDP MMU dirty bits
      https://github.com/kvm-x86/linux/commit/b44914b27e6b
[2/4] KVM: x86/mmu: Remove function comments above clear_dirty_{gfn_range,pt_masked}()
      https://github.com/kvm-x86/linux/commit/d0adc4ce20e8
[3/4] KVM: x86/mmu: Fix and clarify comments about clearing D-bit vs. write-protecting
      https://github.com/kvm-x86/linux/commit/5709b14d1cea
[4/4] KVM: selftests: Add coverage of EPT-disabled to vmx_dirty_log_test
      https://github.com/kvm-x86/linux/commit/1d24b536d85b

--
https://github.com/kvm-x86/linux/tree/next
David Matlack April 10, 2024, 4:05 p.m. UTC | #2
On Tue, Apr 9, 2024 at 5:20 PM Sean Christopherson <seanjc@google.com> wrote:
>
> On Fri, 15 Mar 2024 16:05:37 -0700, David Matlack wrote:
> > Fix a bug in the TDP MMU caught by syzkaller and CONFIG_KVM_PROVE_MMU
> > that causes writes made by L2 to no be reflected in the dirty log when
> > L1 has disabled EPT.
> >
> > Patch 1 contains the fix. Patch 2 and 3 fix comments related to clearing
> > dirty bits in the TDP MMU. Patch 4 adds selftests coverage of dirty
> > logging of L2 when L1 has disabled EPT. i.e.  a regression test for this
> > bug.
> >
> > [...]
>
> Applied to kvm-x86 fixes, with the various tweaks mentioned in reply, and the
> s/READ_ONCE/WRITE_ONCE fixup.  A sanity check would be nice though, I botched
> the first attempt at the fixup (the one time I _should_ have copy+pasted code...).
>
> Thanks!
>
> [1/4] KVM: x86/mmu: Check kvm_mmu_page_ad_need_write_protect() when clearing TDP MMU dirty bits
>       https://github.com/kvm-x86/linux/commit/b44914b27e6b
> [2/4] KVM: x86/mmu: Remove function comments above clear_dirty_{gfn_range,pt_masked}()
>       https://github.com/kvm-x86/linux/commit/d0adc4ce20e8
> [3/4] KVM: x86/mmu: Fix and clarify comments about clearing D-bit vs. write-protecting
>       https://github.com/kvm-x86/linux/commit/5709b14d1cea
> [4/4] KVM: selftests: Add coverage of EPT-disabled to vmx_dirty_log_test
>       https://github.com/kvm-x86/linux/commit/1d24b536d85b

This commit does not have the WRITE_ONCE() fixup, but when I look at
the commits in the fixes branch itself I see [1] which is correct.

The other commits look good as well, thanks for the various fixups.

[1] https://github.com/kvm-x86/linux/commit/f1ef5c343399ec5e21eb1e1e0093e731dce2fa1e

>
> --
> https://github.com/kvm-x86/linux/tree/next
Sean Christopherson April 10, 2024, 4:17 p.m. UTC | #3
On Wed, Apr 10, 2024, David Matlack wrote:
> On Tue, Apr 9, 2024 at 5:20 PM Sean Christopherson <seanjc@google.com> wrote:
> >
> > On Fri, 15 Mar 2024 16:05:37 -0700, David Matlack wrote:
> > > Fix a bug in the TDP MMU caught by syzkaller and CONFIG_KVM_PROVE_MMU
> > > that causes writes made by L2 to no be reflected in the dirty log when
> > > L1 has disabled EPT.
> > >
> > > Patch 1 contains the fix. Patch 2 and 3 fix comments related to clearing
> > > dirty bits in the TDP MMU. Patch 4 adds selftests coverage of dirty
> > > logging of L2 when L1 has disabled EPT. i.e.  a regression test for this
> > > bug.
> > >
> > > [...]
> >
> > Applied to kvm-x86 fixes, with the various tweaks mentioned in reply, and the
> > s/READ_ONCE/WRITE_ONCE fixup.  A sanity check would be nice though, I botched
> > the first attempt at the fixup (the one time I _should_ have copy+pasted code...).
> >
> > Thanks!
> >
> > [1/4] KVM: x86/mmu: Check kvm_mmu_page_ad_need_write_protect() when clearing TDP MMU dirty bits
> >       https://github.com/kvm-x86/linux/commit/b44914b27e6b
> > [2/4] KVM: x86/mmu: Remove function comments above clear_dirty_{gfn_range,pt_masked}()
> >       https://github.com/kvm-x86/linux/commit/d0adc4ce20e8
> > [3/4] KVM: x86/mmu: Fix and clarify comments about clearing D-bit vs. write-protecting
> >       https://github.com/kvm-x86/linux/commit/5709b14d1cea
> > [4/4] KVM: selftests: Add coverage of EPT-disabled to vmx_dirty_log_test
> >       https://github.com/kvm-x86/linux/commit/1d24b536d85b
> 
> This commit does not have the WRITE_ONCE() fixup, but when I look at
> the commits in the fixes branch itself I see [1] which is correct.

Argh, I must have forgot to copy+paste in the correct hashes (like I said above,
it took me a few tries to get things right).

For posterity...

[1/4] KVM: x86/mmu: Write-protect L2 SPTEs in TDP MMU when clearing dirty status
      https://github.com/kvm-x86/linux/commit/b44914b27e6b
[2/4] KVM: x86/mmu: Remove function comments above clear_dirty_{gfn_range,pt_masked}()
      https://github.com/kvm-x86/linux/commit/d0adc4ce20e8
[3/4] KVM: x86/mmu: Fix and clarify comments about clearing D-bit vs. write-protecting
      https://github.com/kvm-x86/linux/commit/5709b14d1cea
[4/4] KVM: selftests: Add coverage of EPT-disabled to vmx_dirty_log_test
      https://github.com/kvm-x86/linux/commit/f1ef5c343399