All of lore.kernel.org
 help / color / mirror / Atom feed
From: Sean Christopherson <seanjc@google.com>
To: Jinu Kim <kimjw04271234@gmail.com>
Cc: Paolo Bonzini <pbonzini@redhat.com>,
	kvm@vger.kernel.org, linux-kernel@vger.kernel.org,
	 x86@kernel.org
Subject: Re: [PATCH v2] KVM: x86/mmu: Write-protect tracked GFNs in all address spaces
Date: Wed, 5 Aug 2026 12:14:04 -0700	[thread overview]
Message-ID: <anOLfLhAGbOXT30p@google.com> (raw)
In-Reply-To: <CAH+3ta4uD-QYju=6YRqHvd4Vwa-WAa9T9a65soCzdu49wNxV8g@mail.gmail.com>

On Wed, Aug 05, 2026, Jinu Kim wrote:
> Thanks. After considering your comments, I think v2 is trying to solve a
> broader problem than the one reported, and that the resulting complexity
> is difficult to justify.
> 
> One thing I do not understand is the proposed revert of 0f38453cdb2e.
> The original pte_list_remove() panic was reproduced on then-current
> mainline with 0cb2af2ea66a, 81ccda30b4e8, and aad885e774966 already
> present.  The panic remained reachable there, and 0f38453cdb2e stopped
> it.  How would those three commits prevent the original upper-level
> shadow page from becoming unsync?

That's why I prefaced that with "Assuming the true badness referenced by commits";
it wasn't clear to me how marking an upper-level SP as unsync leads to a corrupted
rmap, and I hadn't thought too hard about it.

I assume it gets triggered by way of FNAME(sync_spte)() calling drop_spte(),
either directly or via FNAME(prefetch_invalid_gpte)().  Though it's somewhat of
a moot point because marking an upper-level SP unsync triggers a pile of WARNs
in so many other places.

Hmm, but *if* we decide to officially say cross-address-space gPTE writes are
unsupported, then I think I'd vote to revert (to make it abundantly clear that
the behavior is unsupported), and then suppress the issue by skipping like so:

diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
index c519e8e8d646..34d3949b6362 100644
--- a/arch/x86/kvm/mmu/mmu.c
+++ b/arch/x86/kvm/mmu/mmu.c
@@ -2997,6 +2997,12 @@ int mmu_try_to_unsync_pages(struct kvm *kvm, const struct kvm_memory_slot *slot,
                if (prefetch)
                        return -EEXIST;
 
+               /* Comment here about abusing SMM. */
+               if (sp->role.level != PG_LEVEL_4K) {
+                       WARN_ON_ONCE(!!sp->role.smm == !!slot->as_id);
+                       continue;
+               }
+
                /*
                 * TDP MMU page faults require an additional spinlock as they
                 * run with mmu_lock held for read, not write, and the unsync
@@ -3020,7 +3026,6 @@ int mmu_try_to_unsync_pages(struct kvm *kvm, const struct kvm_memory_slot *slot,
                                continue;
                }
 
-               WARN_ON_ONCE(sp->role.level != PG_LEVEL_4K);
                kvm_unsync_page(kvm, sp);
        }
        if (locked)

Actually, irrespective of what we do with SMM, we should harden KVM to skip
marking upper-level SPs as unsync, because while corrupting guest memory is bad,
corrupting guest memory *and* crashing/compromising the host is worse.

So as an immediate defense-in-depth, I think this? (BUG the VM to reduce the
probability of the guest consuming corrupted data).

diff --git a/arch/x86/kvm/mmu/mmu.c b/arch/x86/kvm/mmu/mmu.c
index c519e8e8d646..8d53d37750c5 100644
--- a/arch/x86/kvm/mmu/mmu.c
+++ b/arch/x86/kvm/mmu/mmu.c
@@ -2997,6 +2997,9 @@ int mmu_try_to_unsync_pages(struct kvm *kvm, const struct kvm_memory_slot *slot,
                if (prefetch)
                        return -EEXIST;
 
+               if (KVM_BUG_ON(sp->role.level != PG_LEVEL_4K, kvm))
+                       continue;
+
                /*
                 * TDP MMU page faults require an additional spinlock as they
                 * run with mmu_lock held for read, not write, and the unsync
@@ -3020,7 +3023,6 @@ int mmu_try_to_unsync_pages(struct kvm *kvm, const struct kvm_memory_slot *slot,
                                continue;
                }
 
-               WARN_ON_ONCE(sp->role.level != PG_LEVEL_4K);
                kvm_unsync_page(kvm, sp);
        }
        if (locked)


> Your comments also made me separate the general limitations of write
> tracking from a narrower issue in this case.  I understand that KVM
> cannot guarantee write tracking for every way guest page-table memory can
> be modified, and I have not established a current-mainline host-security
> consequence for the remaining cross-address-space revocation issue.
> 
> In that narrower framing, there may still be something worth fixing.  In
> the reported SMM configuration, KVM creates CPU SPTEs in both address
> spaces for the same GFN and backing page, accounts that GFN as backing an
> indirect shadow page, but can leave the peer SPTE MMU-writable.  KVM's
> shadow-page accounting state and the permissions installed by KVM are
> therefore inconsistent with each other.

I agree it's a bug, I just don't want to fix it. :-)

> Fixing that local mismatch would not imply support for DMA, host writes,
> arbitrary aliases, or a general guarantee that KVM observes all writes to
> guest page-table memory.  Those cases can remain unsupported and be
> documented as such.
> 
> If this narrower boundary makes sense to you, I will rework the patch
> around the existing shadow-page accounting and synchronization
> transitions.  A replacement would keep the normal mapping and memslot
> lifecycle paths unchanged and avoid introducing persistent
> cross-address-space state.

Honestly, I'd wrather support host userspace writes than cross-address-space
writes.  At least those could have a somewhat plausible use case, e.g. if userspace
were to implement its own emulator.

But I am also very biased against KVM's SMM emulation, which is why I want Paolo's
input.

      reply	other threads:[~2026-08-05 19:14 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2026-08-04 10:57 [PATCH v2] KVM: x86/mmu: Write-protect tracked GFNs in all address spaces Jinu Kim
2026-08-04 13:10 ` Sean Christopherson
2026-08-05  8:24   ` Jinu Kim
2026-08-05 19:14     ` Sean Christopherson [this message]

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=anOLfLhAGbOXT30p@google.com \
    --to=seanjc@google.com \
    --cc=kimjw04271234@gmail.com \
    --cc=kvm@vger.kernel.org \
    --cc=linux-kernel@vger.kernel.org \
    --cc=pbonzini@redhat.com \
    --cc=x86@kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.