linux-kernel - Re: [PATCH v2] KVM: nVMX: Mark APIC access page dirty when syncing vmcs12 pages

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]

Message-ID: <aRtpzxkVfY1g-Llp@google.com>
Date: Mon, 17 Nov 2025 10:30:39 -0800
From: Sean Christopherson <seanjc@...gle.com>
To: Fred Griffoul <griffoul@...il.com>
Cc: kvm@...r.kernel.org, Fred Griffoul <fgriffo@...zon.co.uk>, 
	Paolo Bonzini <pbonzini@...hat.com>, Thomas Gleixner <tglx@...utronix.de>, Ingo Molnar <mingo@...hat.com>, 
	Borislav Petkov <bp@...en8.de>, Dave Hansen <dave.hansen@...ux.intel.com>, x86@...nel.org, 
	"H. Peter Anvin" <hpa@...or.com>, linux-kernel@...r.kernel.org
Subject: Re: [PATCH v2] KVM: nVMX: Mark APIC access page dirty when syncing
 vmcs12 pages

On Wed, Sep 10, 2025, Fred Griffoul wrote:
> From: Fred Griffoul <fgriffo@...zon.co.uk>
> 
> For consistency with commit 7afe79f5734a ("KVM: nVMX: Mark vmcs12's APIC
> access page dirty when unmapping"), which marks the page dirty during
> unmap operations, also mark it dirty during vmcs12 page synchronization.
> 
> Signed-off-by: Fred Griffoul <fgriffo@...zon.co.uk>
> ---
> v2: Fix commit ID to use 12 chars instead of 11 (checkpatch warning)
> 
>  arch/x86/kvm/vmx/nested.c | 8 ++++----
>  1 file changed, 4 insertions(+), 4 deletions(-)
> 
> diff --git a/arch/x86/kvm/vmx/nested.c b/arch/x86/kvm/vmx/nested.c
> index b8ea1969113d..02aee6dd1698 100644
> --- a/arch/x86/kvm/vmx/nested.c
> +++ b/arch/x86/kvm/vmx/nested.c
> @@ -3916,10 +3916,10 @@ void nested_mark_vmcs12_pages_dirty(struct kvm_vcpu *vcpu)
>  	struct vmcs12 *vmcs12 = get_vmcs12(vcpu);
>  	gfn_t gfn;
> 
> -	/*
> -	 * Don't need to mark the APIC access page dirty; it is never
> -	 * written to by the CPU during APIC virtualization.
> -	 */
> +	if (nested_cpu_has2(vmcs12, SECONDARY_EXEC_VIRTUALIZE_APIC_ACCESSES)) {
> +		gfn = vmcs12->apic_access_addr >> PAGE_SHIFT;
> +		kvm_vcpu_mark_page_dirty(vcpu, gfn);

Hrm, marking the page dirty in vmx_complete_nested_posted_interrupt() is
unnecessary, because that function is marking the vAPIC and PID pages as dirty
because it explicitly writes those pages.  Not the end of the world, but I think
we can clean up another over-dirtying issue at the same time.

If nested_get_vmcs12_pages() didn't actually map memory into the guest, there's
no need to mark the gfn dirty, as the underlying page is unreachable.  If we add
a helper too fix that flag:

static inline void kvm_vcpu_map_mark_dirty(struct kvm_vcpu *vcpu,
					   struct kvm_host_map *map)
{
	if (kvm_vcpu_mapped(map))
		kvm_vcpu_mark_page_dirty(vcpu, map->gfn);
}

then we can have vmx_complete_nested_posted_interrupt() mark exactly the pages
it writes as dirty (which for me is more about documenting what the code is doing
as opposed to caring about spuriously marking a page dirty).

	kvm_vcpu_map_mark_dirty(vcpu, &vmx->nested.virtual_apic_map);
	kvm_vcpu_map_mark_dirty(vcpu, &vmx->nested.pi_desc_map);

Ugh, and looking at the details made me realize __kvm_vcpu_map() is buggy.  It
uses gfn_to_memslot() instead of kvm_vcpu_gfn_to_memslot().  Luckily, it's benign
as __kvm_vcpu_map() isn't reachable while the vCPU is "in" SMM.

I'll send a v2 as a small series, i.e. with this as the final patch.

> +	}
> 
>  	if (nested_cpu_has(vmcs12, CPU_BASED_TPR_SHADOW)) {
>  		gfn = vmcs12->virtual_apic_page_addr >> PAGE_SHIFT;
> --
> 2.43.0
>