linux-kernel - Re: [PATCH 09/54] KVM: x86/mmu: Unconditionally zap unsync SPs when creating >4k SP at GFN

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] [thread-next>] [day] [month] [year] [list]

Message-ID: <YNNOeIWqNoZ3j8o+@google.com>
Date:   Wed, 23 Jun 2021 15:08:40 +0000
From:   Sean Christopherson <seanjc@...gle.com>
To:     Paolo Bonzini <pbonzini@...hat.com>
Cc:     Vitaly Kuznetsov <vkuznets@...hat.com>,
        Wanpeng Li <wanpengli@...cent.com>,
        Jim Mattson <jmattson@...gle.com>,
        Joerg Roedel <joro@...tes.org>, kvm@...r.kernel.org,
        linux-kernel@...r.kernel.org,
        Yu Zhang <yu.c.zhang@...ux.intel.com>,
        Maxim Levitsky <mlevitsk@...hat.com>
Subject: Re: [PATCH 09/54] KVM: x86/mmu: Unconditionally zap unsync SPs when
 creating >4k SP at GFN

On Wed, Jun 23, 2021, Paolo Bonzini wrote:
> On 22/06/21 19:56, Sean Christopherson wrote:
> > When creating a new upper-level shadow page, zap unsync shadow pages at
> > the same target gfn instead of attempting to sync the pages.  This fixes
> > a bug where an unsync shadow page could be sync'd with an incompatible
> > context, e.g. wrong smm, is_guest, etc... flags.  In practice, the bug is
> > relatively benign as sync_page() is all but guaranteed to fail its check
> > that the guest's desired gfn (for the to-be-sync'd page) matches the
> > current gfn associated with the shadow page.  I.e. kvm_sync_page() would
> > end up zapping the page anyways.
> > 
> > Alternatively, __kvm_sync_page() could be modified to explicitly verify
> > the mmu_role of the unsync shadow page is compatible with the current MMU
> > context.  But, except for this specific case, __kvm_sync_page() is called
> > iff the page is compatible, e.g. the transient sync in kvm_mmu_get_page()
> > requires an exact role match, and the call from kvm_sync_mmu_roots() is
> > only synchronizing shadow pages from the current MMU (which better be
> > compatible or KVM has problems).  And as described above, attempting to
> > sync shadow pages when creating an upper-level shadow page is unlikely
> > to succeed, e.g. zero successful syncs were observed when running Linux
> > guests despite over a million attempts.
> 
> One issue, this WARN_ON may now trigger:
> 
>                         WARN_ON(!list_empty(&invalid_list));
> 
> due to a kvm_mmu_prepare_zap_page that could have happened on an earlier
> iteration of the for_each_valid_sp.  Before your change, __kvm_sync_page
> would be called always before kvm_sync_pages could add anything to
> invalid_list.

Ah, I should have added a comment.  It took me a few minutes of staring to
remember why it can't fire.

The branch at (2), which adds to invalid_list, is taken if and only if the new
page is not a 4k page.

The branch at (3) is taken if and only if the existing page is a 4k page, because
only 4k pages can become unsync.

Because the shadow page's level is incorporated into its role, if the level of
the new page is >4k, the branch at (1) will be taken for all 4k shadow pages.

Maybe something like this for a comment?

			/*
			 * Assert that the page was not zapped if the "sync" was
			 * successful.  Note, this cannot collide with the above
			 * zapping of unsync pages, as this point is reached iff
			 * the new page is a 4k page (only 4k pages can become
			 * unsync and the role check ensures identical levels),
			 * and zapping occurs iff the new page is NOT a 4k page.
			 */
			WARN_ON(!list_empty(&invalid_list));




1)		if (sp->role.word != role.word) {
			/*
			 * If the guest is creating an upper-level page, zap
			 * unsync pages for the same gfn.  While it's possible
			 * the guest is using recursive page tables, in all
			 * likelihood the guest has stopped using the unsync
			 * page and is installing a completely unrelated page.
			 * Unsync pages must not be left as is, because the new
			 * upper-level page will be write-protected.
			 */
2)			if (level > PG_LEVEL_4K && sp->unsync)
				kvm_mmu_prepare_zap_page(vcpu->kvm, sp,
							 &invalid_list);
			continue;
		}

		if (direct_mmu)
			goto trace_get_page;

3)		if (sp->unsync) {
			/*
			 * The page is good, but is stale.  "Sync" the page to
			 * get the latest guest state, but don't write-protect
			 * the page and don't mark it synchronized!  KVM needs
			 * to ensure the mapping is valid, but doesn't need to
			 * fully sync (write-protect) the page until the guest
			 * invalidates the TLB mapping.  This allows multiple
			 * SPs for a single gfn to be unsync.
			 *
			 * If the sync fails, the page is zapped.  If so, break
			 * If so, break in order to rebuild it.
			 */
			if (!kvm_sync_page(vcpu, sp, &invalid_list))
				break;

			WARN_ON(!list_empty(&invalid_list));
			kvm_make_request(KVM_REQ_TLB_FLUSH_CURRENT, vcpu);
		}